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

[Alexander Graf]

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.


Alex

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