> -----Original Message-----
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Wednesday, September 19, 2012 4:33 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; afaer...@suse.de; 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 <bharat.bhus...@freescale.com>
> > ---
> > 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
>
