From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NU2jd-0000P7-TR for qemu-devel@nongnu.org; Sun, 10 Jan 2010 13:41:57 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NU2ja-0000M3-2r for qemu-devel@nongnu.org; Sun, 10 Jan 2010 13:41:57 -0500 Received: from [199.232.76.173] (port=48494 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NU2jZ-0000Ly-Nd for qemu-devel@nongnu.org; Sun, 10 Jan 2010 13:41:53 -0500 Received: from mx20.gnu.org ([199.232.41.8]:17331) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1NU2jZ-0007A3-7J for qemu-devel@nongnu.org; Sun, 10 Jan 2010 13:41:53 -0500 Received: from mail-pz0-f188.google.com ([209.85.222.188]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NU2jX-0001W6-WF for qemu-devel@nongnu.org; Sun, 10 Jan 2010 13:41:52 -0500 Received: by pzk26 with SMTP id 26so11778636pzk.4 for ; Sun, 10 Jan 2010 10:41:48 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1262483450-15206-1-git-send-email-agraf@suse.de> <1262483450-15206-2-git-send-email-agraf@suse.de> <20100103154539.GB8137@redhat.com> <75078DD9-314F-4FA8-896B-36C5B771C6A7@suse.de> <20100103172937.GB8508@redhat.com> <20100103174430.GA8522@redhat.com> <41679128-EE37-47DA-82F6-830A4C364183@suse.de> <20100103180609.GB8522@redhat.com> From: Blue Swirl Date: Sun, 10 Jan 2010 18:41:28 +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: "Michael S. Tsirkin" Cc: Alexander Graf , Aurelien Jarno , QEMU Developers On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl wrote: > On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin 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) =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 q= emu. In >>> >>>>>> order to let host controllers take some influence there, we can = just >>> >>>>>> introduce a converter function that takes whatever accessor we h= ave 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 >>> >>>>>> CC: Michael S. Tsirkin >>> >>>>> >>> >>>>> Thanks! >>> >>>>> >>> >>>>> It always bothered me that the code in pci_host uses x86 encoding= 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_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 an= ything that couldn't be represented by the x86 encoding in 32 bits. I actua= lly 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_reg= ister_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 %"PRIx32"\= 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 *opaque, u= int32_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 %"PRIx32"\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, PCIHos= tState *s) >>> >>> =C2=A0 { >>> >>> =C2=A0 =C2=A0 =C2=A0 register_ioport_write(ioport, 4, 4, pci_host_c= onfig_writel_ioport, >>> >>> =C2=A0 s); >>> >>> =C2=A0 =C2=A0 =C2=A0 register_ioport_read(ioport, 4, 4, pci_host_co= nfig_readl_ioport, s); >>> >>> =C2=A0 } >>> >>> >>> >>> If instead of a simple config_reg =3D 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 a= nd 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 hav= e a proper framework in place in case someone else stumbles across somethin= g similar. >>> >> >>> >> Alex >>> > >>> > There's already ugliness with swap/noswap versions in there. =C2=A0An= ything >>> > that claims to be a proper framework would need to address that as we= ll >>> > 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. =C2=A0But what you posted= 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 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. =C2=A0If 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): [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 32165 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=3D/dev/hdc1 console=3DttyS0 -p debug 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=3D1, N= odes=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.. 5000.00 BogoMIPS (lpj=3D100= 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 There it hangs because the probed address is bad: (gdb) 0x000000000043a1a8 75 __asm__ __volatile__("membar #Sync\= n\t" 1: x/i $pc 0x43a1a8 : lduha [ %o0 ] (29), %o4 (gdb) p $o0 $12 =3D 0x1fe01000806