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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Wednesday, October 03, 2012 5:41 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel qemu-devel; address@hidden List; Bhushan Bharat-R65777; Avi
> Kivity
> Subject: Re: [PATCH 2/2] Adding BAR0 for e500 PCI controller
> 
> 
> 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.

Yes I think this should work:
         pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion 
*)s->bar0);

Thanks
-Bharat

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