[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Blue Swirl]

On Tue, Jan 12, 2010 at 7:29 PM, Blue Swirl <address@hidden> wrote:
> On Mon, Jan 11, 2010 at 10:33 PM, Igor Kovalenko
> <address@hidden> wrote:
>> On Tue, Jan 12, 2010 at 12:29 AM, Blue Swirl <address@hidden> wrote:
>>> On Sun, Jan 10, 2010 at 6:41 PM, Blue Swirl <address@hidden> wrote:
>>>> On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl <address@hidden> wrote:
>>>>> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <address@hidden> wrote:
>>>>>> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>>>>>>>
>>>>>>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>>>>>>>
>>>>>>> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>>>>>>> >>
>>>>>>> >> On 03.01.2010, at 18:29, Michael S. Tsirkin wrote:
>>>>>>> >>
>>>>>>> >>> On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>>>>>>> >>>>
>>>>>>> >>>> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>>>>>>> >>>>
>>>>>>> >>>>> On Sun, Jan 03, 2010 at 02:50:45AM +0100, Alexander Graf wrote:
>>>>>>> >>>>>> Different host buses may have different layouts for config space 
>>>>>>> >>>>>> accessors.
>>>>>>> >>>>>>
>>>>>>> >>>>>> The Mac U3 for example uses the following define to access Type 
>>>>>>> >>>>>> 0 (directly
>>>>>>> >>>>>> attached) devices:
>>>>>>> >>>>>>
>>>>>>> >>>>>> #define MACRISC_CFA0(devfn, off)        \
>>>>>>> >>>>>>      ((1 << (unsigned int)PCI_SLOT(dev_fn)) \
>>>>>>> >>>>>>      | (((unsigned int)PCI_FUNC(dev_fn)) << 8) \
>>>>>>> >>>>>>      | (((unsigned int)(off)) & 0xFCUL))
>>>>>>> >>>>>>
>>>>>>> >>>>>> Obviously, this is different from the encoding we interpret in 
>>>>>>> >>>>>> qemu. In
>>>>>>> >>>>>> order to let host controllers take some influence there, we can 
>>>>>>> >>>>>> just
>>>>>>> >>>>>> introduce a converter function that takes whatever accessor we 
>>>>>>> >>>>>> have and
>>>>>>> >>>>>> makes a qemu understandable one out of it.
>>>>>>> >>>>>>
>>>>>>> >>>>>> This patch is the groundwork for Uninorth PCI config space fixes.
>>>>>>> >>>>>>
>>>>>>> >>>>>> Signed-off-by: Alexander Graf <address@hidden>
>>>>>>> >>>>>> CC: Michael S. Tsirkin <address@hidden>
>>>>>>> >>>>>
>>>>>>> >>>>> Thanks!
>>>>>>> >>>>>
>>>>>>> >>>>> It always bothered me that the code in pci_host uses x86 encoding 
>>>>>>> >>>>> and
>>>>>>> >>>>> other architectures are converted to x86.  As long as we are 
>>>>>>> >>>>> adding
>>>>>>> >>>>> redirection, maybe we should get rid of this, and make 
>>>>>>> >>>>> get_config_reg
>>>>>>> >>>>> return register and devfn separately, so we don't need to 
>>>>>>> >>>>> encode/decode
>>>>>>> >>>>> back and forth?
>>>>>>> >>>>
>>>>>>> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usually 
>>>>>>> >>>> try to build on stuff that is there already. That's exactly what 
>>>>>>> >>>> happened here.
>>>>>>> >>>>
>>>>>>> >>>> I'm personally not against defining the x86 format as qemu 
>>>>>>> >>>> default. That way everyone knows what to deal with. I'm not sure 
>>>>>>> >>>> if PCI defines anything that couldn't be represented by the x86 
>>>>>>> >>>> encoding in 32 bits. I actually doubt it. So it seems like a 
>>>>>>> >>>> pretty good fit, especially given that all the other code is 
>>>>>>> >>>> already in place.
>>>>>>> >>>>
>>>>>>> >>>>> OTOH if we are after a quick fix, won't it be simpler to have
>>>>>>> >>>>> unin_pci simply use its own routines instead of 
>>>>>>> >>>>> pci_host_conf_register_mmio
>>>>>>> >>>>> etc?
>>>>>>> >>>>
>>>>>>> >>>> Hm, I figured this is less work :-).
>>>>>>> >>>
>>>>>>> >>> Let me explain the idea: we have:
>>>>>>> >>>
>>>>>>> >>>   static void pci_host_config_writel_ioport(void *opaque,
>>>>>>> >>>                                             uint32_t addr, uint32_t 
>>>>>>> >>> val)
>>>>>>> >>>   {
>>>>>>> >>>       PCIHostState *s = opaque;
>>>>>>> >>>
>>>>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, 
>>>>>>> >>> addr,
>>>>>>> >>>   val);
>>>>>>> >>>       s->config_reg = val;
>>>>>>> >>>   }
>>>>>>> >>>
>>>>>>> >>>   static uint32_t pci_host_config_readl_ioport(void *opaque, 
>>>>>>> >>> uint32_t
>>>>>>> >>>   addr)
>>>>>>> >>>   {
>>>>>>> >>>       PCIHostState *s = opaque;
>>>>>>> >>>       uint32_t val = s->config_reg;
>>>>>>> >>>
>>>>>>> >>>       PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, 
>>>>>>> >>> addr,
>>>>>>> >>>   val);
>>>>>>> >>>       return val;
>>>>>>> >>>   }
>>>>>>> >>>
>>>>>>> >>>   void pci_host_conf_register_ioport(pio_addr_t ioport, 
>>>>>>> >>> PCIHostState *s)
>>>>>>> >>>   {
>>>>>>> >>>       register_ioport_write(ioport, 4, 4, 
>>>>>>> >>> pci_host_config_writel_ioport,
>>>>>>> >>>   s);
>>>>>>> >>>       register_ioport_read(ioport, 4, 4, 
>>>>>>> >>> pci_host_config_readl_ioport, s);
>>>>>>> >>>   }
>>>>>>> >>>
>>>>>>> >>> If instead of a simple config_reg = val we translate to pc format
>>>>>>> >>> here, the rest will work. No?
>>>>>>> >>
>>>>>>> >> Well, that'd mean I'd have to implement a config_reg read function 
>>>>>>> >> and do the conversion backwards again there. Or I could just save it 
>>>>>>> >> off in the unin state ... hmm ...
>>>>>>> >
>>>>>>> > Right.
>>>>>>> >
>>>>>>> >> Either way, let's better get this done right. I'd rather want to 
>>>>>>> >> have a proper framework in place in case someone else stumbles 
>>>>>>> >> across something similar.
>>>>>>> >>
>>>>>>> >> Alex
>>>>>>> >
>>>>>>> > There's already ugliness with swap/noswap versions in there.  Anything
>>>>>>> > that claims to be a proper framework would need to address that as 
>>>>>>> > well
>>>>>>> > IMO.
>>>>>>>
>>>>>>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>>>>>>> incrementally improving the existing code?
>>>>>>
>>>>>> Not really, incremental improvements are good.  But what you posted does
>>>>>> not seem to clean up most code, what it really does is fixes ppc
>>>>>> unin_pci.  My feeling was since there's only one user for now maybe it
>>>>>> is better to just have device specific code rather than complicate
>>>>>> generic code. Makes sense?
>>>>>>
>>>>>> We need to see several users before we add yet another level of
>>>>>> indirection.  If there is a level of indirection that solves the
>>>>>> swap/noswap ugliness as well that would show this is a good abstraction.
>>>>>> I think I even know what a good abstraction would be: decode address on
>>>>>> write and keep it in decoded form.
>>>>>
>>>>> I think Sparc64 PBM configuration cycles are also a bit different.
>>>>> It's described in UltraSPARC User's Manual 805-0087, p.325.
>>>>>
>>>>> Currently both QEMU and OpenBIOS just use something common, but as
>>>>> soon as Linux boot gets so far as to probe PCI bus, we'd have to fix
>>>>> that.
>>>>
>>>> That time is very close, with latest QEMU and OpenBIOS, PCI probe
>>>> starts (with GDB tricks for calibrate_delay, which won't be needed
>>>> after %tick_cmpr is fixed):
>>>
>>> Now I get this:
>>> [sparc64] Kernel already loaded
>>>
>>> PROMLIB: Sun IEEE Boot Prom 'OBP 3.10.24 1999/01/01 01:01'
>>> PROMLIB: Root node compatible: sun4u
>>> Linux version 2.6.32 (address@hidden) (gcc version 4.2.4) #3 Sat Jan 9
>>> 20:36:42 UTC 2010
>>> bootconsole [earlyprom0] enabled
>>> ARCH: SUN4U
>>> Ethernet address: 52:54:00:12:34:56
>>> Kernel: Using 1 locked TLB entries for main kernel image.
>>> Remapping the kernel... done.
>>> OF stdout device is: 
>>> /address@hidden,0/address@hidden/address@hidden,1/address@hidden/address@hidden
>>> PROM: Built device tree with 32176 bytes of memory.
>>> Top of RAM: 0x7e80000, Total RAM: 0x7e80000
>>> Memory hole size: 0MB
>>> Zone PFN ranges:
>>>  Normal   0x00000000 -> 0x00003f40
>>> Movable zone start PFN for each node
>>> early_node_map[1] active PFN ranges
>>>    0: 0x00000000 -> 0x00003f40
>>> On node 0 totalpages: 16192
>>>  Normal zone: 127 pages used for memmap
>>>  Normal zone: 0 pages reserved
>>>  Normal zone: 16065 pages, LIFO batch:3
>>> Booting Linux...
>>> Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 16065
>>> Kernel command line: root=/dev/hda1 console=ttyS0 -p debug lpj=100000
>>> PID hash table entries: 512 (order: -1, 4096 bytes)
>>> Dentry cache hash table entries: 16384 (order: 4, 131072 bytes)
>>> Inode-cache hash table entries: 8192 (order: 3, 65536 bytes)
>>> Memory: 118480k available (1472k kernel code, 488k data, 104k init)
>>> [fffff80000000000,0000000007e80000]
>>> SLUB: Genslabs=14, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
>>> Hierarchical RCU implementation.
>>> NR_IRQS:255
>>> clocksource: mult[a0000] shift[16]
>>> clockevent: mult[19999999] shift[32]
>>> Console: colour dummy device 80x25
>>> Calibrating delay loop (skipped) preset value.. 50.00 BogoMIPS (lpj=100000)
>>> Mount-cache hash table entries: 512
>>> /pci: PCI IO[1fe02000000] MEM[1ff00000000]
>>> /pci: SABRE PCI Bus Module ver[0:0]
>>> PCI: Scanning PBM /pci
>>> ------------[ cut here ]------------
>>> WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
>>> sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
>>> Call Trace:
>>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>>  [00000000004e92ec] sysfs_add_one+0x8c/0xc0
>>>  [00000000004ea480] sysfs_do_create_link+0x80/0x140
>>>  [000000000053fc88] device_add+0x3c8/0x500
>>>  [0000000000513f34] pci_bus_add_child+0x14/0x80
>>>  [0000000000514144] pci_bus_add_devices+0x144/0x200
>>>  [00000000005140ec] pci_bus_add_devices+0xec/0x200
>>>  [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
>>>  [000000000056d6f0] sabre_probe+0x2d0/0x5a0
>>>  [0000000000569d9c] of_platform_device_probe+0x3c/0x80
>>>  [00000000005427b0] driver_probe_device+0x70/0x160
>>>  [0000000000542918] __driver_attach+0x78/0xa0
>>>  [0000000000541bcc] bus_for_each_dev+0x4c/0x80
>>>  [00000000005421dc] bus_add_driver+0x9c/0x240
>>>  [0000000000542c10] driver_register+0x50/0x160
>>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>> ---[ end trace 139ce121c98e96c9 ]---
>>> pci 0000:00:01.1: Error adding bus, continuing
>>> ------------[ cut here ]------------
>>> WARNING: at /src/linux.git/fs/sysfs/dir.c:491 sysfs_add_one+0x8c/0xc0()
>>> sysfs: cannot create duplicate filename '/class/pci_bus/0000:00'
>>> Call Trace:
>>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>>  [00000000004e92ec] sysfs_add_one+0x8c/0xc0
>>>  [00000000004ea480] sysfs_do_create_link+0x80/0x140
>>>  [000000000053fc88] device_add+0x3c8/0x500
>>>  [0000000000513f34] pci_bus_add_child+0x14/0x80
>>>  [0000000000514144] pci_bus_add_devices+0x144/0x200
>>>  [000000000056ceac] pci_scan_one_pbm+0x6c/0xa0
>>>  [000000000056d6f0] sabre_probe+0x2d0/0x5a0
>>>  [0000000000569d9c] of_platform_device_probe+0x3c/0x80
>>>  [00000000005427b0] driver_probe_device+0x70/0x160
>>>  [0000000000542918] __driver_attach+0x78/0xa0
>>>  [0000000000541bcc] bus_for_each_dev+0x4c/0x80
>>>  [00000000005421dc] bus_add_driver+0x9c/0x240
>>>  [0000000000542c10] driver_register+0x50/0x160
>>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>>  [00000000005f4274] kernel_init+0xd4/0x140
>>> ---[ end trace 139ce121c98e96ca ]---
>>> pci 0000:00:01.0: Error adding bus, continuing
>>> bio: create slab <bio-0> at 0
>>> vgaarb: loaded
>>> Switching to clocksource tick
>>> io scheduler noop registered (default)
>>> ------------[ cut here ]------------
>>> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
>>> proc_dir_entry 'pci/0000:00' already registered
>>> Call Trace:
>>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>>  [00000000004e2948] proc_register+0xe8/0x1c0
>>>  [00000000004e2c30] proc_mkdir_mode+0x30/0x60
>>>  [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
>>>  [00000000006027fc] pci_proc_init+0x5c/0xa0
>>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>>  [00000000005f4274] kernel_init+0xd4/0x140
>>>  [000000000042b130] kernel_thread+0x30/0x60
>>>  [000000000056a8f8] rest_init+0x18/0x80
>>> ---[ end trace 139ce121c98e96cb ]---
>>> ------------[ cut here ]------------
>>> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1c0()
>>> proc_dir_entry 'pci/0000:00' already registered
>>> Call Trace:
>>>  [0000000000450ea8] warn_slowpath_fmt+0x28/0x40
>>>  [00000000004e2948] proc_register+0xe8/0x1c0
>>>  [00000000004e2c30] proc_mkdir_mode+0x30/0x60
>>>  [000000000051ce14] pci_proc_attach_device+0xb4/0xe0
>>>  [00000000006027fc] pci_proc_init+0x5c/0xa0
>>>  [0000000000426a98] do_one_initcall+0x18/0x160
>>>  [00000000005f4274] kernel_init+0xd4/0x140
>>>  [000000000042b130] kernel_thread+0x30/0x60
>>>  [000000000056a8f8] rest_init+0x18/0x80
>>> ---[ end trace 139ce121c98e96cc ]---
>>> Uniform Multi-Platform E-IDE driver
>>> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
>>> PCI: Enabling device: (0000:00:05.0), cmd 301
>>> cmd64x 0000:00:05.0: unable to enable IDE controller
>>> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x00)
>>> CMD64x_IDE 0000:00:05.0: BAR 0: can't reserve I/O region
>>> [0x1fe02000500-0x1fe02000507]
>>> cmd64x 0000:00:05.0: can't reserve resources
>>> CMD64x_IDE: probe of 0000:00:05.0 failed with error -16
>>> ide-gd driver 1.18
>>> ide-cd driver 5.00
>>> mice: PS/2 mouse device common for all mice
>>> turn off boot console earlyprom0
>>>
>>> So PCI probe seems to work. The errors show some bugs with the APB bridges.
>>
>> You have just committed a bug, writes and reads should list methods in
>> {b,w,l} order.
>> See cpu_register_io_memory_fixed where we do map byte access at index 0 etc..
>> Not shure if it helps solving current issue, but the code should look
>> like the following:
>>
>>  static CPUWriteMemoryFunc * const apb_pci_config_writes[] = {
>> -    &apb_pci_config_writel,
>> -    &apb_pci_config_writew,
>>     &apb_pci_config_writeb,
>> +    &apb_pci_config_writew,
>> +    &apb_pci_config_writel,
>>  };
>>
>>  static CPUReadMemoryFunc * const apb_pci_config_reads[] = {
>> -    &apb_pci_config_readl,
>> -    &apb_pci_config_readw,
>>     &apb_pci_config_readb,
>> +    &apb_pci_config_readw,
>> +    &apb_pci_config_readl,
>>  };
>
> Good catch. I actually did try the correct order, but maybe I edited
> the wrong place and it didn't work then.
>
> With your change, CMD646 revision is shown correctly:
> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x01)
> PCI: Enabling device: (0000:00:05.0), cmd 301
> cmd64x 0000:00:05.0: unable to enable IDE controller
> cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x01)

Maybe there is some other bug with the revision field. Just for fun, I
enabled virtio for Sparc64 but Linux sees the zero revision field as 1
(used as ABI version):

cmd64x 0000:00:05.0: IDE controller (0x1095:0x0646 rev 0x01)
CMD64x_IDE 0000:00:05.0: BAR 0: can't reserve I/O region
[0x1fe02000500-0x1fe02000507]
cmd64x 0000:00:05.0: can't reserve resources
CMD64x_IDE: probe of 0000:00:05.0 failed with error -16
ide-gd driver 1.18
ne2k-pci.c:v1.03 9/22/2003 D. Becker/P. Gortmaker
PCI: Enabling device: (0000:00:04.0), cmd 301
eth0: RealTek RTL-8029 found at 0x1fe02000400, IRQ 1, 52:54:00:12:34:56.
pcnet32.c:v1.35 21.Apr.2008 address@hidden
mice: PS/2 mouse device common for all mice
virtio_pci: expected ABI version 0, got 1

However, RTL-8029 gets the MAC address read out correctly.