On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote:
>
> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote:
>
> > On Sun, Jan 03, 2010 at 02:50:45AM +0100, Alexander Graf wrote:
> >> Different host buses may have different layouts for config space accessors.
> >>
> >> The Mac U3 for example uses the following define to access Type 0 (directly
> >> attached) devices:
> >>
> >> #define MACRISC_CFA0(devfn, off) \
> >> ((1 << (unsigned int)PCI_SLOT(dev_fn)) \
> >> | (((unsigned int)PCI_FUNC(dev_fn)) << 8) \
> >> | (((unsigned int)(off)) & 0xFCUL))
> >>
> >> Obviously, this is different from the encoding we interpret in qemu. In
> >> order to let host controllers take some influence there, we can just
> >> introduce a converter function that takes whatever accessor we have and
> >> makes a qemu understandable one out of it.
> >>
> >> This patch is the groundwork for Uninorth PCI config space fixes.
> >>
> >> Signed-off-by: Alexander Graf <ag...@suse.de>
> >> CC: Michael S. Tsirkin <m...@redhat.com>
> >
> > Thanks!
> >
> > It always bothered me that the code in pci_host uses x86 encoding and
> > other architectures are converted to x86. As long as we are adding
> > redirection, maybe we should get rid of this, and make get_config_reg
> > return register and devfn separately, so we don't need to encode/decode
> > back and forth?
>
> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to
> build on stuff that is there already. That's exactly what happened here.
>
> I'm personally not against defining the x86 format as qemu default. That way
> everyone knows what to deal with. I'm not sure if PCI defines anything that
> couldn't be represented by the x86 encoding in 32 bits. I actually doubt it.
> So it seems like a pretty good fit, especially given that all the other code
> is already in place.
>
> > OTOH if we are after a quick fix, won't it be simpler to have
> > unin_pci simply use its own routines instead of pci_host_conf_register_mmio
> > etc?
>
> Hm, I figured this is less work :-).
Let me explain the idea: we have:
static void pci_host_config_writel_ioport(void *opaque,
uint32_t addr, uint32_t val)
{
PCIHostState *s = opaque;
PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr,
val);
s->config_reg = val;
}
static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t
addr)
{
PCIHostState *s = opaque;
uint32_t val = s->config_reg;
PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr,
val);
return val;
}
void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
{
register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport,
s);
register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
}
If instead of a simple config_reg = val we translate to pc format
here, the rest will work. No?
> >
> >> ---
> >> hw/apb_pci.c | 1 +
> >> hw/grackle_pci.c | 1 +
> >> hw/gt64xxx.c | 1 +
> >> hw/pci_host.c | 11 +++++++++++
> >> hw/pci_host.h | 5 +++++
> >> hw/pci_host_template.h | 30 ++++++++++++++++++------------
> >> hw/piix_pci.c | 1 +
> >> hw/ppc4xx_pci.c | 1 +
> >> hw/ppce500_pci.c | 1 +
> >> hw/unin_pci.c | 1 +
> >> 10 files changed, 41 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> >> index f05308b..fedb84c 100644
> >> --- a/hw/apb_pci.c
> >> +++ b/hw/apb_pci.c
> >> @@ -222,6 +222,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
> >> /* mem_data */
> >> sysbus_mmio_map(s, 3, mem_base);
> >> d = FROM_SYSBUS(APBState, s);
> >> + pci_host_init(&d->host_state);
> >> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> >> pci_apb_set_irq, pci_pbm_map_irq,
> >> pic,
> >> 0, 32);
> >> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> >> index ee4fed5..7feba63 100644
> >> --- a/hw/grackle_pci.c
> >> +++ b/hw/grackle_pci.c
> >> @@ -88,6 +88,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic)
> >> qdev_init_nofail(dev);
> >> s = sysbus_from_qdev(dev);
> >> d = FROM_SYSBUS(GrackleState, s);
> >> + pci_host_init(&d->host_state);
> >> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> >> pci_grackle_set_irq,
> >> pci_grackle_map_irq,
> >> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> >> index fb7f5bd..8cf93ca 100644
> >> --- a/hw/gt64xxx.c
> >> +++ b/hw/gt64xxx.c
> >> @@ -1120,6 +1120,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
> >> s = qemu_mallocz(sizeof(GT64120State));
> >> s->pci = qemu_mallocz(sizeof(GT64120PCIState));
> >>
> >> + pci_host_init(s->pci);
> >> s->pci->bus = pci_register_bus(NULL, "pci",
> >> pci_gt64120_set_irq,
> >> pci_gt64120_map_irq,
> >> pic, 144, 4);
> >> diff --git a/hw/pci_host.c b/hw/pci_host.c
> >> index eeb8dee..2d12887 100644
> >> --- a/hw/pci_host.c
> >> +++ b/hw/pci_host.c
> >> @@ -228,3 +228,14 @@ void pci_host_data_register_ioport(pio_addr_t ioport,
> >> PCIHostState *s)
> >> register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s);
> >> register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s);
> >> }
> >> +
> >> +/* This implements the default x86 way of interpreting the config
> >> register */
> >> +static uint32_t pci_host_get_config_reg(PCIHostState *s, uint32_t addr)
> >> +{
> >> + return s->config_reg | (addr & 3);
> >> +}
> >> +
> >> +void pci_host_init(PCIHostState *s)
> >> +{
> >> + s->get_config_reg = pci_host_get_config_reg;
> >> +}
> >
> > So config_reg is always set but only used for PC now?
> > This is not very pretty ...
>
> config_reg is used by the template.h helpers. So everyone should use it.
> get_config_reg is always set. It's set to pci_host_get_config_reg as default
> and can be overridden by a host bus if it knows it's using a different layout.
>
>
> Alex
