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

[Alexander Graf]

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