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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Avi Kivity [mailto:address@hidden
> Sent: Thursday, October 04, 2012 6:02 PM
> To: Bhushan Bharat-R65777
> Cc: address@hidden; address@hidden; address@hidden; Bhushan Bharat-
> R65777
> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
> 
> On 10/03/2012 01:50 PM, Bharat Bhushan wrote:
> >      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;
> >  };
> 
> void *?  Why?

Passing parameter via qdev is either possible by value or by void *. That's why 
I used void *.

> 
> >
> >  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);
> > +
> 
> This is broken, you're registering an object on the stack which will be freed 
> as
> soon as the function returns.
> 
> Just pass &s->bar0 as Alex suggests.

Ok.

> 
> However this is best done from the pci device's initialization function, not
> here (the MemoryRegion should be a member of the pci device as well).

We want to set BAR0 of PCI controller so we did this here. But yes, we also 
want that all devices under the PCI controller have this mapping, so when they 
access the MPIC register to send MSI then they can do that.

Which device's initialization function you are talking about?

Thanks
-Bharat

> 
> >      return 0;
> >  }
> >
> 
> 
> --
> error compiling committee.c: too many arguments to function