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