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

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Thursday, October 04, 2012 9:37 PM
> To: Bhushan Bharat-R65777
> Cc: Avi Kivity; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI controller
> 
> 
> On 04.10.2012, at 18:03, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Avi Kivity [mailto:address@hidden
> >> Sent: Thursday, October 04, 2012 8:28 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: address@hidden; address@hidden; address@hidden
> >> Subject: Re: [Qemu-devel] [PATCH 2/2] Adding BAR0 for e500 PCI
> >> controller
> >>
> >> On 10/04/2012 03:46 PM, Bhushan Bharat-R65777 wrote:
> >>>
> >>>
> >>>> -----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 *.
> >>
> >> Then you shouldn't be using qdev for this.  But if you make the
> >> changes below, it will likely not be necessary.
> >>
> >>>>
> >>>> 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.
> >>
> >> Please elaborate.  One way to describe what you want is to issue an 'info
> mtree'
> >> and show what changes you want.
> >
> > Setup is something like this:
> >
> >  |-------------|
> >  | PCI device  |
> >  |             |
> >  ---------------
> >        |
> >        |
> >        |
> >        |
> > |-------------------|
> > |                   |
> > | PCI controller    |
> > |                   |
> > |   --------------  |
> > |   |  BAR0 in   |  |
> > |   |   TYPE1    |  |
> > |   | Config HDR |  |
> > |   --------------  |
> > |                   |
> >  -------------------
> >
> > BAR0: it is an inbound window, and on e500 devices this maps the pci bus
> address to CCSR address.
> >
> > My requirement are:
> > 1) when guest read pci controller BAR0, it is able to get proper size
> > ( Size of CCSR space)
> > 2) Guest can program this to any address in pci address space. When PCI 
> > device
> access this address range then that address should be mapped to CCSR address
> space.
> >
> > Though this patch only met the requirement number (1).
> 
> No, it also meets (2). The PCI address space is identical to the CPU memory
> space in our mapping right now. So if the guest maps BAR0 somewhere, it
> automatically maps CCSR into CPU address space, which exposes it to PCI 
> address
> space.
> 
> >
> >>
> >>>
> >>> Which device's initialization function you are talking about?
> >>
> >> static const TypeInfo e500_host_bridge_info = {
> >>    .name          = "e500-host-bridge",
> >>    .parent        = TYPE_PCI_DEVICE,
> >>    .instance_size = sizeof(PCIDevice),
> >>    .class_init    = e500_host_bridge_class_init,
> >> };
> >>
> >> This needs to describe a derived class of PCIDevice, hold the BAR as
> >> a data member, and register the BAR in the init function (which
> >> doesn't exist yet because you haven't subclassed it).  This way the
> >> BAR lives in the device which exposes it, like BARs everywhere.
> >
> > Do you mean that we add the "MemoryRegion bar0" in PCIDevice struct. Do the
> same thing that I was doing in e500_pcihost_initfn() in the k->init() (will 
> add
> this) function of "e500-host-bridge"
> 
> No, he means that you create a new struct like this:
> 
>   struct foo {
>     PCIDevice p;
>     MemoryRegion bar0;
>   };
> 
> Please check out any other random PCI device in QEMU. Almost all of them do 
> this
> to store more information than their parent class can hold.

Just want to be sure I understood you correctly: Do you mean something like 
this : ( I know I have to switch to QOM mechanism to share parameters)

diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 92b1dc0..a948bc6 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -89,6 +89,12 @@ struct PPCE500PCIState {
     MemoryRegion iomem;
 };
 
+struct BHARAT {
+    PCIDevice p;
+    void *bar0;
+};
+
+typedef struct BHARAT bharat;
 typedef struct PPCE500PCIState PPCE500PCIState;
 
 static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
@@ -307,6 +313,16 @@ static const VMStateDescription vmstate_ppce500_pci = {
 
 #include "exec-memory.h"
 
+static int e500_pcihost_bridge_initfn(PCIDevice *d)
+{
+    bharat *b = DO_UPCAST(bharat, p, d);
+
+    printf("Addr = %llx, size = %llx\n", ((MemoryRegion *)b->bar0)->addr, 
(unsigned long long)int128_get64(((Me
+    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion 
*)b->bar0);
+    return 0;
+}
+
+MemoryRegion ccsr;
 static int e500_pcihost_initfn(SysBusDevice *dev)
 {
     PCIHostState *h;
@@ -315,6 +331,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
     int i;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *address_space_io = get_system_io();
+    PCIDevice *d;
 
     h = PCI_HOST_BRIDGE(dev);
     s = PPC_E500_PCI_HOST_BRIDGE(dev);
@@ -328,7 +345,12 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
                          address_space_io, PCI_DEVFN(0x11, 0), 4);
     h->bus = b;
 
-    pci_create_simple(b, 0, "e500-host-bridge");
+    d = pci_create(b, 0, "e500-host-bridge");
+    /* ccsr-> should be passed from hw/ppc/e500.c */
+    ccsr.addr = 0xE0000000;
+    ccsr.size = int128_make64(0x00100000);
+    qdev_prop_set_ptr(&d->qdev, "bar0_region", &ccsr);
+    qdev_init_nofail(&d->qdev);
 
     memory_region_init(&s->container, "pci-container", PCIE500_ALL_SIZE);
     memory_region_init_io(&h->conf_mem, &pci_host_conf_be_ops, h,
@@ -345,11 +367,18 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
     return 0;
 }
 
+static Property pci_host_dev_info[] = {
+    DEFINE_PROP_PTR("bar0_region", bharat, bar0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 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;
+    dc->props = pci_host_dev_info;
     k->vendor_id = PCI_VENDOR_ID_FREESCALE;
     k->device_id = PCI_DEVICE_ID_MPC8533E;
     k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
@@ -359,10 +388,11 @@ 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(bharat),
     .class_init    = e500_host_bridge_class_init,
 };
 
 static void e500_pcihost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);

Thanks
-Bharat

> 
> 
> Alex
> 
> >
> > This way I should be able to met both of my requirements.
> >
> > Thanks
> > -Bharat
> >
> >>
> >> --
> >> error compiling committee.c: too many arguments to function
> >
> >
>