[Qemu-devel] Re: [PATCH 2/6] Add config space conversion function for uni_north

[Michael S. Tsirkin]

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 <address@hidden>
> >>>>>> ---
> >>>>>> 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.