Re: [Qemu-ppc] [PATCH: RFC] Adding BAR0 for e500 PCI controller

[Alexander Graf]

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 <address@hidden> 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:
> > > > > address@hidden; address@hidden; address@hidden; 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