Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller

[Alexander Graf]

On 03.10.2012, at 13:50, Bharat Bhushan wrote:

> PCI Root complex have TYPE-1 configuration header while PCI endpoint
> have type-0 configuration header. The type-1 configuration header have
> a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci
> address space to CCSR address space. This can used for 2 purposes: 1)
> for MSI interrupt generation 2) Allow CCSR registers access when configured
> as PCI endpoint, which I am not sure is a use case with QEMU-KVM guest.
> 
> What I observed is that when guest read the size of BAR0 of host controller
> configuration header (TYPE1 header) then it always reads it as 0. When
> looking into the QEMU hw/ppce500_pci.c, I do not find the PCI controller
> device registering BAR0. I do not find any other controller also doing so
> may they do not use BAR0.
> 
> There are two issues when BAR0 is not there (which I can think of):
> 1) There should be BAR0 emulated for PCI Root comaplex (TYPE1 header) and
> when reading the size of BAR0, it should give size as per real h/w.
> 
> This patch solves this problem.
> 
> 2) Do we need this BAR0 inbound address translation?
>        When BAR0 is of non-zero size then it will be configured for PCI
> address space to local address(CCSR) space translation on inbound access.
> The primary use case is for MSI interrupt generation. The device is
> configured with a address offsets in PCI address space, which will be
> translated to MSI interrupt generation MPIC registers. Currently I do
> not understand the MSI interrupt generation mechanism in QEMU and also
> IIRC we do not use QEMU MSI interrupt mechanism on e500 guest machines.
> But this BAR0 will be used when using MSI on e500.
> 
> I can see one more issue, There are ATMUs emulated in hw/ppce500_pci.c,
> but i do not see these being used for address translation.
> So far that works because pci address space and local address space are 1:1
> mapped. BAR0 inbound translation + ATMU translation will complete the address
> translation of inbound traffic.
> 
> Signed-off-by: Bharat Bhushan <address@hidden>
> ---
> hw/ppc/e500.c    |    1 +
> hw/ppce500_pci.c |   13 +++++++++++++
> 2 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 197411d..c7ae2b6 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params)
> 
>     /* PCI */
>     dev = qdev_create(NULL, "e500-pcihost");
> +    qdev_prop_set_ptr(dev, "bar0_region", ccsr);
>     qdev_init_nofail(dev);
>     s = sysbus_from_qdev(dev);
>     sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]);
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..16e4af2 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -87,6 +87,7 @@ struct PPCE500PCIState {
>     /* mmio maps */
>     MemoryRegion container;
>     MemoryRegion iomem;
> +    void *bar0;
> };
> 
> typedef struct PPCE500PCIState PPCE500PCIState;
> @@ -315,6 +316,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     int i;
>     MemoryRegion *address_space_mem = get_system_memory();
>     MemoryRegion *address_space_io = get_system_io();
> +    PCIDevice *pdev;
> +    MemoryRegion bar0;
> 
>     h = PCI_HOST_BRIDGE(dev);
>     s = PPC_E500_PCI_HOST_BRIDGE(dev);
> @@ -342,6 +345,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
>     sysbus_init_mmio(dev, &s->container);
> 
> +    bar0 = *(MemoryRegion *)s->bar0;
> +    pdev = pci_find_device(b, 0, 0);
> +    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0);

Any reason you can't just pass in s->bar0 here? Though I'm not sure whether we 
have to do something special to get a memory region alias.

Avi, any ideas? We want the same memory region mapped twice. Once at a fixed 
address, once as BAR0 of the PCI host controller.


Alex

> +
>     return 0;
> }
> 
> @@ -363,6 +370,11 @@ static const TypeInfo e500_host_bridge_info = {
>     .class_init    = e500_host_bridge_class_init,
> };
> 
> +static Property pci_host_dev_info[] = {
> +    DEFINE_PROP_PTR("bar0_region", PPCE500PCIState, bar0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void e500_pcihost_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -370,6 +382,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, 
> void *data)
> 
>     k->init = e500_pcihost_initfn;
>     dc->vmsd = &vmstate_ppce500_pci;
> +    dc->props = pci_host_dev_info;
> }
> 
> static const TypeInfo e500_pcihost_info = {
> -- 
> 1.7.0.4
> 
>