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