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

From: "Cédric Le Goater" <clg@kaod.org>
To: Frederic Barrat <fbarrat@linux.ibm.com>, <qemu-ppc@nongnu.org>,
	<qemu-devel@nongnu.org>
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>,
	Greg Kurz <groug@kaod.org>
Subject: Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices
Date: Tue, 7 Dec 2021 11:45:48 +0100	[thread overview]
Message-ID: <fe4c3744-91b1-bd5b-2a04-e2782c46fc09@kaod.org> (raw)
In-Reply-To: <453c5cc3-5ac5-c6ff-fa7f-5ccb8492e314@linux.ibm.com>

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 ?

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

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.