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

["Michael S. Tsirkin" <mst@redhat.com>]

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 <agraf@suse.de>
> >> ---
> >> 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