[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Michael S. Tsirkin]

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 <address@hidden>
> >> CC: Michael S. Tsirkin <address@hidden>
> > 
> > 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