| Message ID | 6A3DF150A5B70D4F9B66A25E3F7C888D064B410A@039-SN2MPN1-022.039d.mgd.msft.net |
|---|---|
| State | New |
| Headers | show |
On 05.10.2012, at 09:11, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Thursday, October 04, 2012 9:37 PM >> To: Bhushan Bharat-R65777 >> Cc: Avi Kivity; qemu-devel@nongnu.org; qemu-ppc@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:avi@redhat.com] >>>> Sent: Thursday, October 04, 2012 8:28 PM >>>> To: Bhushan Bharat-R65777 >>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@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:avi@redhat.com] >>>>>> Sent: Thursday, October 04, 2012 6:02 PM >>>>>> To: Bhushan Bharat-R65777 >>>>>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@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 >>> >>> >> > >
On 10/05/2012 01:59 PM, Alexander Graf wrote: >>>> 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 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 memory_region_init_io(&d->bar0, ...); >> + pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)b->bar0); > > That one still has to call its parent initfn, no? > yes
On 07.10.2012, at 11:48, Avi Kivity <avi@redhat.com> wrote: > On 10/05/2012 01:59 PM, Alexander Graf wrote: >>>>> 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 > > MemoryRegion bar0; Why? We want the same region that is mapped outside of the pci device be available as BAR0 too. Alex > >> >>> +}; >>> + >>> +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 > > memory_region_init_io(&d->bar0, ...); > >>> + pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)b->bar0); >> >> That one still has to call its parent initfn, no? >> > > yes > > -- > error compiling committee.c: too many arguments to function
On 10/07/2012 01:57 PM, Alexander Graf wrote: > > > On 07.10.2012, at 11:48, Avi Kivity <avi@redhat.com> wrote: > >> On 10/05/2012 01:59 PM, Alexander Graf wrote: >>>>>> 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 >> >> MemoryRegion bar0; > > Why? We want the same region that is mapped outside of the pci device be available as BAR0 too. A MemoryRegion can only have one container. If you want dual maps, use memory_region_init_alias(). It is possible (not trivial though) to extend the memory API to support m:n container:subregion relationships. I don't think it's worthwhile though.
> >>>>> 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? 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? > > > + 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. Thanks -Bharat > > > 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 > >>> > >>> > >> > > > > >
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
Am 08.10.2012 10:50, schrieb Alexander Graf: > > On 08.10.2012, at 10:23, Bhushan Bharat-R65777 wrote: >>>> @@ -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? .class_init and .instance_init are called iteratively through the hierarchy, so no parent method needs to be called manually. This is different for user-coded methods like CPUState::reset. qdev initfns haven't changed in that respect to date - they are orthogonal to QOM. Andreas
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);