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

[Michael S. Tsirkin]

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? :-)
> 
> > 
> >> +    }
> >> +
> >> +    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.
> 

OK.

> Alex