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

Glenn Miles posted 11 patches 1 year ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Glenn Miles <milesg@linux.vnet.ibm.com>, "Cédric Le Goater" <clg@kaod.org>, Nicholas Piggin <npiggin@gmail.com>, "Frédéric Barrat" <fbarrat@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Glenn Miles 1 year ago
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";
     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,
-- 
2.31.1
Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Cédric Le Goater 1 year ago
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 ?

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

> +}
> +
> +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.



> +        .class_init    = pnv_machine_p10_rainier_class_init,
> +        .interfaces = (InterfaceInfo[]) {
> +            { TYPE_XIVE_FABRIC },
> +            { },
> +        },
> +    },
>       {
>           .name          = MACHINE_TYPE_NAME("powernv10"),
>           .parent        = TYPE_PNV_MACHINE,
Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Miles Glenn 1 year ago
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,


Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Nicholas Piggin 1 year ago
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?

Thanks,
Nick

>
> 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,
Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Cédric Le Goater 1 year ago
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.


> 
>>
>> 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,
>
Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Nicholas Piggin 1 year ago
On Tue Nov 21, 2023 at 5:29 PM AEST, 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.

If a bare qemu machine could be useful, I don't have a problem with
it. I'm more thinking of what an average OPAL/PowerNV Linux user
developer would want, they (I) would probably want to use powernv,
powernv9, or powernv10, and just get a reasonable "realistic" machine.

The bare system could be powernv10-generic or powernv10-minimal for
those who know what they're doing.

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

Yeah we will want to add other machines at some point, I think
this does make sense, my only real concern is what we call them.

Thanks,
Nick
Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Miles Glenn 1 year ago
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,


Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Cédric Le Goater 1 year ago
On 11/21/23 17:36, Miles Glenn wrote:
> 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.
(previous email sent to fast)

You would need to go through the deprecation process [1] if you want
to remove the machine. I suggest keeping it for now since it is two
lines of type definition.

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

yes. It might be time now for PowerNV and pSeries to update the
default processor. Let's address that in the QEMU 9.0 cycle.

Thanks,

C.

[1] https://qemu.readthedocs.io/en/v8.1.0/about/deprecated.html


Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Miles Glenn 1 year ago
On Tue, 2023-11-21 at 19:26 +0100, Cédric Le Goater wrote:
> On 11/21/23 17:36, Miles Glenn wrote:
> > 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.
> (previous email sent to fast)
> 
> You would need to go through the deprecation process [1] if you want
> to remove the machine. I suggest keeping it for now since it is two
> lines of type definition.
> 

Yes, I agree.

> > 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.
> 
> yes. It might be time now for PowerNV and pSeries to update the
> default processor. Let's address that in the QEMU 9.0 cycle.
> 
> Thanks,
> 
> C.
> 
> [1] https://qemu.readthedocs.io/en/v8.1.0/about/deprecated.html
> 

Sounds good, thanks!

Glenn


Re: [PATCH v4 03/11] ppc/pnv: New powernv10-rainier machine type
Posted by Cédric Le Goater 1 year ago
On 11/21/23 17:36, Miles Glenn wrote:
> 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. 

This would be an empty board with only POWER10 processors, no
platform devices.


Thanks,

C.

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