Re: [Qemu-devel] [PATCH v2 2/4] spapr: Use DeviceClass::fw_name for device tree CPU node

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 08/16/2013 08:35 AM, Andreas Färber wrote:
> Instead of relying on cpu_model, obtain the device tree node label
> per CPU. Use DeviceClass::fw_name when available. This implicitly
> resolves HOST@0 node labels for those CPUs through inheritance.
> 
> Whenever DeviceClass::fw_name is not available, derive it from the CPU's
> type name and fill it in for that class with a "PowerPC," prefix for
> PAPR compliance.


I'd rather use the family's @desc instead of CPU class name, would be
simpler and we would not have nodes like "PowerPC,POWER7-family@0" (this is
what I get when comment out dc->fw_name for power7 with my PVR patch, just
to test).

Either way, in what case do you expect that code to work at all? power7,
7+, 8 have fw_name field initialized, what else is really supported for
spapr and requires this workaround?



> As a consequence, spapr_fixup_cpu_dt() can operate on each CPU's fw_name,
> obsoleting sPAPREnvironment::cpu_model, and spapr_create_fdt_skel() can
> drop its cpu_model argument.
> 
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/ppc/spapr.c         | 36 ++++++++++++++++--------------------
>  include/hw/ppc/spapr.h |  1 -
>  2 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 16bfab9..6d984dc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -165,9 +165,8 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>      int smt = kvmppc_smt_threads();
>      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
>  
> -    assert(spapr->cpu_model);
> -
>      for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
> +        DeviceClass *dc = DEVICE_GET_CLASS(cpu);
>          uint32_t associativity[] = {cpu_to_be32(0x5),
>                                      cpu_to_be32(0x0),
>                                      cpu_to_be32(0x0),
> @@ -179,7 +178,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
>              continue;
>          }
>  
> -        snprintf(cpu_model, 32, "/cpus/%s@%x", spapr->cpu_model,
> +        snprintf(cpu_model, 32, "/cpus/%s@%x", dc->fw_name,
>                   cpu->cpu_index);
>  
>          offset = fdt_path_offset(fdt, cpu_model);
> @@ -249,8 +248,7 @@ static size_t create_page_sizes_prop(CPUPPCState *env, uint32_t *prop,
>      } while (0)
>  
>  
> -static void *spapr_create_fdt_skel(const char *cpu_model,
> -                                   hwaddr initrd_base,
> +static void *spapr_create_fdt_skel(hwaddr initrd_base,
>                                     hwaddr initrd_size,
>                                     hwaddr kernel_size,
>                                     const char *boot_device,
> @@ -266,7 +264,6 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>      char qemu_hypertas_prop[] = "hcall-memop1";
>      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
>      uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(smp_cpus)};
> -    char *modelname;
>      int i, smt = kvmppc_smt_threads();
>      unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
>  
> @@ -322,18 +319,10 @@ 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);
> -
> -    for (i = 0; i < strlen(modelname); i++) {
> -        modelname[i] = toupper(modelname[i]);
> -    }
> -
> -    /* This is needed during FDT finalization */
> -    spapr->cpu_model = g_strdup(modelname);
> -
>      for (cs = first_cpu; cs != NULL; cs = cs->next_cpu) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>          CPUPPCState *env = &cpu->env;
> +        DeviceClass *dc = DEVICE_GET_CLASS(cs);
>          PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
>          int index = cs->cpu_index;
>          uint32_t servers_prop[smp_threads];
> @@ -350,7 +339,17 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>              continue;
>          }
>  
> -        nodename = g_strdup_printf("%s@%x", modelname, index);
> +        if (dc->fw_name == NULL) {
> +            ObjectClass *oc = OBJECT_CLASS(pcc);
> +            const char *typename;
> +
> +            typename = object_class_get_name(oc);
> +            nodename = g_strndup(typename,
> +                                 strlen(typename) - strlen("-" TYPE_POWERPC_CPU));
> +            dc->fw_name = g_strdup_printf("PowerPC,%s", nodename);


A bit strange to malloc a string and never free it :)


> +            g_free(nodename);
> +        }
> +        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
>  
>          _FDT((fdt_begin_node(fdt, nodename)));
>  
> @@ -430,8 +429,6 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>          _FDT((fdt_end_node(fdt)));
>      }
>  
> -    g_free(modelname);
> -
>      _FDT((fdt_end_node(fdt)));
>  
>      /* RTAS */
> @@ -1308,8 +1305,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>                           &savevm_htab_handlers, spapr);
>  
>      /* Prepare the device tree */
> -    spapr->fdt_skel = spapr_create_fdt_skel(cpu_model,
> -                                            initrd_base, initrd_size,
> +    spapr->fdt_skel = spapr_create_fdt_skel(initrd_base, initrd_size,
>                                              kernel_size,
>                                              boot_device, kernel_cmdline,
>                                              spapr->epow_irq);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 9fc1972..b4a7656 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -27,7 +27,6 @@ typedef struct sPAPREnvironment {
>      target_ulong entry_point;
>      uint32_t next_irq;
>      uint64_t rtc_offset;
> -    char *cpu_model;
>      bool has_graphics;
>  
>      uint32_t epow_irq;
> 


-- 
Alexey