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

[Alexander Graf]

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