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

From: Frederic Barrat <fbarrat@linux.ibm.com>
To: "Cédric Le Goater" <clg@kaod.org>,
	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:00:24 +0100	[thread overview]
Message-ID: <453c5cc3-5ac5-c6ff-fa7f-5ccb8492e314@linux.ibm.com> (raw)
In-Reply-To: <20211202144235.1276352-8-clg@kaod.org>

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.

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

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.
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.
That's outside of the scope of this series though. So maybe for a future 
patch? Who knows, I might volunteer...

   Fred

>   include/hw/ppc/pnv.h |  2 ++
>   hw/ppc/pnv.c         | 20 +++++++++-----------
>   2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 247379ef1f88..f2c238062f4a 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -53,6 +53,7 @@ struct PnvChip {
>       PnvCore      **cores;
>   
>       uint32_t     num_phbs;
> +    uint32_t     num_pecs;
>   
>       MemoryRegion xscom_mmio;
>       MemoryRegion xscom;
> @@ -136,6 +137,7 @@ struct PnvChipClass {
>       uint64_t     chip_cfam_id;
>       uint64_t     cores_mask;
>       uint32_t     num_phbs;
> +    uint32_t     num_pecs;
>   
>       DeviceRealize parent_realize;
>   
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 45d8ecbf2bf7..185464a1d443 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -658,7 +658,7 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip, Monitor *mon)
>       pnv_xive_pic_print_info(&chip9->xive, mon);
>       pnv_psi_pic_print_info(&chip9->psi, mon);
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
>           for (j = 0; j < pec->num_stacks; j++) {
>               pnv_phb4_pic_print_info(&pec->stacks[j].phb, mon);
> @@ -1330,15 +1330,14 @@ static void pnv_chip_power9_instance_init(Object *obj)
>   
>       object_initialize_child(obj, "homer", &chip9->homer, TYPE_PNV9_HOMER);
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    if (defaults_enabled()) {
> +        chip->num_pecs = pcc->num_pecs;
> +    }
> +
> +    for (i = 0; i < chip->num_pecs; i++) {
>           object_initialize_child(obj, "pec[*]", &chip9->pecs[i],
>                                   TYPE_PNV_PHB4_PEC);
>       }
> -
> -    /*
> -     * Number of PHBs is the chip default
> -     */
> -    chip->num_phbs = pcc->num_phbs;
>   }
>   
>   static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
> @@ -1374,7 +1373,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>       int i, j;
>       int phb_id = 0;
>   
> -    for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
> +    for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
>           PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>           uint32_t pec_nest_base;
> @@ -1402,8 +1401,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
>   
> -        for (j = 0; j < pec->num_stacks && phb_id < chip->num_phbs;
> -             j++, phb_id++) {
> +        for (j = 0; j < pec->num_stacks; j++, phb_id++) {
>               PnvPhb4PecStack *stack = &pec->stacks[j];
>               Object *obj = OBJECT(&stack->phb);
>   
> @@ -1559,7 +1557,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>       k->xscom_core_base = pnv_chip_power9_xscom_core_base;
>       k->xscom_pcba = pnv_chip_power9_xscom_pcba;
>       dc->desc = "PowerNV Chip POWER9";
> -    k->num_phbs = 6;
> +    k->num_pecs = PNV9_CHIP_MAX_PEC;
>   
>       device_class_set_parent_realize(dc, pnv_chip_power9_realize,
>                                       &k->parent_realize);
> 