From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NRVyf-0004Qu-Ro for qemu-devel@nongnu.org; Sun, 03 Jan 2010 14:19:01 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NRVyc-0004N0-2Z for qemu-devel@nongnu.org; Sun, 03 Jan 2010 14:19:01 -0500 Received: from [199.232.76.173] (port=42732 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NRVyb-0004Mj-JV for qemu-devel@nongnu.org; Sun, 03 Jan 2010 14:18:57 -0500 Received: from mail-pz0-f188.google.com ([209.85.222.188]:52021) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NRVyb-0003xu-Bx for qemu-devel@nongnu.org; Sun, 03 Jan 2010 14:18:57 -0500 Received: by pzk26 with SMTP id 26so9122949pzk.4 for ; Sun, 03 Jan 2010 11:18:55 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20100103180609.GB8522@redhat.com> 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, 3 Jan 2010 19:18:35 +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 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 qe= mu. In >> >>>>>> order to let host controllers take some influence there, we can j= ust >> >>>>>> introduce a converter function that takes whatever accessor we ha= ve 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/d= ecode >> >>>>> 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 h= ere. >> >>>> >> >>>> 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 any= thing that couldn't be represented by the x86 encoding in 32 bits. I actual= ly 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_regi= ster_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, ui= nt32_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, PCIHost= State *s) >> >>> =C2=A0 { >> >>> =C2=A0 =C2=A0 =C2=A0 register_ioport_write(ioport, 4, 4, pci_host_co= nfig_writel_ioport, >> >>> =C2=A0 s); >> >>> =C2=A0 =C2=A0 =C2=A0 register_ioport_read(ioport, 4, 4, pci_host_con= fig_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 an= d do the conversion backwards again there. Or I could just save it off in t= he 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. =C2=A0Any= thing >> > that claims to be a proper framework would need to address that as wel= l >> > 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.