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

[Blue Swirl <blauwirbel@gmail.com>]

On Sun, Jan 10, 2010 at 6:41 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <mst@redhat.com> 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 <agraf@suse.de>
>>>> >>>>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>>> >>>>>
>>>> >>>>> 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 (test@host) (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: /pci@1fe,0/pci@1/pci@1,1/ebus@3/su@1fe
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.