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

[Alexander Graf]

On 10.10.2012, at 16:28, 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 complex (TYPE1 header) and
> when reading the size of BAR0, it should give size as per real h/w.
> 
> 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 an 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>
> ---
> v3:
> - minor cleanup (variable name corrected from pci_ccsr to ccsr and
>   spelling in patch description)
> 
> hw/ppc/e500-ccsr.h |   17 +++++++++++++++
> hw/ppc/e500.c      |   56 +++++++++++++++++++++++++++++++++++++++++++--------
> hw/ppce500_pci.c   |   29 ++++++++++++++++++++++++++-
> 3 files changed, 92 insertions(+), 10 deletions(-)
> create mode 100644 hw/ppc/e500-ccsr.h
> 
> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
> new file mode 100644
> index 0000000..f20f51b
> --- /dev/null
> +++ b/hw/ppc/e500-ccsr.h
> @@ -0,0 +1,17 @@
> +#ifndef E500_CCSR_H
> +#define E500_CCSR_H
> +
> +#include "../sysbus.h"
> +
> +typedef struct PPCE500CCSRState {
> +    /*< private >*/
> +    SysBusDevice parent;
> +    /*< public >*/
> +
> +    MemoryRegion ccsr_space;
> +} PPCE500CCSRState;
> +
> +#define TYPE_CCSR "e500-ccsr"
> +#define CCSR(obj) OBJECT_CHECK(PPCE500CCSRState, (obj), TYPE_CCSR)
> +
> +#endif /* E500_CCSR_H */
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 187def2..89ee2ef 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -17,6 +17,7 @@
> #include "config.h"
> #include "qemu-common.h"
> #include "e500.h"
> +#include "e500-ccsr.h"
> #include "net.h"
> #include "hw/hw.h"
> #include "hw/pc.h"
> @@ -423,8 +424,9 @@ void ppce500_init(PPCE500Params *params)
>     qemu_irq **irqs, *mpic;
>     DeviceState *dev;
>     CPUPPCState *firstenv = NULL;
> -    MemoryRegion *ccsr;
> +    MemoryRegion *ccsr_addr_space;
>     SysBusDevice *s;
> +    PPCE500CCSRState *ccsr;
> 
>     /* Setup CPUs */
>     if (params->cpu_model == NULL) {
> @@ -481,12 +483,18 @@ void ppce500_init(PPCE500Params *params)
>     vmstate_register_ram_global(ram);
>     memory_region_add_subregion(address_space_mem, 0, ram);
> 
> -    ccsr = g_malloc0(sizeof(MemoryRegion));
> -    memory_region_init(ccsr, "e500-ccsr", MPC8544_CCSRBAR_SIZE);
> -    memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, 
> ccsr);
> +    ccsr_addr_space = g_malloc0(sizeof(MemoryRegion));

x = a;

> +    dev = qdev_create(NULL, "e500-ccsr");
> +    object_property_add_child(qdev_get_machine(), "e500-ccsr",
> +                              OBJECT(dev), NULL);
> +    qdev_init_nofail(dev);
> +    ccsr = CCSR(dev);
> +    ccsr_addr_space = &ccsr->ccsr_space;

x = b;

without any use of x in between? I suppose the first one is simply redundant?

The rest looks quite nice from what I can tell.


Alex

