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