Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3157 bytes --]

On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote:
> On 06/12/2018 07:58 AM, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
> >> On 06/06/2018 08:39 AM, David Gibson wrote:
> >>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
> >>>> Based on previous work done in skiboot, the physical mapping array
> >>>> helps in calculating the MMIO ranges of each controller depending on
> >>>> the chip id and the chip type. This is will be particularly useful for
> >>>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
> >>>>
> >>>> A link on the chip is now necessary to calculate MMIO BARs and
> >>>> sizes. This is why such a link is introduced in the PSIHB model.
> >>>
> >>> I think this message needs some work.  This says what it's for, but
> >>> what actually *is* this array, and how does it work?
> >>
> >> OK. It is relatively simple: each controller has an entry defining its 
> >> MMIO range. 
> >>
> >>> The outside-core differences between POWER8 and POWER9 are substantial
> >>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
> >>> different machine types (sharing some routines, of course).
> >>
> >> yes and no. I have survived using a common PnvChip framework but
> >> it is true that I had to add P9 classes for each: LPC, PSI, OCC 
> >> They are very similar but not enough. P9 uses much more MMIOs than 
> >> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. 
> > 
> > Well, it's certainly *possible* to use the same machine type, I'm just
> > not convinced it's a good idea.  It seems kind of dodgy to me that so
> > many peripherals on the system change as a side-effect of setting the
> > cpu.  Compare to how x86 works where cpu really does change the CPU,
> > plugging it into the same virtual "chipset".  Different chipsets *are*
> > different machine types there (pc vs. q35).
> 
> OK, I agree, and we can use a set of common routines to instantiate the 
> different chipset models. 
> 
> So we would have a common pnv_init() routine to initialize the different 
> 'powernv8' and 'powernv9' machines and the PnvChip typename would be a 
> machine class attribute ?

Well.. that's one option.  Usually for these things, it works out
better to instead of parameterizing big high-level routines like
pnv_init(), you have separate versions of those calling a combination
of case-specific and common routines as necessary.

Mostly it just comes down to what is simplest to implement for you, though.

> Nevertheless we would still need to introduce "a physical mapping array 
> describing MMIO ranges" but we can start by differentiating the chipsets 
> first.

Well, maybe.  I'm wondering if you can more easily encapsulate the
information in that array in a top-level init routine, that calls
common helpers with different addresses / device types / whatever.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]