> +    memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE,
> +                                ccsr_addr_space);
> 
>     /* MPIC */
> -    mpic = mpic_init(ccsr, MPC8544_MPIC_REGS_OFFSET,
> +    mpic = mpic_init(ccsr_addr_space, MPC8544_MPIC_REGS_OFFSET,
>                      smp_cpus, irqs, NULL);
> 
>     if (!mpic) {
> @@ -495,13 +503,13 @@ void ppce500_init(PPCE500Params *params)
> 
>     /* Serial */
>     if (serial_hds[0]) {
> -        serial_mm_init(ccsr, MPC8544_SERIAL0_REGS_OFFSET,
> +        serial_mm_init(ccsr_addr_space, MPC8544_SERIAL0_REGS_OFFSET,
>                        0, mpic[12+26], 399193,
>                        serial_hds[0], DEVICE_BIG_ENDIAN);
>     }
> 
>     if (serial_hds[1]) {
> -        serial_mm_init(ccsr, MPC8544_SERIAL1_REGS_OFFSET,
> +        serial_mm_init(ccsr_addr_space, MPC8544_SERIAL1_REGS_OFFSET,
>                        0, mpic[12+26], 399193,
>                        serial_hds[1], DEVICE_BIG_ENDIAN);
>     }
> @@ -510,7 +518,7 @@ void ppce500_init(PPCE500Params *params)
>     dev = qdev_create(NULL, "mpc8544-guts");
>     qdev_init_nofail(dev);
>     s = SYS_BUS_DEVICE(dev);
> -    memory_region_add_subregion(ccsr, MPC8544_UTIL_OFFSET,
> +    memory_region_add_subregion(ccsr_addr_space, MPC8544_UTIL_OFFSET,
>                                 sysbus_mmio_get_region(s, 0));
> 
>     /* PCI */
> @@ -521,7 +529,7 @@ void ppce500_init(PPCE500Params *params)
>     sysbus_connect_irq(s, 1, mpic[pci_irq_nrs[1]]);
>     sysbus_connect_irq(s, 2, mpic[pci_irq_nrs[2]]);
>     sysbus_connect_irq(s, 3, mpic[pci_irq_nrs[3]]);
> -    memory_region_add_subregion(ccsr, MPC8544_PCI_REGS_OFFSET,
> +    memory_region_add_subregion(ccsr_addr_space, MPC8544_PCI_REGS_OFFSET,
>                                 sysbus_mmio_get_region(s, 0));
> 
>     pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
> @@ -596,3 +604,33 @@ void ppce500_init(PPCE500Params *params)
>         kvmppc_init();
>     }
> }
> +
> +static int e500_ccsr_initfn(SysBusDevice *dev)
> +{
> +    PPCE500CCSRState *ccsr;
> +
> +    ccsr = CCSR(dev);
> +    memory_region_init(&ccsr->ccsr_space, "e500-ccsr",
> +                       MPC8544_CCSRBAR_SIZE);
> +    return 0;
> +}
> +
> +static void e500_ccsr_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    k->init = e500_ccsr_initfn;
> +}
> +
> +static const TypeInfo e500_ccsr_info = {
> +    .name          = TYPE_CCSR,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PPCE500CCSRState),
> +    .class_init    = e500_ccsr_class_init,
> +};
> +
> +static void e500_register_types(void)
> +{
> +    type_register_static(&e500_ccsr_info);
> +}
> +
> +type_init(e500_register_types)
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..9ca685e 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -15,6 +15,7 @@
>  */
> 
> #include "hw.h"
> +#include "hw/ppc/e500-ccsr.h"
> #include "pci.h"
> #include "pci_host.h"
> #include "bswap.h"
> @@ -89,6 +90,19 @@ struct PPCE500PCIState {
>     MemoryRegion iomem;
> };
> 
> +#define TYPE_PPC_E500_PCI_BRIDGE "e500-host-bridge"
> +#define PPC_E500_PCI_BRIDGE(obj) \
> +    OBJECT_CHECK(PPCE500PCIBridgeState, (obj), TYPE_PPC_E500_PCI_BRIDGE)
> +
> +struct PPCE500PCIBridgeState {
> +    /*< private >*/
> +    PCIDevice parent;
> +    /*< public >*/
> +
> +    MemoryRegion bar0;
> +};
> +
> +typedef struct PPCE500PCIBridgeState PPCE500PCIBridgeState;
> typedef struct PPCE500PCIState PPCE500PCIState;
> 
> static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
> @@ -307,6 +321,18 @@ static const VMStateDescription vmstate_ppce500_pci = {
> 
> #include "exec-memory.h"
> 
> +static int e500_pcihost_bridge_initfn(PCIDevice *d)
> +{
> +    PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
> +    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
> +                                  "/e500-ccsr"));
> +
> +    memory_region_init_alias(&b->bar0, "e500-pci-bar0", &ccsr->ccsr_space,
> +                             0, int128_get64(ccsr->ccsr_space.size));
> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
> +    return 0;
> +}
> +
> static int e500_pcihost_initfn(SysBusDevice *dev)
> {
>     PCIHostState *h;
> @@ -350,6 +376,7 @@ static void e500_host_bridge_class_init(ObjectClass 
> *klass, void *data)
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> 
> +    k->init = e500_pcihost_bridge_initfn;
>     k->vendor_id = PCI_VENDOR_ID_FREESCALE;
>     k->device_id = PCI_DEVICE_ID_MPC8533E;
>     k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
> @@ -359,7 +386,7 @@ static void e500_host_bridge_class_init(ObjectClass 
> *klass, void *data)
> static const TypeInfo e500_host_bridge_info = {
>     .name          = "e500-host-bridge",
>     .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(PCIDevice),
> +    .instance_size = sizeof(PPCE500PCIBridgeState),
>     .class_init    = e500_host_bridge_class_init,
> };
> 
> -- 
> 1.7.0.4
> 
>