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

[Bhushan Bharat-R65777]

> >>>>> 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;
> 
> MemoryRegion *bar0

If I uses " MemoryRegion *bar0"  or " MemoryRegion bar0" then how the size and 
address of bar0 will be populated?

As far as I can see, other PCI devices using this way to have extra information 
but they does not get MemoryRegion information from other object.

> 
> > +};
> > +
> > +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);
> 
> That one still has to call its parent initfn, no?

I am really sorry, but I did not get. The object says its parent is 
TYPE_PCI_DEVICE, so which function are you talking about?

> 
> > +    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);
> 
> What is this?

I am trying to pass the CCSR information to "e500-host-bridge". This is 
currently hardcoded, but finally this will come from hw/ppc/e500.c.

Thanks
-Bharat

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