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

[Alexander Graf]

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 :-).

> 
>> ---
>> 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