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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Wednesday, September 19, 2012 4:33 PM
> To: Bhushan Bharat-R65777
> Cc: address@hidden; address@hidden; address@hidden; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH] Adding BAR0 for e500 PCI controller
> 
> 
> 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.

Yes, ok.

> 
> > /* 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.

Right, will take care in next version.

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

Yes, I will first do this cleanup and then make bar0 patch on the cleanup patch.

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

Right, especially the MPIC device for MSI interrupt will be inherited.

Thanks
-Bharat

> 
> 
> Alex
>