Re: [Qemu-ppc] [PATCH 2/2] [v3] target-ppc: Enhance CPU nodes of device tree to be PAPR compliant.

[Andreas Färber]

Am 08.08.2013 09:26, schrieb Prerna Saxena:
> 
> From: Prerna Saxena <address@hidden>
> Date: Thu, 8 Aug 2013 06:38:03 +0530
> Subject: [PATCH 2/2] Enhance CPU nodes of device tree to be PAPR compliant.
> 
> This is based on patch from Andreas which enables the default CPU with KVM
> to show up as "-cpu <type>", such as "address@hidden"
> 
> While this is definitely, more descriptive, PAPR mandates the device tree CPU
> node names to be of the form : "PowerPC,<name>" where <name> should not have
> underscores.
> Hence replacing the CPU model (which has underscores) with CPU alias.
> 
> With this patch, the CPU nodes of device tree show up as :
> /proc/device-tree/cpus/PowerPC,address@hidden/...
> /proc/device-tree/cpus/PowerPC,address@hidden/...
> 
> Signed-off-by: Prerna Saxena <address@hidden>

Not yet happy...

> ---
>  hw/ppc/spapr.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 59e2fea..8efd84e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -43,6 +43,7 @@
>  #include "hw/pci-host/spapr.h"
>  #include "hw/ppc/xics.h"
>  #include "hw/pci/msi.h"
> +#include "cpu-models.h"
>  
>  #include "hw/pci/pci.h"
>  
> @@ -80,6 +81,8 @@
>  
>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
>  
> +#define PPC_DEVTREE_STR         "PowerPC,"
> +
>  sPAPREnvironment *spapr;
>  
>  int spapr_allocate_irq(int hint, bool lsi)
> @@ -322,9 +325,16 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>      _FDT((fdt_property_cell(fdt, "#address-cells", 0x1)));
>      _FDT((fdt_property_cell(fdt, "#size-cells", 0x0)));
>  
> -    modelname = g_strdup(cpu_model);
> +    /*
> +     * PAPR convention mandates that
> +     * Device tree nodes must be named as:
> +     * PowerPC,address@hidden
> +     * Also, CPU-NAME must not have underscores.(hence use of CPU-ALIAS)
> +     */
> +
> +    modelname = g_strdup_printf(PPC_DEVTREE_STR "%s", cpu_model);
>  
> -    for (i = 0; i < strlen(modelname); i++) {
> +    for (i = strlen(PPC_DEVTREE_STR); i < strlen(modelname); i++) {
>          modelname[i] = toupper(modelname[i]);
>      }
>  

One of your colleagues had brought up that "PowerPC," prefix were not
mandatory - is it *required* by the PAPR spec now, or is it just that
the IBM CPUs used with PAPR happen to have such a name?

> @@ -1315,6 +1325,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>  
>          cpu_model = g_strndup(parent_name,
>              strlen(parent_name) - strlen("-" TYPE_POWERPC_CPU));
> +
> +        for (i = 0; ppc_cpu_aliases[i].model != NULL; i++) {
> +            if (strcmp(ppc_cpu_aliases[i].model, cpu_model) == 0) {
> +                g_free(cpu_model);
> +                cpu_model = g_strndup(ppc_cpu_aliases[i].alias,
> +                                strlen(ppc_cpu_aliases[i].alias));
> +            }
> +        }
>      }
>  
>      /* Prepare the device tree */

This is still fixing up the name in the wrong place: -cpu POWER7_v2.3
will not get fixed, only -cpu host or KVM's default.

The solution I had discussed with Alex is the following: When devices
need to expose their name to firmware in a special way, we have the
DeviceClass::fw_name field. All we have to do is assign it and use it
instead of cpu_model if non-NULL, just like we assign DeviceClass::desc.
The way to do it would be to extend the family of POWERPC_DEF* macros to
specify the additional field on the relevant CPU models.

Therefore my above question: Would it be sufficient to explicitly name
POWER7_v2.3 PowerPC,POWER7 etc. and to drop the upper-casing?
Or would we also need to name a CPU such as MPC8572E (random Freescale
CPU where I don't know the expected fw_name and that is unlikely to
occur/work in sPAPR) "PowerPC,MPC8572E" if someone specified it with
-cpu MPC8572E?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg