| Message ID | 1344948607-23291-1-git-send-email-Bharat.Bhushan@freescale.com |
|---|---|
| State | New |
| Headers | show |
On 08/14/2012 07:50 AM, 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. > > 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. This patch is only trying to address #1, right? I don't see any connection from this BAR to CCSR. > + memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h, > + "PCIHOST-bar0", 0x1000000); 0x01000000 is correct for e500mc-based systems, but it should be 0x00100000 for e500v2-based systems. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, August 15, 2012 6:59 AM > To: Bhushan Bharat-R65777 > Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan Bharat- > R65777 > Subject: Re: [Qemu-ppc] [PATCH: RFC] Adding BAR0 for e500 PCI controller > > On 08/14/2012 07:50 AM, 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. > > > > 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. > > This patch is only trying to address #1, right? Yes. > I don't see any connection from > this BAR to CCSR. Yes, it is not there. That can be the next step. > > > + memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h, > > + "PCIHOST-bar0", 0x1000000); > > 0x01000000 is correct for e500mc-based systems, but it should be 0x00100000 for > e500v2-based systems. Yes, I will correct this. Thanks -Bharat > > -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, August 15, 2012 6:59 AM > To: Bhushan Bharat-R65777 > Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan Bharat- > R65777 > Subject: Re: [Qemu-ppc] [PATCH: RFC] Adding BAR0 for e500 PCI controller > > On 08/14/2012 07:50 AM, 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. > > > > 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. > > This patch is only trying to address #1, right? I don't see any connection from > this BAR to CCSR. > > > + memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h, > > + "PCIHOST-bar0", 0x1000000); > > 0x01000000 is correct for e500mc-based systems, but it should be 0x00100000 for > e500v2-based systems. Scott, Currently we have a generic e500 machine which have CCSR size 0x00100000 (MPC8544_CCSRBAR_SIZE). We do not have e500mc and e500v2 machines. So should we make this 0x00100000 as per generic e500 machine? Can we somehow pass this via qdev/varargs from machine emulation code (hw/ppc/e500.c) ? Thanks -Bharat > > -Scott
On 09/03/2012 01:44 AM, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- From: Wood Scott-B07421 Sent: Wednesday, >> August 15, 2012 6:59 AM To: Bhushan Bharat-R65777 Cc: >> qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan >> Bharat- R65777 Subject: Re: [Qemu-ppc] [PATCH: RFC] Adding BAR0 for >> e500 PCI controller >> >> On 08/14/2012 07:50 AM, 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. >>> >>> 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. >> >> This patch is only trying to address #1, right? I don't see any >> connection from this BAR to CCSR. >> >>> + memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h, + >>> "PCIHOST-bar0", 0x1000000); >> >> 0x01000000 is correct for e500mc-based systems, but it should be >> 0x00100000 for e500v2-based systems. > > Scott, > > Currently we have a generic e500 machine which have CCSR size > 0x00100000 (MPC8544_CCSRBAR_SIZE). We do not have e500mc and e500v2 > machines. So should we make this 0x00100000 as per generic e500 > machine? Yes, but structure it so that board code decides the size, not the PCI code. > Can we somehow pass this via qdev/varargs from machine emulation code > (hw/ppc/e500.c) ? Possibly, though it may not be the best idea to express every single aspect of intercomponent integration via qdev -- maybe that's best left for things that are reasonably user-tweakable. If CCSR size is user tweakable, it would be somewhere other than the PCI controller. -Scott
On 07.09.2012, at 01:15, Scott Wood <scottwood@freescale.com> wrote: > On 09/03/2012 01:44 AM, Bhushan Bharat-R65777 wrote: >> >> >>> -----Original Message----- From: Wood Scott-B07421 Sent: Wednesday, >>> August 15, 2012 6:59 AM To: Bhushan Bharat-R65777 Cc: >>> qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan >>> Bharat- R65777 Subject: Re: [Qemu-ppc] [PATCH: RFC] Adding BAR0 for >>> e500 PCI controller >>> >>> On 08/14/2012 07:50 AM, 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. >>>> >>>> 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. >>> >>> This patch is only trying to address #1, right? I don't see any >>> connection from this BAR to CCSR. >>> >>>> + memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h, + >>>> "PCIHOST-bar0", 0x1000000); >>> >>> 0x01000000 is correct for e500mc-based systems, but it should be >>> 0x00100000 for e500v2-based systems. >> >> Scott, >> >> Currently we have a generic e500 machine which have CCSR size >> 0x00100000 (MPC8544_CCSRBAR_SIZE). We do not have e500mc and e500v2 >> machines. So should we make this 0x00100000 as per generic e500 >> machine? > > Yes, but structure it so that board code decides the size, not the PCI code. > >> Can we somehow pass this via qdev/varargs from machine emulation code >> (hw/ppc/e500.c) ? > > Possibly, though it may not be the best idea to express every single > aspect of intercomponent integration via qdev -- maybe that's best left > for things that are reasonably user-tweakable. If CCSR size is user > tweakable, it would be somewhere other than the PCI controller. It depends. Qdev properties are basically object constructor parameters. So if you were weiting C++ code and would have a constructor that gets the size as argument, it would end up being modeled as qdev property. If however actual functionality differs, thus you would in OO speech create a subclass / child class, then you are better off creating a new device struct. In this case, I'm not sure. They are different devices really, but are close enough that the differences could be expressed through qdev properties. Let's ask someone who knows his way around these spheres. Andreas? :) Alex
On 09/07/2012 03:08 AM, Alexander Graf wrote: > > > On 07.09.2012, at 01:15, Scott Wood <scottwood@freescale.com> wrote: > >> On 09/03/2012 01:44 AM, Bhushan Bharat-R65777 wrote: >>> >>> >>>> -----Original Message----- From: Wood Scott-B07421 Sent: Wednesday, >>>> August 15, 2012 6:59 AM To: Bhushan Bharat-R65777 Cc: >>>> qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan >>>> Bharat- R65777 Subject: Re: [Qemu-ppc] [PATCH: RFC] Adding BAR0 for >>>> e500 PCI controller >>>> >>>> On 08/14/2012 07:50 AM, 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. >>>>> >>>>> 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. >>>> >>>> This patch is only trying to address #1, right? I don't see any >>>> connection from this BAR to CCSR. >>>> >>>>> + memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h, + >>>>> "PCIHOST-bar0", 0x1000000); >>>> >>>> 0x01000000 is correct for e500mc-based systems, but it should be >>>> 0x00100000 for e500v2-based systems. >>> >>> Scott, >>> >>> Currently we have a generic e500 machine which have CCSR size >>> 0x00100000 (MPC8544_CCSRBAR_SIZE). We do not have e500mc and e500v2 >>> machines. So should we make this 0x00100000 as per generic e500 >>> machine? >> >> Yes, but structure it so that board code decides the size, not the PCI code. >> >>> Can we somehow pass this via qdev/varargs from machine emulation code >>> (hw/ppc/e500.c) ? >> >> Possibly, though it may not be the best idea to express every single >> aspect of intercomponent integration via qdev -- maybe that's best left >> for things that are reasonably user-tweakable. If CCSR size is user >> tweakable, it would be somewhere other than the PCI controller. > > It depends. Qdev properties are basically object constructor > parameters. So if you were weiting C++ code and would have a > constructor that gets the size as argument, it would end up being > modeled as qdev property. > > If however actual functionality differs, thus you would in OO speech > create a subclass / child class, then you are better off creating a > new device struct. > > In this case, I'm not sure. They are different devices really, but > are close enough that the differences could be expressed through qdev > properties. I wasn't suggesting that they be different devices. I was suggesting that this isn't a property of the PCI controller, but rather of some other entity to which the PCI controller connects. So maybe a reference to the associated CCSR object would be a qdev parameter, but not the size of that CCSR. -Scott
On 09/07/2012 08:58 PM, Scott Wood wrote: > On 09/07/2012 03:08 AM, Alexander Graf wrote: >> >> On 07.09.2012, at 01:15, Scott Wood <scottwood@freescale.com> wrote: >> >>> On 09/03/2012 01:44 AM, Bhushan Bharat-R65777 wrote: >>>> >>>>> -----Original Message----- From: Wood Scott-B07421 Sent: Wednesday, >>>>> August 15, 2012 6:59 AM To: Bhushan Bharat-R65777 Cc: >>>>> qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan >>>>> Bharat- R65777 Subject: Re: [Qemu-ppc] [PATCH: RFC] Adding BAR0 for >>>>> e500 PCI controller >>>>> >>>>> On 08/14/2012 07:50 AM, 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. >>>>>> >>>>>> 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. >>>>> This patch is only trying to address #1, right? I don't see any >>>>> connection from this BAR to CCSR. >>>>> >>>>>> + memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h, + >>>>>> "PCIHOST-bar0", 0x1000000); >>>>> 0x01000000 is correct for e500mc-based systems, but it should be >>>>> 0x00100000 for e500v2-based systems. >>>> Scott, >>>> >>>> Currently we have a generic e500 machine which have CCSR size >>>> 0x00100000 (MPC8544_CCSRBAR_SIZE). We do not have e500mc and e500v2 >>>> machines. So should we make this 0x00100000 as per generic e500 >>>> machine? >>> Yes, but structure it so that board code decides the size, not the PCI code. >>> >>>> Can we somehow pass this via qdev/varargs from machine emulation code >>>> (hw/ppc/e500.c) ? >>> Possibly, though it may not be the best idea to express every single >>> aspect of intercomponent integration via qdev -- maybe that's best left >>> for things that are reasonably user-tweakable. If CCSR size is user >>> tweakable, it would be somewhere other than the PCI controller. >> It depends. Qdev properties are basically object constructor >> parameters. So if you were weiting C++ code and would have a >> constructor that gets the size as argument, it would end up being >> modeled as qdev property. >> >> If however actual functionality differs, thus you would in OO speech >> create a subclass / child class, then you are better off creating a >> new device struct. >> >> In this case, I'm not sure. They are different devices really, but >> are close enough that the differences could be expressed through qdev >> properties. > I wasn't suggesting that they be different devices. I was suggesting > that this isn't a property of the PCI controller, but rather of some > other entity to which the PCI controller connects. So maybe a reference > to the associated CCSR object would be a qdev parameter, but not the > size of that CCSR. The first common place of information we get is the machine description. So here we can do: create_device(e500_ccsr, CCSR_SIZE); create_device(e500_pci_host_controller, CCSR_SIZE); Obviously code-wise this would look quite different from above, as object constructor parameters go through qdev properties. Alex
Am 07.09.2012 20:58, schrieb Scott Wood: > On 09/07/2012 03:08 AM, Alexander Graf wrote: >> >> On 07.09.2012, at 01:15, Scott Wood <scottwood@freescale.com> wrote: >> >>> On 09/03/2012 01:44 AM, Bhushan Bharat-R65777 wrote: >>>> >>>> Can we somehow pass this via qdev/varargs from machine emulation code >>>> (hw/ppc/e500.c) ? >>> >>> Possibly, though it may not be the best idea to express every single >>> aspect of intercomponent integration via qdev -- maybe that's best left >>> for things that are reasonably user-tweakable. If CCSR size is user >>> tweakable, it would be somewhere other than the PCI controller. >> >> It depends. Qdev properties are basically object constructor >> parameters. So if you were weiting C++ code and would have a >> constructor that gets the size as argument, it would end up being >> modeled as qdev property. >> >> If however actual functionality differs, thus you would in OO speech >> create a subclass / child class, then you are better off creating a >> new device struct. >> >> In this case, I'm not sure. They are different devices really, but >> are close enough that the differences could be expressed through qdev >> properties. > > I wasn't suggesting that they be different devices. I was suggesting > that this isn't a property of the PCI controller, but rather of some > other entity to which the PCI controller connects. So maybe a reference > to the associated CCSR object would be a qdev parameter, but not the > size of that CCSR. For a reference to another object a QOM link<> property would be preferred over a static qdev property. Andreas
On 09/11/2012 02:23 PM, Andreas Färber wrote: > Am 07.09.2012 20:58, schrieb Scott Wood: >> On 09/07/2012 03:08 AM, Alexander Graf wrote: >>> On 07.09.2012, at 01:15, Scott Wood <scottwood@freescale.com> wrote: >>> >>>> On 09/03/2012 01:44 AM, Bhushan Bharat-R65777 wrote: >>>>> Can we somehow pass this via qdev/varargs from machine emulation code >>>>> (hw/ppc/e500.c) ? >>>> Possibly, though it may not be the best idea to express every single >>>> aspect of intercomponent integration via qdev -- maybe that's best left >>>> for things that are reasonably user-tweakable. If CCSR size is user >>>> tweakable, it would be somewhere other than the PCI controller. >>> It depends. Qdev properties are basically object constructor >>> parameters. So if you were weiting C++ code and would have a >>> constructor that gets the size as argument, it would end up being >>> modeled as qdev property. >>> >>> If however actual functionality differs, thus you would in OO speech >>> create a subclass / child class, then you are better off creating a >>> new device struct. >>> >>> In this case, I'm not sure. They are different devices really, but >>> are close enough that the differences could be expressed through qdev >>> properties. >> I wasn't suggesting that they be different devices. I was suggesting >> that this isn't a property of the PCI controller, but rather of some >> other entity to which the PCI controller connects. So maybe a reference >> to the associated CCSR object would be a qdev parameter, but not the >> size of that CCSR. > For a reference to another object a QOM link<> property would be > preferred over a static qdev property. How does that work? Can we do RPC to other objects to access its CCSR and/or enumerate the CCSR size? Or maybe even receive its memory api block? Alex
Am 11.09.2012 14:27, schrieb Alexander Graf: > On 09/11/2012 02:23 PM, Andreas Färber wrote: >> For a reference to another object a QOM link<> property would be >> preferred over a static qdev property. > > How does that work? Can we do RPC to other objects to access its CCSR > and/or enumerate the CCSR size? Or maybe even receive its memory api block? What I had in mind was something like: typedef struct CCSRState { Object *parent; /* or DeviceState or whatever */ MemoryRegion *ccsr_region; } CCSRState; #define CCSR(obj) OBJECT_CHECK(CCSRState, (obj), TYPE_CCSR) MemoryRegion pci_ccsr_region; CCSRState *ccsr = CCSR(pointer_from_link_property_or_path); memory_region_init_alias(&pci_ccsr_region, "pci-ccsr", ccsr->ccsr_region, 0, memory_region_size(ccsr->ccsr_region)); Andreas
On 09/11/2012 06:33 AM, Alexander Graf wrote: > On 09/07/2012 08:58 PM, Scott Wood wrote: >> I wasn't suggesting that they be different devices. I was suggesting >> that this isn't a property of the PCI controller, but rather of some >> other entity to which the PCI controller connects. So maybe a reference >> to the associated CCSR object would be a qdev parameter, but not the >> size of that CCSR. > > The first common place of information we get is the machine description. > So here we can do: > > create_device(e500_ccsr, CCSR_SIZE); > create_device(e500_pci_host_controller, CCSR_SIZE); > > Obviously code-wise this would look quite different from above, as > object constructor parameters go through qdev properties. Keep in mind that in order to make MSIs work we need the BAR to actually be hooked up to CCSR, not just sized properly. If qdev properties are really stable API as you indicated recently, we want to get this right and not introduce a short-term hack. -Scott
On 11.09.2012, at 21:05, Scott Wood <scottwood@freescale.com> wrote: > On 09/11/2012 06:33 AM, Alexander Graf wrote: >> On 09/07/2012 08:58 PM, Scott Wood wrote: >>> I wasn't suggesting that they be different devices. I was suggesting >>> that this isn't a property of the PCI controller, but rather of some >>> other entity to which the PCI controller connects. So maybe a reference >>> to the associated CCSR object would be a qdev parameter, but not the >>> size of that CCSR. >> >> The first common place of information we get is the machine description. >> So here we can do: >> >> create_device(e500_ccsr, CCSR_SIZE); >> create_device(e500_pci_host_controller, CCSR_SIZE); >> >> Obviously code-wise this would look quite different from above, as >> object constructor parameters go through qdev properties. > > Keep in mind that in order to make MSIs work we need the BAR to actually > be hooked up to CCSR, not just sized properly. If qdev properties are > really stable API as you indicated recently, we want to get this right > and not introduce a short-term hack. Right. So I like Andreas' proposal. We pass in a reference to the ccsr object into our host controller object as parameter. That reference could maybe be a real pointer (if supported) or by-path. We can then use that to request the memory region that ccsr resides in. From that we can enumerate the size and also hook it up as BAR MMIO backing. Alex > > -Scott > >
diff --git a/hw/pci_host.h b/hw/pci_host.h index 359e38f..0ec0691 100644 --- a/hw/pci_host.h +++ b/hw/pci_host.h @@ -36,6 +36,7 @@ struct PCIHostState { MemoryRegion data_mem; MemoryRegion mmcfg; MemoryRegion *address_space; + MemoryRegion bar0; uint32_t config_reg; PCIBus *bus; }; diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c index 0f60b24..7e58a62 100644 --- a/hw/ppce500_pci.c +++ b/hw/ppce500_pci.c @@ -306,6 +306,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev) PCIHostState *h; PPCE500PCIState *s; PCIBus *b; + PCIDevice *pdev; int i; MemoryRegion *address_space_mem = get_system_memory(); MemoryRegion *address_space_io = get_system_io(); @@ -336,6 +337,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); + memory_region_init_io(&h->bar0, &pci_host_conf_be_ops, h, + "PCIHOST-bar0", 0x1000000); + pdev = pci_find_device(b, 0, 0); + pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &h->bar0); return 0; }
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. 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/pci_host.h | 1 + hw/ppce500_pci.c | 5 +++++ 2 files changed, 6 insertions(+), 0 deletions(-)