From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NWxa8-0004l2-KE for qemu-devel@nongnu.org; Mon, 18 Jan 2010 14:48:12 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NWxa4-0004jr-Lv for qemu-devel@nongnu.org; Mon, 18 Jan 2010 14:48:12 -0500 Received: from [199.232.76.173] (port=36192 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NWxa4-0004jl-Hn for qemu-devel@nongnu.org; Mon, 18 Jan 2010 14:48:08 -0500 Received: from mail-pw0-f43.google.com ([209.85.160.43]:41745) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NWxa3-0007MK-72 for qemu-devel@nongnu.org; Mon, 18 Jan 2010 14:48:07 -0500 Received: by pwj11 with SMTP id 11so2137735pwj.2 for ; Mon, 18 Jan 2010 11:48:06 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1262483450-15206-1-git-send-email-agraf@suse.de> <20100103174430.GA8522@redhat.com> <41679128-EE37-47DA-82F6-830A4C364183@suse.de> <20100103180609.GB8522@redhat.com> From: Blue Swirl Date: Mon, 18 Jan 2010 19:47:46 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Kovalenko Cc: QEMU Developers , Alexander Graf , Aurelien Jarno , "Michael S. Tsirkin" On Tue, Jan 12, 2010 at 7:29 PM, Blue Swirl wrote: > On Mon, Jan 11, 2010 at 10:33 PM, Igor Kovalenko > wrote: >> On Tue, Jan 12, 2010 at 12:29 AM, Blue Swirl wrot= e: >>> On Sun, Jan 10, 2010 at 6:41 PM, Blue Swirl wrot= e: >>>> On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl wrot= e: >>>>> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin w= rote: >>>>>> 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 wrot= e: >>>>>>> >>>>>> Different host buses may have different layouts for config s= pace accessors. >>>>>>> >>>>>> >>>>>>> >>>>>> The Mac U3 for example uses the following define to access T= ype 0 (directly >>>>>>> >>>>>> attached) devices: >>>>>>> >>>>>> >>>>>>> >>>>>> #define MACRISC_CFA0(devfn, off) =C2=A0 =C2=A0 =C2=A0 =C2=A0= \ >>>>>>> >>>>>> =C2=A0 =C2=A0 =C2=A0((1 << (unsigned int)PCI_SLOT(dev_fn)) \ >>>>>>> >>>>>> =C2=A0 =C2=A0 =C2=A0| (((unsigned int)PCI_FUNC(dev_fn)) << 8= ) \ >>>>>>> >>>>>> =C2=A0 =C2=A0 =C2=A0| (((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 f= ixes. >>>>>>> >>>>>> >>>>>>> >>>>>> Signed-off-by: Alexander Graf >>>>>>> >>>>>> CC: Michael S. Tsirkin >>>>>>> >>>>> >>>>>>> >>>>> Thanks! >>>>>>> >>>>> >>>>>>> >>>>> It always bothered me that the code in pci_host uses x86 enco= ding and >>>>>>> >>>>> other architectures are converted to x86. =C2=A0As long as we= are adding >>>>>>> >>>>> redirection, maybe we should get rid of this, and make get_co= nfig_reg >>>>>>> >>>>> return register and devfn separately, so we don't need to enc= ode/decode >>>>>>> >>>>> back and forth? >>>>>>> >>>> >>>>>>> >>>> Hmm, when touching code I'm not 100% sure what I'm doing I usu= ally try to build on stuff that is there already. That's exactly what happe= ned here. >>>>>>> >>>> >>>>>>> >>>> I'm personally not against defining the x86 format as qemu def= ault. That way everyone knows what to deal with. I'm not sure if PCI define= s anything that couldn't be represented by the x86 encoding in 32 bits. I a= ctually 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: >>>>>>> >>> >>>>>>> >>> =C2=A0 static void pci_host_config_writel_ioport(void *opaque, >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 uint32_t addr, uint32_t val) >>>>>>> >>> =C2=A0 { >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 PCIHostState *s =3D opaque; >>>>>>> >>> >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx= 32"\n", __func__, addr, >>>>>>> >>> =C2=A0 val); >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 s->config_reg =3D val; >>>>>>> >>> =C2=A0 } >>>>>>> >>> >>>>>>> >>> =C2=A0 static uint32_t pci_host_config_readl_ioport(void *opaqu= e, uint32_t >>>>>>> >>> =C2=A0 addr) >>>>>>> >>> =C2=A0 { >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 PCIHostState *s =3D opaque; >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 uint32_t val =3D s->config_reg; >>>>>>> >>> >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx3= 2"\n", __func__, addr, >>>>>>> >>> =C2=A0 val); >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 return val; >>>>>>> >>> =C2=A0 } >>>>>>> >>> >>>>>>> >>> =C2=A0 void pci_host_conf_register_ioport(pio_addr_t ioport, PC= IHostState *s) >>>>>>> >>> =C2=A0 { >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 register_ioport_write(ioport, 4, 4, pci_ho= st_config_writel_ioport, >>>>>>> >>> =C2=A0 s); >>>>>>> >>> =C2=A0 =C2=A0 =C2=A0 register_ioport_read(ioport, 4, 4, pci_hos= t_config_readl_ioport, s); >>>>>>> >>> =C2=A0 } >>>>>>> >>> >>>>>>> >>> If instead of a simple config_reg =3D val we translate to pc fo= rmat >>>>>>> >>> here, the rest will work. No? >>>>>>> >> >>>>>>> >> Well, that'd mean I'd have to implement a config_reg read functi= on 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 some= thing similar. >>>>>>> >> >>>>>>> >> Alex >>>>>>> > >>>>>>> > There's already ugliness with swap/noswap versions in there. =C2= =A0Anything >>>>>>> > that claims to be a proper framework would need to address that a= s well >>>>>>> > IMO. >>>>>>> >>>>>>> Hm, so you'd rather wait for 5 years to have an all-in-one rework t= han >>>>>>> incrementally improving the existing code? >>>>>> >>>>>> Not really, incremental improvements are good. =C2=A0But what you po= sted does >>>>>> not seem to clean up most code, what it really does is fixes ppc >>>>>> unin_pci. =C2=A0My feeling was since there's only one user for now m= aybe 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. =C2=A0If there is a level of indirection that solves th= e >>>>>> swap/noswap ugliness as well that would show this is a good abstract= ion. >>>>>> 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: >>> =C2=A0Normal =C2=A0 0x00000000 -> 0x00003f40 >>> Movable zone start PFN for each node >>> early_node_map[1] active PFN ranges >>> =C2=A0 =C2=A00: 0x00000000 -> 0x00003f40 >>> On node 0 totalpages: 16192 >>> =C2=A0Normal zone: 127 pages used for memmap >>> =C2=A0Normal zone: 0 pages reserved >>> =C2=A0Normal zone: 16065 pages, LIFO batch:3 >>> Booting Linux... >>> Built 1 zonelists in Zone order, mobility grouping on. =C2=A0Total page= s: 16065 >>> Kernel command line: root=3D/dev/hda1 console=3DttyS0 -p debug lpj=3D10= 0000 >>> 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=3D14, HWalign=3D32, Order=3D0-3, MinObjects=3D0, CPUs=3D= 1, Nodes=3D1 >>> 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=3D1= 00000) >>> 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: >>> =C2=A0[0000000000450ea8] warn_slowpath_fmt+0x28/0x40 >>> =C2=A0[00000000004e92ec] sysfs_add_one+0x8c/0xc0 >>> =C2=A0[00000000004ea480] sysfs_do_create_link+0x80/0x140 >>> =C2=A0[000000000053fc88] device_add+0x3c8/0x500 >>> =C2=A0[0000000000513f34] pci_bus_add_child+0x14/0x80 >>> =C2=A0[0000000000514144] pci_bus_add_devices+0x144/0x200 >>> =C2=A0[00000000005140ec] pci_bus_add_devices+0xec/0x200 >>> =C2=A0[000000000056ceac] pci_scan_one_pbm+0x6c/0xa0 >>> =C2=A0[000000000056d6f0] sabre_probe+0x2d0/0x5a0 >>> =C2=A0[0000000000569d9c] of_platform_device_probe+0x3c/0x80 >>> =C2=A0[00000000005427b0] driver_probe_device+0x70/0x160 >>> =C2=A0[0000000000542918] __driver_attach+0x78/0xa0 >>> =C2=A0[0000000000541bcc] bus_for_each_dev+0x4c/0x80 >>> =C2=A0[00000000005421dc] bus_add_driver+0x9c/0x240 >>> =C2=A0[0000000000542c10] driver_register+0x50/0x160 >>> =C2=A0[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: >>> =C2=A0[0000000000450ea8] warn_slowpath_fmt+0x28/0x40 >>> =C2=A0[00000000004e92ec] sysfs_add_one+0x8c/0xc0 >>> =C2=A0[00000000004ea480] sysfs_do_create_link+0x80/0x140 >>> =C2=A0[000000000053fc88] device_add+0x3c8/0x500 >>> =C2=A0[0000000000513f34] pci_bus_add_child+0x14/0x80 >>> =C2=A0[0000000000514144] pci_bus_add_devices+0x144/0x200 >>> =C2=A0[000000000056ceac] pci_scan_one_pbm+0x6c/0xa0 >>> =C2=A0[000000000056d6f0] sabre_probe+0x2d0/0x5a0 >>> =C2=A0[0000000000569d9c] of_platform_device_probe+0x3c/0x80 >>> =C2=A0[00000000005427b0] driver_probe_device+0x70/0x160 >>> =C2=A0[0000000000542918] __driver_attach+0x78/0xa0 >>> =C2=A0[0000000000541bcc] bus_for_each_dev+0x4c/0x80 >>> =C2=A0[00000000005421dc] bus_add_driver+0x9c/0x240 >>> =C2=A0[0000000000542c10] driver_register+0x50/0x160 >>> =C2=A0[0000000000426a98] do_one_initcall+0x18/0x160 >>> =C2=A0[00000000005f4274] kernel_init+0xd4/0x140 >>> ---[ end trace 139ce121c98e96ca ]--- >>> pci 0000:00:01.0: Error adding bus, continuing >>> bio: create slab 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/0x1= c0() >>> proc_dir_entry 'pci/0000:00' already registered >>> Call Trace: >>> =C2=A0[0000000000450ea8] warn_slowpath_fmt+0x28/0x40 >>> =C2=A0[00000000004e2948] proc_register+0xe8/0x1c0 >>> =C2=A0[00000000004e2c30] proc_mkdir_mode+0x30/0x60 >>> =C2=A0[000000000051ce14] pci_proc_attach_device+0xb4/0xe0 >>> =C2=A0[00000000006027fc] pci_proc_init+0x5c/0xa0 >>> =C2=A0[0000000000426a98] do_one_initcall+0x18/0x160 >>> =C2=A0[00000000005f4274] kernel_init+0xd4/0x140 >>> =C2=A0[000000000042b130] kernel_thread+0x30/0x60 >>> =C2=A0[000000000056a8f8] rest_init+0x18/0x80 >>> ---[ end trace 139ce121c98e96cb ]--- >>> ------------[ cut here ]------------ >>> WARNING: at /src/linux.git/fs/proc/generic.c:590 proc_register+0xe8/0x1= c0() >>> proc_dir_entry 'pci/0000:00' already registered >>> Call Trace: >>> =C2=A0[0000000000450ea8] warn_slowpath_fmt+0x28/0x40 >>> =C2=A0[00000000004e2948] proc_register+0xe8/0x1c0 >>> =C2=A0[00000000004e2c30] proc_mkdir_mode+0x30/0x60 >>> =C2=A0[000000000051ce14] pci_proc_attach_device+0xb4/0xe0 >>> =C2=A0[00000000006027fc] pci_proc_init+0x5c/0xa0 >>> =C2=A0[0000000000426a98] do_one_initcall+0x18/0x160 >>> =C2=A0[00000000005f4274] kernel_init+0xd4/0x140 >>> =C2=A0[000000000042b130] kernel_thread+0x30/0x60 >>> =C2=A0[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 brid= ges. >> >> 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: >> >> =C2=A0static CPUWriteMemoryFunc * const apb_pci_config_writes[] =3D { >> - =C2=A0 =C2=A0&apb_pci_config_writel, >> - =C2=A0 =C2=A0&apb_pci_config_writew, >> =C2=A0 =C2=A0 &apb_pci_config_writeb, >> + =C2=A0 =C2=A0&apb_pci_config_writew, >> + =C2=A0 =C2=A0&apb_pci_config_writel, >> =C2=A0}; >> >> =C2=A0static CPUReadMemoryFunc * const apb_pci_config_reads[] =3D { >> - =C2=A0 =C2=A0&apb_pci_config_readl, >> - =C2=A0 =C2=A0&apb_pci_config_readw, >> =C2=A0 =C2=A0 &apb_pci_config_readb, >> + =C2=A0 =C2=A0&apb_pci_config_readw, >> + =C2=A0 =C2=A0&apb_pci_config_readl, >> =C2=A0}; > > 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 tsbogend@alpha.franken.de 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.