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

[Cédric Le Goater]

On 06/14/2018 08:44 AM, David Gibson wrote:
> On Wed, Jun 13, 2018 at 09:03:22AM +0200, Cédric Le Goater wrote:
>> 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);
> 
> Looks sensible up to this point.

yes. 

The common part only initializes the xscom bus. I wished we could create 
also the cores but, on the P9, we need the XIVE interrupt controller
to be realized before :/ 

> 
>>     Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
>>     ISABus *(*isa_create)(PnvChip *chip);
> 
> I'm a little more dubious about this - I would have thought your
> realize() hook could construct the right intc and isa, rather than
> going into generic code, then back out to the callback.

The 'intc_create' op creates the interrupt presenter. it is called from 
the powernv core realize routine and each chip has a different interrupt 
controller, and so a different presenter type. We cannot create the
presenter before and 'pass' it to the core object at realize time 
because the presenter links in the CPU state. It's a bit of a chicken
and egg problem, but I think it is good to have a presenter object
all well initialized before we use it. I am open to any suggestions
to get rid of this op. sPAPR has exactly the same need.

As for the isa_create op, this is because there is only one isa_bus 
per machine, but you might be right. We can probably create the isa
bus on all chips and link the machine to the one on chip0. It should 
save the op.


> But it's not a big deal.
> 
>> 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.
> 
> Nice.
> 
>> 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.
> 
> Ok.  Going to a data-driven approach for constructing things sounds
> like it might be a reasonable plan in its own right.  But I'd prefer
> not to conflate with other structural questions.
> 

OK. It can come later with XIVE, which needs 4 extra MMIO regions.

Thanks,

C.