[prev in list] [next in list] [prev in thread] [next in thread] 

List:       qemu-ppc
Subject:    Re: [PATCH v3 05/12] ppc/pnv: turn PnvPHB4 into a PnvPHB backend
From:       Daniel Henrique Barboza <danielhb413 () gmail ! com>
Date:       2022-07-28 13:09:05
Message-ID: 79ffedd9-0bf5-4ed2-dc0e-50d6d41c554c () gmail ! com
[Download RAW message or body]



On 7/27/22 14:41, Frederic Barrat wrote:
> 
> 
> 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.

Good point. I changed it to look like this:


static int pnv_chip_power9_pic_print_info_child(Object *child, void *opaque)
{
     Monitor *mon = opaque;
     PnvPHB *phb =  (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB);

     if (!phb) {
         return 0;
     }

     pnv_phb4_pic_print_info(PNV_PHB4(phb->backend), mon);

     return 0;
}

phb->backend being either NULL or not a PHB4 object is serious enough to assert
out, so the PNV_PHB4() macro seems justified.


Thanks,


Daniel

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


[prev in list] [next in list] [prev in thread] [next in thread]