| Message ID | 1349265000-23834-3-git-send-email-Bharat.Bhushan@freescale.com |
|---|---|
| State | New |
| Headers | show |
On 03.10.2012, at 13:50, Bharat Bhushan wrote: > PCI Root complex have TYPE-1 configuration header while PCI endpoint > have type-0 configuration header. The type-1 configuration header have > a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci > address space to CCSR address space. This can used for 2 purposes: 1) > for MSI interrupt generation 2) Allow CCSR registers access when configured > as PCI endpoint, which I am not sure is a use case with QEMU-KVM guest. > > What I observed is that when guest read the size of BAR0 of host controller > configuration header (TYPE1 header) then it always reads it as 0. When > looking into the QEMU hw/ppce500_pci.c, I do not find the PCI controller > device registering BAR0. I do not find any other controller also doing so > may they do not use BAR0. > > There are two issues when BAR0 is not there (which I can think of): > 1) There should be BAR0 emulated for PCI Root comaplex (TYPE1 header) and > when reading the size of BAR0, it should give size as per real h/w. > > This patch solves this problem. > > 2) Do we need this BAR0 inbound address translation? > When BAR0 is of non-zero size then it will be configured for PCI > address space to local address(CCSR) space translation on inbound access. > The primary use case is for MSI interrupt generation. The device is > configured with a address offsets in PCI address space, which will be > translated to MSI interrupt generation MPIC registers. Currently I do > not understand the MSI interrupt generation mechanism in QEMU and also > IIRC we do not use QEMU MSI interrupt mechanism on e500 guest machines. > But this BAR0 will be used when using MSI on e500. > > I can see one more issue, There are ATMUs emulated in hw/ppce500_pci.c, > but i do not see these being used for address translation. > So far that works because pci address space and local address space are 1:1 > mapped. BAR0 inbound translation + ATMU translation will complete the address > translation of inbound traffic. > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> > --- > hw/ppc/e500.c | 1 + > hw/ppce500_pci.c | 13 +++++++++++++ > 2 files changed, 14 insertions(+), 0 deletions(-) > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c > index 197411d..c7ae2b6 100644 > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params) > > /* PCI */ > dev = qdev_create(NULL, "e500-pcihost"); > + qdev_prop_set_ptr(dev, "bar0_region", ccsr); > qdev_init_nofail(dev); > s = sysbus_from_qdev(dev); > 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; > }; > > typedef struct PPCE500PCIState PPCE500PCIState; > @@ -315,6 +316,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev) > int i; > MemoryRegion *address_space_mem = get_system_memory(); > MemoryRegion *address_space_io = get_system_io(); > + PCIDevice *pdev; > + MemoryRegion bar0; > > h = PCI_HOST_BRIDGE(dev); > s = PPC_E500_PCI_HOST_BRIDGE(dev); > @@ -342,6 +345,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev) > memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem); > sysbus_init_mmio(dev, &s->container); > > + bar0 = *(MemoryRegion *)s->bar0; > + pdev = pci_find_device(b, 0, 0); > + pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0); Any reason you can't just pass in s->bar0 here? Though I'm not sure whether we have to do something special to get a memory region alias. Avi, any ideas? We want the same memory region mapped twice. Once at a fixed address, once as BAR0 of the PCI host controller. Alex > + > return 0; > } > > @@ -363,6 +370,11 @@ static const TypeInfo e500_host_bridge_info = { > .class_init = e500_host_bridge_class_init, > }; > > +static Property pci_host_dev_info[] = { > + DEFINE_PROP_PTR("bar0_region", PPCE500PCIState, bar0), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void e500_pcihost_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -370,6 +382,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data) > > k->init = e500_pcihost_initfn; > dc->vmsd = &vmstate_ppce500_pci; > + dc->props = pci_host_dev_info; > } > > static const TypeInfo e500_pcihost_info = { > -- > 1.7.0.4 > >
> -----Original Message----- > From: Alexander Graf [mailto:agraf@suse.de] > Sent: Wednesday, October 03, 2012 5:41 PM > To: Bhushan Bharat-R65777 > Cc: qemu-devel qemu-devel; qemu-ppc@nongnu.org List; Bhushan Bharat-R65777; Avi > Kivity > Subject: Re: [PATCH 2/2] Adding BAR0 for e500 PCI controller > > > On 03.10.2012, at 13:50, Bharat Bhushan wrote: > > > PCI Root complex have TYPE-1 configuration header while PCI endpoint > > have type-0 configuration header. The type-1 configuration header have > > a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci > > address space to CCSR address space. This can used for 2 purposes: 1) > > for MSI interrupt generation 2) Allow CCSR registers access when > > configured as PCI endpoint, which I am not sure is a use case with QEMU-KVM > guest. > > > > What I observed is that when guest read the size of BAR0 of host > > controller configuration header (TYPE1 header) then it always reads it > > as 0. When looking into the QEMU hw/ppce500_pci.c, I do not find the > > PCI controller device registering BAR0. I do not find any other > > controller also doing so may they do not use BAR0. > > > > There are two issues when BAR0 is not there (which I can think of): > > 1) There should be BAR0 emulated for PCI Root comaplex (TYPE1 header) > > and when reading the size of BAR0, it should give size as per real h/w. > > > > This patch solves this problem. > > > > 2) Do we need this BAR0 inbound address translation? > > When BAR0 is of non-zero size then it will be configured for > > PCI address space to local address(CCSR) space translation on inbound access. > > The primary use case is for MSI interrupt generation. The device is > > configured with a address offsets in PCI address space, which will be > > translated to MSI interrupt generation MPIC registers. Currently I do > > not understand the MSI interrupt generation mechanism in QEMU and also > > IIRC we do not use QEMU MSI interrupt mechanism on e500 guest machines. > > But this BAR0 will be used when using MSI on e500. > > > > I can see one more issue, There are ATMUs emulated in > > hw/ppce500_pci.c, but i do not see these being used for address translation. > > So far that works because pci address space and local address space > > are 1:1 mapped. BAR0 inbound translation + ATMU translation will > > complete the address translation of inbound traffic. > > > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> > > --- > > hw/ppc/e500.c | 1 + > > hw/ppce500_pci.c | 13 +++++++++++++ > > 2 files changed, 14 insertions(+), 0 deletions(-) > > > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 197411d..c7ae2b6 > > 100644 > > --- a/hw/ppc/e500.c > > +++ b/hw/ppc/e500.c > > @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params) > > > > /* PCI */ > > dev = qdev_create(NULL, "e500-pcihost"); > > + qdev_prop_set_ptr(dev, "bar0_region", ccsr); > > qdev_init_nofail(dev); > > s = sysbus_from_qdev(dev); > > 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; > > }; > > > > typedef struct PPCE500PCIState PPCE500PCIState; @@ -315,6 +316,8 @@ > > static int e500_pcihost_initfn(SysBusDevice *dev) > > int i; > > MemoryRegion *address_space_mem = get_system_memory(); > > MemoryRegion *address_space_io = get_system_io(); > > + PCIDevice *pdev; > > + MemoryRegion bar0; > > > > h = PCI_HOST_BRIDGE(dev); > > s = PPC_E500_PCI_HOST_BRIDGE(dev); @@ -342,6 +345,10 @@ static int > > e500_pcihost_initfn(SysBusDevice *dev) > > memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem); > > sysbus_init_mmio(dev, &s->container); > > > > + bar0 = *(MemoryRegion *)s->bar0; > > + pdev = pci_find_device(b, 0, 0); > > + pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0); > > Any reason you can't just pass in s->bar0 here? Though I'm not sure whether we > have to do something special to get a memory region alias. Yes I think this should work: pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, (MemoryRegion *)s->bar0); Thanks -Bharat > > Avi, any ideas? We want the same memory region mapped twice. Once at a fixed > address, once as BAR0 of the PCI host controller. > > > Alex > > > + > > return 0; > > } > > > > @@ -363,6 +370,11 @@ static const TypeInfo e500_host_bridge_info = { > > .class_init = e500_host_bridge_class_init, > > }; > > > > +static Property pci_host_dev_info[] = { > > + DEFINE_PROP_PTR("bar0_region", PPCE500PCIState, bar0), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > static void e500_pcihost_class_init(ObjectClass *klass, void *data) { > > DeviceClass *dc = DEVICE_CLASS(klass); @@ -370,6 +382,7 @@ static > > void e500_pcihost_class_init(ObjectClass *klass, void *data) > > > > k->init = e500_pcihost_initfn; > > dc->vmsd = &vmstate_ppce500_pci; > > + dc->props = pci_host_dev_info; > > } > > > > static const TypeInfo e500_pcihost_info = { > > -- > > 1.7.0.4 > > > > >
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? > > typedef struct PPCE500PCIState PPCE500PCIState; > @@ -315,6 +316,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev) > int i; > MemoryRegion *address_space_mem = get_system_memory(); > MemoryRegion *address_space_io = get_system_io(); > + PCIDevice *pdev; > + MemoryRegion bar0; > > h = PCI_HOST_BRIDGE(dev); > s = PPC_E500_PCI_HOST_BRIDGE(dev); > @@ -342,6 +345,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev) > memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem); > sysbus_init_mmio(dev, &s->container); > > + bar0 = *(MemoryRegion *)s->bar0; > + pdev = pci_find_device(b, 0, 0); > + pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0); > + This is broken, you're registering an object on the stack which will be freed as soon as the function returns. Just pass &s->bar0 as Alex suggests. 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). > return 0; > } >
> -----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 *. > > > > > typedef struct PPCE500PCIState PPCE500PCIState; @@ -315,6 +316,8 @@ > > static int e500_pcihost_initfn(SysBusDevice *dev) > > int i; > > MemoryRegion *address_space_mem = get_system_memory(); > > MemoryRegion *address_space_io = get_system_io(); > > + PCIDevice *pdev; > > + MemoryRegion bar0; > > > > h = PCI_HOST_BRIDGE(dev); > > s = PPC_E500_PCI_HOST_BRIDGE(dev); @@ -342,6 +345,10 @@ static > > int e500_pcihost_initfn(SysBusDevice *dev) > > memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem); > > sysbus_init_mmio(dev, &s->container); > > > > + bar0 = *(MemoryRegion *)s->bar0; > > + pdev = pci_find_device(b, 0, 0); > > + pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0); > > + > > This is broken, you're registering an object on the stack which will be freed as > soon as the function returns. > > Just pass &s->bar0 as Alex suggests. Ok. > > 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. Which device's initialization function you are talking about? Thanks -Bharat > > > return 0; > > } > > > > > -- > error compiling committee.c: too many arguments to function
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. > > 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.
Am 03.10.2012 13:50, schrieb Bharat Bhushan: > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c > index 197411d..c7ae2b6 100644 > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params) > > /* PCI */ > dev = qdev_create(NULL, "e500-pcihost"); > + qdev_prop_set_ptr(dev, "bar0_region", ccsr); > qdev_init_nofail(dev); > s = sysbus_from_qdev(dev); > sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); Please... > 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; > }; > > typedef struct PPCE500PCIState PPCE500PCIState; ...do not do this. qdev_prop_set_ptr() is considered deprecated and we had long discussions how to solve this differently. Was there anything wrong with using a SysBusDevice for the CCSR to encapsulate the MemoryRegion? Andreas
On 04.10.2012, at 17:54, Andreas Färber wrote: > Am 03.10.2012 13:50, schrieb Bharat Bhushan: >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >> index 197411d..c7ae2b6 100644 >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params) >> >> /* PCI */ >> dev = qdev_create(NULL, "e500-pcihost"); >> + qdev_prop_set_ptr(dev, "bar0_region", ccsr); >> qdev_init_nofail(dev); >> s = sysbus_from_qdev(dev); >> sysbus_connect_irq(s, 0, mpic[pci_irq_nrs[0]]); > > Please... > >> 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; >> }; >> >> typedef struct PPCE500PCIState PPCE500PCIState; > > ...do not do this. qdev_prop_set_ptr() is considered deprecated and we > had long discussions how to solve this differently. > > Was there anything wrong with using a SysBusDevice for the CCSR to > encapsulate the MemoryRegion? Not at all. This was meant as a first shot, so we can slowly move towards the CCSR-as-device model. In fact, now that we have this code as far as it is, we can go over to tackle it that way. Bharat, mind to model a new CCSR device now that contains all the CCSR devices? That one creates a memory region then. The PCI host controller gets a reference to the CCSR device and from there can call a CCSR specific function to receive the memory region pointer. And suddenly we have all of this solved :). Alex
> -----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). > > > > > 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" This way I should be able to met both of my requirements. Thanks -Bharat > > -- > error compiling committee.c: too many arguments to function
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. Alex > > This way I should be able to met both of my requirements. > > Thanks > -Bharat > >> >> -- >> error compiling committee.c: too many arguments to function > >
> -----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. Really? I think on powerpc the pci address space is defined as: it maps the outbound window just below 0x1_0000_0000, then CCSR and then inbound window. So inbound window is 1:1 map if guest physical starts from 0x0. But I do not think CCSR is 1:1 map in pci address space and cpu 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. Ok, 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 04.10.2012, at 18:48, 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. > > Really? I think on powerpc the pci address space is defined as: it maps the outbound window just below 0x1_0000_0000, then CCSR and then inbound window. So inbound window is 1:1 map if guest physical starts from 0x0. But I do not think CCSR is 1:1 map in pci address space and cpu address space. In QEMU, we map everything 1:1 today. Alex
On 10/04/2012 06:50 PM, Alexander Graf wrote: >>> >>> 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. >> >> Really? I think on powerpc the pci address space is defined as: it maps the outbound window just below 0x1_0000_0000, then CCSR and then inbound window. So inbound window is 1:1 map if guest physical starts from 0x0. But I do not think CCSR is 1:1 map in pci address space and cpu address space. > > In QEMU, we map everything 1:1 today. An unmerged patch set entitled "Integrate DMA into the memory API" changes that. I'll be happy to work with you to make use of it to emulate the hardware properly, it will give me a nice test case.
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 197411d..c7ae2b6 100644 --- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -518,6 +518,7 @@ void ppce500_init(PPCE500Params *params) /* PCI */ dev = qdev_create(NULL, "e500-pcihost"); + qdev_prop_set_ptr(dev, "bar0_region", ccsr); qdev_init_nofail(dev); s = sysbus_from_qdev(dev); 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; }; typedef struct PPCE500PCIState PPCE500PCIState; @@ -315,6 +316,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev) int i; MemoryRegion *address_space_mem = get_system_memory(); MemoryRegion *address_space_io = get_system_io(); + PCIDevice *pdev; + MemoryRegion bar0; h = PCI_HOST_BRIDGE(dev); s = PPC_E500_PCI_HOST_BRIDGE(dev); @@ -342,6 +345,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev) memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem); sysbus_init_mmio(dev, &s->container); + bar0 = *(MemoryRegion *)s->bar0; + pdev = pci_find_device(b, 0, 0); + pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &bar0); + return 0; } @@ -363,6 +370,11 @@ static const TypeInfo e500_host_bridge_info = { .class_init = e500_host_bridge_class_init, }; +static Property pci_host_dev_info[] = { + DEFINE_PROP_PTR("bar0_region", PPCE500PCIState, bar0), + DEFINE_PROP_END_OF_LIST(), +}; + static void e500_pcihost_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -370,6 +382,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data) k->init = e500_pcihost_initfn; dc->vmsd = &vmstate_ppce500_pci; + dc->props = pci_host_dev_info; } static const TypeInfo e500_pcihost_info = {
PCI Root complex have TYPE-1 configuration header while PCI endpoint have type-0 configuration header. The type-1 configuration header have a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci address space to CCSR address space. This can used for 2 purposes: 1) for MSI interrupt generation 2) Allow CCSR registers access when configured as PCI endpoint, which I am not sure is a use case with QEMU-KVM guest. What I observed is that when guest read the size of BAR0 of host controller configuration header (TYPE1 header) then it always reads it as 0. When looking into the QEMU hw/ppce500_pci.c, I do not find the PCI controller device registering BAR0. I do not find any other controller also doing so may they do not use BAR0. There are two issues when BAR0 is not there (which I can think of): 1) There should be BAR0 emulated for PCI Root comaplex (TYPE1 header) and when reading the size of BAR0, it should give size as per real h/w. This patch solves this problem. 2) Do we need this BAR0 inbound address translation? When BAR0 is of non-zero size then it will be configured for PCI address space to local address(CCSR) space translation on inbound access. The primary use case is for MSI interrupt generation. The device is configured with a address offsets in PCI address space, which will be translated to MSI interrupt generation MPIC registers. Currently I do not understand the MSI interrupt generation mechanism in QEMU and also IIRC we do not use QEMU MSI interrupt mechanism on e500 guest machines. But this BAR0 will be used when using MSI on e500. I can see one more issue, There are ATMUs emulated in hw/ppce500_pci.c, but i do not see these being used for address translation. So far that works because pci address space and local address space are 1:1 mapped. BAR0 inbound translation + ATMU translation will complete the address translation of inbound traffic. Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com> --- hw/ppc/e500.c | 1 + hw/ppce500_pci.c | 13 +++++++++++++ 2 files changed, 14 insertions(+), 0 deletions(-)