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

[Alexander Graf]

On 05.10.2012, at 09:11, Bhushan Bharat-R65777 wrote:

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

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