From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NRSj0-0001Ku-Qd for qemu-devel@nongnu.org; Sun, 03 Jan 2010 10:50:38 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NRSiw-0001JM-5d for qemu-devel@nongnu.org; Sun, 03 Jan 2010 10:50:38 -0500 Received: from [199.232.76.173] (port=53936 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NRSiw-0001JJ-2r for qemu-devel@nongnu.org; Sun, 03 Jan 2010 10:50:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:25225) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NRSiv-00088X-HF for qemu-devel@nongnu.org; Sun, 03 Jan 2010 10:50:33 -0500 Date: Sun, 3 Jan 2010 17:47:26 +0200 From: "Michael S. Tsirkin" Message-ID: <20100103154726.GC8137@redhat.com> References: <1262483450-15206-1-git-send-email-agraf@suse.de> <1262483450-15206-3-git-send-email-agraf@suse.de> <20100103153215.GA8137@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Blue Swirl , QEMU Developers , Aurelien Jarno On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote: > > On 03.01.2010, at 16:32, Michael S. Tsirkin wrote: > > > On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote: > >> As stated in the previous patch, the Uninorth PCI bridge requires different > >> layouts in its PCI config space accessors. > >> > >> This patch introduces a conversion function that makes it compatible with > >> the way Linux accesses it. > >> > >> I also kept an OpenBIOS compatibility hack in. I think it'd be better to > >> take small steps here and do the config space access rework in OpenBIOS > >> later on. When that's done we can remove that hack. > >> > >> Signed-off-by: Alexander Graf > >> --- > >> hw/unin_pci.c | 35 +++++++++++++++++++++++++++++++++++ > >> 1 files changed, 35 insertions(+), 0 deletions(-) > >> > >> diff --git a/hw/unin_pci.c b/hw/unin_pci.c > >> index fdb9401..1c49008 100644 > >> --- a/hw/unin_pci.c > >> +++ b/hw/unin_pci.c > >> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque) > >> { > >> } > >> > >> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr) > >> +{ > >> + uint32_t retval; > >> + uint32_t reg = s->config_reg; > >> + > >> + if (reg & (1u << 31)) { > >> + /* XXX OpenBIOS compatibility hack */ > >> + retval = reg; > >> + addr |= reg & 7; > >> + } else if (reg & 1) { > >> + /* Set upper valid bit and remove lower one */ > >> + retval = (reg & ~3u) | (1u << 31); > >> + } else { > >> + uint32_t slot, func; > >> + uint32_t devfn; > >> + > >> + /* Grab CFA0 style values */ > >> + slot = ffs(reg & 0xfffff800) - 1; > >> + func = (reg >> 8) & 7; > >> + devfn = PCI_DEVFN(slot, func); > >> + > >> + /* ... and then convert them to x86 format */ > >> + retval = (reg & 0xfc) | (devfn << 8) | (1u << 31); > > > > Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit > > number? This way this encoding can be changed down the road. > > I don't think I understand this comment? :-) This puts reg+dev+fn in a format that pci_host can the understand correct? So it would make sense to have an inline function in pci host that gets 3 parameters and does the encoding. > > > >> + } > >> + > >> + retval &= ~3u; > >> + retval |= (addr & 7); > >> + > >> + UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n", > >> + reg, addr, retval); > >> + > >> + return retval; > >> +} > >> + > >> static int pci_unin_main_init_device(SysBusDevice *dev) > >> { > >> UNINState *s; > >> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev) > >> s = FROM_SYSBUS(UNINState, dev); > >> > >> pci_host_init(&s->host_state); > >> + s->host_state.get_config_reg = unin_get_config_reg; > > > > This looks slightly ugly: let's make pci_host_init accept > > the conversion function instead? > > Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones. > > > Alex