Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices

[Frederic Barrat <fbarrat@linux.ibm.com>]

On 07/12/2021 11:45, Cédric Le Goater wrote:
> On 12/7/21 11:00, Frederic Barrat wrote:
>>
>>
>> On 02/12/2021 15:42, Cédric Le Goater wrote:
>>> POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
>>> each PEC can have several PHBs :
>>>
>>>    * PEC0 provides 1 PHB  (PHB0)
>>>    * PEC1 provides 2 PHBs (PHB1 and PHB2)
>>>    * PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
>>>
>>> A num_pecs class attribute represents better the logic units of the
>>> POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
>>> This will ease adding support for user created devices.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>
>> With this patch, chip->num_phbs is only defined and used on P8. We may 
>> want to add a comment to make it clear.
> 
> Yes.
> 
> With the latest changes, I think we can now move num_phbs under PnvChip8
> and num_pecs under PnvChip9 since they are only used in these routines :
> 
> P8:
>      static void pnv_chip_power8_instance_init(Object *obj)
>              chip->num_phbs = pcc->num_phbs;
>          for (i = 0; i < chip->num_phbs; i++) {
> 
>      static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>          for (i = 0; i < chip->num_phbs; i++) {
> P9:
>      static void pnv_chip_power9_instance_init(Object *obj)
>              chip->num_pecs = pcc->num_pecs;
>          for (i = 0; i < chip->num_pecs; i++) {
> 
>      static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>          for (i = 0; i < chip->num_pecs; i++) {
> 
>> As I review this series, something is bugging me though: the 
>> difference of handling between P8 and P9.
>> On P9, we seem to have a more logical hiearachy:
>> phb <- PCI controller (PEC) <- chip
> 
> Yes. It's cleaner than P8 in terms of logic. P8 initial support was
> done hastily for skiboot bringup in 2014.
> 
>> With P8, we don't have an explicit PEC, but we have a PBCQ object, 
>> which is somewhat similar. The hierarchy seems also more convoluted.
> 
> But we don't have stacks on P8. Do we ?


Stacks were introduced on P9 because all the lanes handled by a PEC 
could be grouped differently, each group being called a stack. And each 
stack is associated to a PHB.
On P8, there's no such split, so the doc didn't mention stacks. But each 
PEC handles exactly one PHB. So we could still keep the same abstractions.
On all chips, a PEC would really be equal to a pbcq interface to the 
power bus. The pbcq is servicing one (on P8) or more (on P9/P10) PHBs.



>> I don't see why it's treated differently. It seems both chips could be 
>> treated the same, which would make the code easier to follow.
> 
> I agree. Daniel certainly would also :)
> 
>> That's outside of the scope of this series though. 
> 
> Well, this patchset enables libvirt support for the PowerNV machines.
> Once this is pushed, we need to keep the API, the object model names
> being part of it.
> 
> 7.0 is a good time for a change, really. After that, we won't be able
> to change the QOM hierarchy that much.
> 
>> So maybe for a future patch? Who knows, I might volunteer...
> 
> You would introduce a phb3-pec on top of the phb3s ?


Or rename pnv_phb3_pbcq.c to pnv_phb3_pec.c and starts from there. 
Conceptually, the TYPE_PNV_PBCQ and TYPE_PNV_PHB4_PEC_STACK objects seem 
close. But that's easy to say in an email...

   Fred


> Let me send a v2 first and may be we could rework the object hierarchy
> in the 7.0 time frame. We don't have to merge this ASAP.
> 
> Thanks,
> 
> C.