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

[Miles Glenn]

On Tue, 2023-11-21 at 08:29 +0100, Cédric Le Goater wrote:
> On 11/21/23 02:33, Nicholas Piggin wrote:
> > On Tue Nov 21, 2023 at 9:51 AM AEST, Glenn Miles wrote:
> > > Create a new powernv machine type, powernv10-rainier, that
> > > will contain rainier-specific devices.
> > 
> > Is the plan to have a base powernv10 common to all and then
> > powernv10-rainier looks like a Rainier? Or would powernv10
> > just be a rainier?
> > 
> > It's fine to structure code this way, I'm just wondering about
> > the machine types available to user. Is a base powernv10 machine
> > useful to run?
> 
> There are multiple P10 boards defined in Linux :
> 
>    aspeed-bmc-ibm-bonnell.dts
>    aspeed-bmc-ibm-everest.dts
>    aspeed-bmc-ibm-rainier-1s4u.dts
>    aspeed-bmc-ibm-rainier-4u.dts
>    aspeed-bmc-ibm-rainier.dts
> 
> and we could model the machines above with a fixed number of sockets.
> The "powernv10" would be the generic system that can be customized
> at will on the command line, even I2C devices. There is also the
> P10 Denali which is FSP based. This QEMU machine would certainly be
> very different. I thought of doing the same for P9 with a -zaius
> and include NPU2 models for it. I lacked time and the interest was
> small at the time of OpenPOWER.
> 
> Anyhow, adding a new machine makes sense and it prepares ground for
> possible new ones. I am OK with or without. As primary users, you are
> the ones that can tell if there will be a second machine.
>   
> Thanks,
> 
> C.
> 

I am not sure what the powernv10 machine would be used for.  The
only reason I kept it was because I didn't want to break anyone out
there that might be using it.

My preference would have been to just make powernv10-rainier an
alias of the powernv10 machine, but only one alias name per machine
is supported and there is already a plan to make "powernv" an
alias for the powernv10 machine.

Thanks,

Glenn

> 
> > > 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";
> > >       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";
> > > +
> > > +}
> > > +
> > > +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,
> > > +        .class_init    = pnv_machine_p10_rainier_class_init,
> > > +        .interfaces = (InterfaceInfo[]) {
> > > +            { TYPE_XIVE_FABRIC },
> > > +            { },
> > > +        },
> > > +    },
> > >       {
> > >           .name          = MACHINE_TYPE_NAME("powernv10"),
> > >           .parent        = TYPE_PNV_MACHINE,