On 09/07/2012 03:08 AM, Alexander Graf wrote: > > > On 07.09.2012, at 01:15, Scott Wood <scottw...@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-...@nongnu.org; ag...@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
