Re: [PATCH v3 05/12] ppc/pnv: turn PnvPHB4 into a PnvPHB backend

From: Frederic Barrat <fbarrat@linux.ibm.com>
To: Daniel Henrique Barboza <danielhb413@gmail.com>, qemu-devel@nongnu.org
Cc: qemu-ppc@nongnu.org, clg@kaod.org
Subject: Re: [PATCH v3 05/12] ppc/pnv: turn PnvPHB4 into a PnvPHB backend
Date: Wed, 27 Jul 2022 19:41:34 +0200	[thread overview]
Message-ID: <fdb9248c-1b25-ec59-1532-a2ce093edf96@linux.ibm.com> (raw)
In-Reply-To: <20220624084921.399219-6-danielhb413@gmail.com>

On 24/06/2022 10:49, Daniel Henrique Barboza wrote:

> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 1df91971b8..b7273f386e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -672,7 +672,14 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, Monitor *mon)
>   static int pnv_chip_power9_pic_print_info_child(Object *child, void *opaque)
>   {
>       Monitor *mon = opaque;
> -    PnvPHB4 *phb4 = (PnvPHB4 *) object_dynamic_cast(child, TYPE_PNV_PHB4);
> +    PnvPHB *phb =  (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB);
> +    PnvPHB4 *phb4;
> +
> +    if (!phb) {
> +        return 0;
> +    }
> +
> +    phb4 = (PnvPHB4 *)phb->backend;
>   
>       if (phb4) {
>           pnv_phb4_pic_print_info(phb4, mon);

The full code in pnv_chip_power9_pic_print_info_child() looks like this:

     PnvPHB *phb =  (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB);
     PnvPHB4 *phb4;

     if (!phb) {
         return 0;
     }

     phb4 = (PnvPHB4 *)phb->backend;

     if (phb4) {
         pnv_phb4_pic_print_info(phb4, mon);
     }

Which is correct. However, if I want to nitpick, phb->backend is defined 
when the PnvPHB object is realized, so I don't think we can get here 
with the pointer being null, so we could remove the second if statement 
for readability. The reason I mention it is that we don't take that much 
care in the pnv_chip_power8_pic_print_info() function just above, so it 
looks a bit odd.

In any case:
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred

> @@ -2122,8 +2129,14 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>       PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
>       static const char compat[] = "qemu,powernv9\0ibm,powernv";
>   
> +    static GlobalProperty phb_compat[] = {
> +        { TYPE_PNV_PHB, "version", "4" },
> +    };
> +
>       mc->desc = "IBM PowerNV (Non-Virtualized) POWER9";
>       mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> +    compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat));
> +
>       xfc->match_nvt = pnv_match_nvt;
>   
>       mc->alias = "powernv";
> @@ -2140,8 +2153,13 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
>       XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc);
>       static const char compat[] = "qemu,powernv10\0ibm,powernv";
>   
> +    static GlobalProperty phb_compat[] = {
> +        { TYPE_PNV_PHB, "version", "5" },
> +    };
> +
>       mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
>       mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v2.0");
> +    compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat));
>   
>       pmc->compat = compat;
>       pmc->compat_size = sizeof(compat);
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 90843ac3a9..f22253358f 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -18,6 +18,7 @@
>   typedef struct PnvPhb4PecState PnvPhb4PecState;
>   typedef struct PnvPhb4PecStack PnvPhb4PecStack;
>   typedef struct PnvPHB4 PnvPHB4;
> +typedef struct PnvPHB PnvPHB;
>   typedef struct PnvChip PnvChip;
>   
>   /*
> @@ -78,7 +79,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB4, PNV_PHB4)
>   #define PCI_MMIO_TOTAL_SIZE        (0x1ull << 60)
>   
>   struct PnvPHB4 {
> -    PCIExpressHost parent_obj;
> +    DeviceState parent;
> +
> +    PnvPHB *phb_base;
>   
>       uint32_t chip_id;
>       uint32_t phb_id;