Patchwork [2/2] Adding BAR0 for e500 PCI controller

login
register
mail settings
Submitter Bharat Bhushan
Date Oct. 5, 2012, 7:11 a.m.
Message ID <6A3DF150A5B70D4F9B66A25E3F7C888D064B410A@039-SN2MPN1-022.039d.mgd.msft.net>
Download mbox | patch
Permalink /patch/189417/
State New
Headers show

Comments

Bharat Bhushan - Oct. 5, 2012, 7:11 a.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, October 04, 2012 9:37 PM
> To: Bhushan Bharat-R65777
> Cc: Avi Kivity; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> 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:avi@redhat.com]
> >> Sent: Thursday, October 04, 2012 8:28 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de
> >> 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:avi@redhat.com]
> >>>> Sent: Thursday, October 04, 2012 6:02 PM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;
> >>>> 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)

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
> >
> >
>
Alexander Graf - Oct. 5, 2012, 11:59 a.m.
On 05.10.2012, at 09:11, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, October 04, 2012 9:37 PM
>> To: Bhushan Bharat-R65777
>> Cc: Avi Kivity; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
>> 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:avi@redhat.com]
>>>> Sent: Thursday, October 04, 2012 8:28 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de
>>>> 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:avi@redhat.com]
>>>>>> Sent: Thursday, October 04, 2012 6:02 PM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;
>>>>>> 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;

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

That one still has to call its parent initfn, no?

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


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
>>> 
>>> 
>> 
> 
>
Avi Kivity - Oct. 7, 2012, 9:48 a.m.
On 10/05/2012 01:59 PM, Alexander Graf wrote:
>>>> 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

MemoryRegion 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

memory_region_init_io(&d->bar0, ...);

>> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)b->bar0);
> 
> That one still has to call its parent initfn, no?
> 

yes
Alexander Graf - Oct. 7, 2012, 11:57 a.m.
On 07.10.2012, at 11:48, Avi Kivity <avi@redhat.com> wrote:

> On 10/05/2012 01:59 PM, Alexander Graf wrote:
>>>>> 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
> 
> MemoryRegion bar0;

Why? We want the same region that is mapped outside of the pci device be available as BAR0 too.

Alex

> 
>> 
>>> +};
>>> +
>>> +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
> 
> memory_region_init_io(&d->bar0, ...);
> 
>>> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)b->bar0);
>> 
>> That one still has to call its parent initfn, no?
>> 
> 
> yes
> 
> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Oct. 7, 2012, 12:41 p.m.
On 10/07/2012 01:57 PM, Alexander Graf wrote:
> 
> 
> On 07.10.2012, at 11:48, Avi Kivity <avi@redhat.com> wrote:
> 
>> On 10/05/2012 01:59 PM, Alexander Graf wrote:
>>>>>> 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
>> 
>> MemoryRegion bar0;
> 
> Why? We want the same region that is mapped outside of the pci device be available as BAR0 too.

A MemoryRegion can only have one container.  If you want dual maps, use
memory_region_init_alias().

It is possible (not trivial though) to extend the memory API to support
m:n container:subregion relationships.  I don't think it's worthwhile
though.
Bharat Bhushan - Oct. 8, 2012, 8:23 a.m.
> >>>>> 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
> >>>
> >>>
> >>
> >
> >
>
Alexander Graf - Oct. 8, 2012, 8:50 a.m.
On 08.10.2012, at 10:23, Bhushan Bharat-R65777 wrote:

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

If I understand Avi's comments correctly, by creating a memory region alias. Who would be responsible for creating that alias and how that alias would come to be in this struct is still puzzling myself too though :).

Avi?

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

In object oriented programming, every time you overload a class, your constructor should call the overloaded class's constructor. I don't see this happening for other PCI device's init functions though.

Andreas, shouldn't parent class init be called for pci subclass devices?

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

hw/ppc/e500.c owns the CCSR memory region and maps the CCSR memory region. In Andreas' model, the whole CCSR region is a separate object. The PCI host device should only ever take the memory region it gets from the CCSR device (or hw/ppc/e500.c for now) and map that as BAR0. No need to know any details about it.


Alex
Andreas Färber - Oct. 8, 2012, 1:57 p.m.
Am 08.10.2012 10:50, schrieb Alexander Graf:
> 
> On 08.10.2012, at 10:23, Bhushan Bharat-R65777 wrote:
>>>> @@ -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?
> 
> In object oriented programming, every time you overload a class, your constructor should call the overloaded class's constructor. I don't see this happening for other PCI device's init functions though.
> 
> Andreas, shouldn't parent class init be called for pci subclass devices?

.class_init and .instance_init are called iteratively through the
hierarchy, so no parent method needs to be called manually. This is
different for user-coded methods like CPUState::reset. qdev initfns
haven't changed in that respect to date - they are orthogonal to QOM.

Andreas

Patch

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