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

[Cédric Le Goater]

On 06/13/2018 02:47 AM, David Gibson wrote:
> 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.

I am introducing a powernv8 machine, the machine init routine is still
generic and did not change much. But I have deepen the PnvChip class
inheritance hierarchy with an intermediate class taking care of the
Chip sub controllers, which gives us something like :

  pnv_init()
    . skiboot
    . kernel
    . initrd
    . chips creation
    . platform bus/device :
       isa bus
       pci layout
       bmc handling.

  p8 chip hierarchy:
 
   power8_v2.0-pnv-chip (gives the cpu type)
   pnv8-chip   : creates the devices only       
   pnv-chip    : creates xscom and the cores 

The powervn9 machine has this hierarchy :

   power9_v2.0-pnv-chip
   pnv9-chip
   pnv-chip

I had to introduce these new PnvChipClass ops : 

    void (*realize)(PnvChip *chip, Error **errp);
    Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
    ISABus *(*isa_create)(PnvChip *chip);

Overall, it's looking fine and it should remove most of these tests :

         pnv_chip_is_power9(chip)

If not, it means we are missing a PnvChipClass ops anyway.

I will send a patchset this week, the final one will shuffle quite a
bit of code and the resulting diff will be a bit fuzy. You will have
to trust me on this one.

 
>> 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.

Hmm, I think I understand but could you give me a prototype example. 
Please. To make sure.

I would like to keep the array somewhere because, in a quick look, 
it gives you an overview of the POWER Processor address space. 

Thanks,

C.