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

[Alexander Graf]

On 19.09.2012, at 09:41, 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/pci_host.h    |    9 +++++++++
> hw/ppc/e500.c    |   21 +++++++++++++++++++++
> hw/ppce500_pci.c |    7 +++++++
> 3 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index 4b9c300..c1ec7eb 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -41,10 +41,19 @@ struct PCIHostState {
>     MemoryRegion data_mem;
>     MemoryRegion mmcfg;
>     MemoryRegion *address_space;
> +    MemoryRegion bar0;
>     uint32_t config_reg;
>     PCIBus *bus;
> };
> 
> +typedef struct PPCE500CCSRState {
> +    SysBusDevice *parent;
> +    MemoryRegion ccsr_space;
> +} PPCE500CCSRState;
> +
> +#define TYPE_CCSR "e500-ccsr"
> +#define CCSR(obj) OBJECT_CHECK(PPCE500CCSRState, (obj), TYPE_CCSR)
> +

All of this is e500 specific, so it should definitely not be in any generic pci 
host header.

> /* common internal helpers for PCI/PCIe hosts, cut off overflows */
> void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
>                                   uint32_t limit, uint32_t val, uint32_t len);
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 6f0de6d..1f5da28 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -31,6 +31,7 @@
> #include "hw/loader.h"
> #include "elf.h"
> #include "hw/sysbus.h"
> +#include "hw/pci_host.h"
> #include "exec-memory.h"
> #include "host-utils.h"
> 
> @@ -587,3 +588,23 @@ void ppce500_init(PPCE500Params *params)
>         kvmppc_init();
>     }
> }
> +
> +static void e500_ccsr_type_init(Object *obj)
> +{
> +    PPCE500CCSRState *ccsr = CCSR(obj);
> +    ccsr->ccsr_space.size = int128_make64(MPC8544_CCSRBAR_SIZE);
> +}
> +
> +static const TypeInfo e500_ccsr_info = {
> +    .name          = TYPE_CCSR,
> +    .parent        =  TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PPCE500CCSRState),
> +    .instance_init = e500_ccsr_type_init,
> +};
> +
> +static void e500_ccsr_register_types(void)
> +{
> +    type_register_static(&e500_ccsr_info);
> +}
> +
> +type_init(e500_ccsr_register_types)
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..135f2a6 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -315,6 +315,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     int i;
>     MemoryRegion *address_space_mem = get_system_memory();
>     MemoryRegion *address_space_io = get_system_io();
> +    PPCE500CCSRState *ccsr;
> +    PCIDevice *pdev;
> 
>     h = PCI_HOST_BRIDGE(dev);
>     s = PPC_E500_PCI_HOST_BRIDGE(dev);
> @@ -342,6 +344,11 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>     memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
>     sysbus_init_mmio(dev, &s->container);
> 
> +    ccsr = CCSR(object_new(TYPE_CCSR));
> +    memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h,
> +                          "pci-bar0", memory_region_size(&ccsr->ccsr_space));
> +    pdev = pci_find_device(b, 0, 0);
> +    pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &h->bar0);

No. The PCI host bridge doesn't create the CCSR object. The machine model does. 
And we also need to move all the devices that reside inside the CCSR object at 
least into its memory region, probably even inside a container object called 
"e500-ccsr".

The host bridge then asks that ccsr object for its memory region and maps that 
in as bar0. Since all devices inside the ccsr region are already mapped inside 
the memory region, we get device access to bar0 for free.


Alex