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

[Alexander Graf]

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