Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type

[Miles Glenn]

On Tue, 2023-11-21 at 07:46 +0100, Cédric Le Goater wrote:
> On 11/21/23 00:51, Glenn Miles wrote:
> > Create a new powernv machine type, powernv10-rainier, that
> > will contain rainier-specific devices.
> > 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> >   hw/ppc/pnv.c | 29 +++++++++++++++++++++++++++--
> >   1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 9c29727337..3481a1220e 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -2249,7 +2249,7 @@ static void
> > pnv_machine_power9_class_init(ObjectClass *oc, void *data)
> >       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
> >   }
> >   
> > -static void pnv_machine_power10_class_init(ObjectClass *oc, void
> > *data)
> > +static void pnv_machine_p10_common_class_init(ObjectClass *oc,
> > void *data)
> >   {
> >       MachineClass *mc = MACHINE_CLASS(oc);
> >       PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> > @@ -2261,7 +2261,6 @@ static void
> > pnv_machine_power10_class_init(ObjectClass *oc, void *data)
> >           { TYPE_PNV_PHB_ROOT_PORT, "version", "5" },
> >       };
> >   
> > -    mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
> 
> I would keep this description because the "powernv10" machine still
> can
> be used, without I2C devices. Unless we don't want to ?
> 

I'm not sure about the usefulness of the powernv10 machine, but the
description still exists in the pnv_machine_power10_class_init function
(and is unchanged).  The pnv_machine_p10_common_class_init function was
created to hold initialization of things that are common between all
P10 machines, and is called by all power10 based machines (powernv10
and powernv10-rainier so far).

> >       mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v2.0");
> >       compat_props_add(mc->compat_props, phb_compat,
> > G_N_ELEMENTS(phb_compat));
> >   
> > @@ -2274,6 +2273,23 @@ static void
> > pnv_machine_power10_class_init(ObjectClass *oc, void *data)
> >       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB);
> >   }
> >   
> > +static void pnv_machine_power10_class_init(ObjectClass *oc, void
> > *data)
> > +{
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> > +
> > +    pnv_machine_p10_common_class_init(oc, data);
> > +    mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
> > +
> 
> Superfluous white line.
> 
Removed in v5.

> > +}
> > +
> > +static void pnv_machine_p10_rainier_class_init(ObjectClass *oc,
> > void *data)
> > +{
> > +    MachineClass *mc = MACHINE_CLASS(oc);
> > +
> > +    pnv_machine_p10_common_class_init(oc, data);
> > +    mc->desc = "IBM PowerNV (Non-Virtualized) POWER10 rainier";
> > +}
> > +
> >   static bool pnv_machine_get_hb(Object *obj, Error **errp)
> >   {
> >       PnvMachineState *pnv = PNV_MACHINE(obj);
> > @@ -2379,6 +2395,15 @@ static void
> > pnv_machine_class_init(ObjectClass *oc, void *data)
> >       }
> >   
> >   static const TypeInfo types[] = {
> > +    {
> > +        .name          = MACHINE_TYPE_NAME("powernv10-rainier"),
> > +        .parent        = TYPE_PNV_MACHINE,
> 
> Could the parent be :
> 
>              .parent          = MACHINE_TYPE_NAME("powernv10"),
> 
> This should avoid the definition of the .interfaces below.
> 
> Thanks,
> 
> C.
> 

Changed as suggested in v5.

Thanks,

Glenn

> 
> 
> > +        .class_init    = pnv_machine_p10_rainier_class_init,
> > +        .interfaces = (InterfaceInfo[]) {
> > +            { TYPE_XIVE_FABRIC },
> > +            { },
> > +        },
> > +    },
> >       {
> >           .name          = MACHINE_TYPE_NAME("powernv10"),
> >           .parent        = TYPE_PNV_MACHINE,