From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NRUDm-0001bB-4D for qemu-devel@nongnu.org; Sun, 03 Jan 2010 12:26:30 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NRUDh-0001XN-LW for qemu-devel@nongnu.org; Sun, 03 Jan 2010 12:26:29 -0500 Received: from [199.232.76.173] (port=32909 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NRUDh-0001XJ-Fn for qemu-devel@nongnu.org; Sun, 03 Jan 2010 12:26:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:27523) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NRUDg-000736-WB for qemu-devel@nongnu.org; Sun, 03 Jan 2010 12:26:25 -0500 Date: Sun, 3 Jan 2010 19:23:19 +0200 From: "Michael S. Tsirkin" Message-ID: <20100103172319.GA8508@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> <20100103154726.GC8137@redhat.com> <3E418050-4BEB-4583-86B6-C47F2D1B8649@suse.de> <20100103162028.GE8137@redhat.com> <8B5CDDDB-2BE0-4C38-A594-214E12448786@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8B5CDDDB-2BE0-4C38-A594-214E12448786@suse.de> 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 05:35:01PM +0100, Alexander Graf wrote: > > On 03.01.2010, at 17:20, Michael S. Tsirkin wrote: > > > On Sun, Jan 03, 2010 at 05:13:12PM +0100, Alexander Graf wrote: > >> > >> On 03.01.2010, at 16:47, Michael S. Tsirkin wrote: > >> > >>> 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. > >> > >> We're doing the reverse here. We get a uint32_t (host->config_reg) and need to do something with it. > >> > >> We could either call a helper that splits it into bus,dev,fn or we could just put all of them into a single uint32_t again that later on gets interpreted in a specified format. > >> > >> I figured I'd have to touch less code and keep things more stable for the other (non-uninorth) buses if I keep the x86 format as "default format" and just convert to it. Passing a uint32_t is also easier than passing 3 ints :-). > >> > >> Alex > > > > So what the comment above suggests, is adding helper routine > > that gets register device, function and creates 32 bit value > > in "default format". > > Oh, so you mean that instead of returning a uint32_t that magically gets converted inside the conversion function, we'd create another function like this: > > uint32_t pci_host_config_address(int bus, int dev, int fn) > { > return (1u << 31) | (bus << 11) | (dev << 3) | fn; > } > > which then would be called at the end of the conversion function? > > > Alex Yes.