[Qemu-devel] [PATCH v2 0/4] target-ppc: Tidy sPAPR device tree CPU nodes

[Qemu-devel] [PATCH v2 0/4] target-ppc: Tidy sPAPR device tree CPU nodes

[Andreas Färber]

Hello Prerna and Alex,

This series cleans up the fdt CPU nodes for -M pseries as attempted by Prerna.

v2 reuses DeviceClass::fw_name for name storage and cleans up sPAPR code to
not rely on machine-global cpu_model or sPAPREnvironment::cpu_model.
Underscores are avoided by using fw_name that doesn't have them for known CPUs
or by stripping them explicitly.

Regards,
Andreas

preview -> v2:
* Set fw_name at family level rather than model level, saving macro extensions.
* Appended patch stripping underscores (Prerna).

v1 / Prerna's v3 -> preview:
* Instead of overwriting cpu_model with parent's type name for -cpu host,
  set fw_name for some models and read type / parent's types otherwise.
* Don't pass cpu_model to functions, determine name per CPU iterated.
* Avoid walking alias list by taking fw_name or model part of type name.

Cc: Prerna Saxena <prerna@linux.vnet.ibm.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: qemu-ppc@nongnu.org

Andreas Färber (4):
  target-ppc: Fill in OpenFirmware names for some PowerPCCPU families
  spapr: Use DeviceClass::fw_name for device tree CPU node
  spapr: Improve device tree CPU node for -cpu host with unknown OF name
  spapr: Suppress underscores in device tree CPU node

 hw/ppc/spapr.c              | 53 +++++++++++++++++++++++++++------------------
 include/hw/ppc/spapr.h      |  1 -
 target-ppc/translate_init.c |  3 +++
 3 files changed, 35 insertions(+), 22 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 1/4] target-ppc: Fill in OpenFirmware names for some PowerPCCPU families

[Andreas Färber]

Set the expected values for POWER7, POWER7+, POWER8 and POWER5+.
Note that POWER5+ and POWER7+ are intentionally lacking the '+', so the
lack of a POWER7P family constitutes no problem.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-ppc/translate_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 13b290c..28ca447 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7102,6 +7102,7 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
 
+    dc->fw_name = "PowerPC,POWER5";
     dc->desc = "POWER5+";
     pcc->init_proc = init_proc_power5plus;
     pcc->check_pow = check_pow_970FX;
@@ -7212,6 +7213,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
 
+    dc->fw_name = "PowerPC,POWER7";
     dc->desc = "POWER7";
     pcc->init_proc = init_proc_POWER7;
     pcc->check_pow = check_pow_nocheck;
@@ -7246,6 +7248,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
 
+    dc->fw_name = "PowerPC,POWER8";
     dc->desc = "POWER8";
     pcc->init_proc = init_proc_POWER7;
     pcc->check_pow = check_pow_nocheck;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 1/4] target-ppc: Fill in OpenFirmware names for some PowerPCCPU families

[Alexey Kardashevskiy]

On 08/16/2013 08:35 AM, Andreas Färber wrote:
> Set the expected values for POWER7, POWER7+, POWER8 and POWER5+.
> Note that POWER5+ and POWER7+ are intentionally lacking the '+', so the
> lack of a POWER7P family constitutes no problem.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>


Out of curiosity - is anything going to happen to this series? Is it
awaiting for someone's review? Just asking as it is quite old and nobody
seems to care :)


-- 
Alexey

Re: [Qemu-devel] [PATCH v2 1/4] target-ppc: Fill in OpenFirmware names for some PowerPCCPU families

[Alexey Kardashevskiy]

On 09/10/2013 02:15 PM, Alexey Kardashevskiy wrote:
> On 08/16/2013 08:35 AM, Andreas Färber wrote:
>> Set the expected values for POWER7, POWER7+, POWER8 and POWER5+.
>> Note that POWER5+ and POWER7+ are intentionally lacking the '+', so the
>> lack of a POWER7P family constitutes no problem.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> 
> Out of curiosity - is anything going to happen to this series? Is it
> awaiting for someone's review? Just asking as it is quite old and nobody
> seems to care :)

Ping, anyone? Not sure if any of my mails even reaches maillists :-/


-- 
Alexey

Re: [Qemu-devel] [PATCH v2 1/4] target-ppc: Fill in OpenFirmware names for some PowerPCCPU families

[Alexey Kardashevskiy]

On 09/17/2013 12:16 AM, Alexey Kardashevskiy wrote:
> On 09/10/2013 02:15 PM, Alexey Kardashevskiy wrote:
>> On 08/16/2013 08:35 AM, Andreas Färber wrote:
>>> Set the expected values for POWER7, POWER7+, POWER8 and POWER5+.
>>> Note that POWER5+ and POWER7+ are intentionally lacking the '+', so the
>>> lack of a POWER7P family constitutes no problem.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>
>>
>> Out of curiosity - is anything going to happen to this series? Is it
>> awaiting for someone's review? Just asking as it is quite old and nobody
>> seems to care :)
> 
> Ping, anyone? Not sure if any of my mails even reaches maillists :-/

Ping?

It conflicts with "[PATCH] pseries: Fix loading of little endian kernels"
posted today. It would be great to have either this series or the new patch
upstream...



-- 
Alexey

Re: [Qemu-devel] [PATCH v2 1/4] target-ppc: Fill in OpenFirmware names for some PowerPCCPU families

[Alexander Graf]

On 09/25/2013 11:01 AM, Alexey Kardashevskiy wrote:
> On 09/17/2013 12:16 AM, Alexey Kardashevskiy wrote:
>> On 09/10/2013 02:15 PM, Alexey Kardashevskiy wrote:
>>> On 08/16/2013 08:35 AM, Andreas Färber wrote:
>>>> Set the expected values for POWER7, POWER7+, POWER8 and POWER5+.
>>>> Note that POWER5+ and POWER7+ are intentionally lacking the '+', so the
>>>> lack of a POWER7P family constitutes no problem.
>>>>
>>>> Signed-off-by: Andreas Färber<afaerber@suse.de>
>>>
>>> Out of curiosity - is anything going to happen to this series? Is it
>>> awaiting for someone's review? Just asking as it is quite old and nobody
>>> seems to care :)
>> Ping, anyone? Not sure if any of my mails even reaches maillists :-/
> Ping?
>
> It conflicts with "[PATCH] pseries: Fix loading of little endian kernels"
> posted today. It would be great to have either this series or the new patch
> upstream...

Andreas is going to rework it, yes :).


Alex

Re: [Qemu-devel] [PATCH v2 1/4] target-ppc: Fill in OpenFirmware names for some PowerPCCPU families

[Alexey Kardashevskiy]

On 10/01/2013 03:55 AM, Alexander Graf wrote:
> On 09/25/2013 11:01 AM, Alexey Kardashevskiy wrote:
>> On 09/17/2013 12:16 AM, Alexey Kardashevskiy wrote:
>>> On 09/10/2013 02:15 PM, Alexey Kardashevskiy wrote:
>>>> On 08/16/2013 08:35 AM, Andreas Färber wrote:
>>>>> Set the expected values for POWER7, POWER7+, POWER8 and POWER5+.
>>>>> Note that POWER5+ and POWER7+ are intentionally lacking the '+', so the
>>>>> lack of a POWER7P family constitutes no problem.
>>>>>
>>>>> Signed-off-by: Andreas Färber<afaerber@suse.de>
>>>>
>>>> Out of curiosity - is anything going to happen to this series? Is it
>>>> awaiting for someone's review? Just asking as it is quite old and nobody
>>>> seems to care :)
>>> Ping, anyone? Not sure if any of my mails even reaches maillists :-/
>> Ping?
>>
>> It conflicts with "[PATCH] pseries: Fix loading of little endian kernels"
>> posted today. It would be great to have either this series or the new patch
>> upstream...
> 
> Andreas is going to rework it, yes :).


Wheeeen? Not complaining or anything, just asking :)


-- 
Alexey

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

[Andreas Färber]

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.

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);
+            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;
-- 
1.8.1.4

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

[Alexey Kardashevskiy]

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

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

[Andreas Färber]

Am 29.08.2013 06:29, schrieb Alexey Kardashevskiy:
> 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).

Negative, desc is a free-text field and may contain spaces, parenthesis,
etc. Each model may set desc differently btw, so given my change request
for the comparison, we might end up with "POWER7 v2.1" on that
particular PVR.

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

970 comes to mind? Anyway, this was just a more direct way to address
the issues raised by Prerna. If you guys don't see the need to enforce
these naming rules beyond a supported list of POWER CPUs then we can
strip it down further, possibly falling back to a fixed
"PowerPC,UNKNOWN" rather than trying to construct a name.

Andreas

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

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

[Alexey Kardashevskiy]

On 08/29/2013 03:30 PM, Andreas Färber wrote:
> Am 29.08.2013 06:29, schrieb Alexey Kardashevskiy:
>> 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).
> 
> Negative, desc is a free-text field and may contain spaces, parenthesis,
> etc. Each model may set desc differently btw, so given my change request
> for the comparison, we might end up with "POWER7 v2.1" on that
> particular PVR.

These patches are for spapr and spapr-supported CPUs have short nice names
in @desc. But ok, may be that's a wrong idea.


>> 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?
> 
> 970 comes to mind? Anyway, this was just a more direct way to address
> the issues raised by Prerna. If you guys don't see the need to enforce
> these naming rules beyond a supported list of POWER CPUs then we can
> strip it down further, possibly falling back to a fixed
> "PowerPC,UNKNOWN" rather than trying to construct a name.


The direct way would be to finish what the series started and assign
reasonable fw_name values to every existing family class (970, cell,
power6, rs64?).

There is very limited set of spapr CPU families, they are all there (except
power7+ but I'll have a patch for that too) and new CPU family will require
a new class anyway (so we will put fw_name there when we'll be adding the
class) OR we implement "compatibility mode" which will use one of existing
classes so we get a correct fw_name either way.

Either way I do not see problems with the patches as they are, they work, I
know how, and I am fine. I just do not like the approach very much (we
could have 2 patches but we have 4 even knowing that the last 2 will never
be used) but my (bad) taste does not matter here. Thanks.


-- 
Alexey

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

[Alexander Graf]

On 30.08.2013, at 08:05, Alexey Kardashevskiy wrote:

> On 08/29/2013 03:30 PM, Andreas Färber wrote:
>> Am 29.08.2013 06:29, schrieb Alexey Kardashevskiy:
>>> 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).
>> 
>> Negative, desc is a free-text field and may contain spaces, parenthesis,
>> etc. Each model may set desc differently btw, so given my change request
>> for the comparison, we might end up with "POWER7 v2.1" on that
>> particular PVR.
> 
> These patches are for spapr and spapr-supported CPUs have short nice names
> in @desc. But ok, may be that's a wrong idea.
> 
> 
>>> 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?
>> 
>> 970 comes to mind? Anyway, this was just a more direct way to address
>> the issues raised by Prerna. If you guys don't see the need to enforce
>> these naming rules beyond a supported list of POWER CPUs then we can
>> strip it down further, possibly falling back to a fixed
>> "PowerPC,UNKNOWN" rather than trying to construct a name.
> 
> 
> The direct way would be to finish what the series started and assign
> reasonable fw_name values to every existing family class (970, cell,
> power6, rs64?).

I agree :). Then there's no need for magic fw_name generation anymore and we have a very obvious code path, making the code simple.

> There is very limited set of spapr CPU families, they are all there (except
> power7+ but I'll have a patch for that too) and new CPU family will require
> a new class anyway (so we will put fw_name there when we'll be adding the
> class) OR we implement "compatibility mode" which will use one of existing
> classes so we get a correct fw_name either way.

Well, I think it makes sense to always provide a fw_name field, regardless of whether that particular CPU works with sPAPR or not. We could use the same field for ePAPR too for example.


Alex

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

[Andreas Färber]

Am 30.08.2013 15:54, schrieb Alexander Graf:
> 
> On 30.08.2013, at 08:05, Alexey Kardashevskiy wrote:
> 
>> On 08/29/2013 03:30 PM, Andreas Färber wrote:
>>> Am 29.08.2013 06:29, schrieb Alexey Kardashevskiy:
>>>> 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).
>>>
>>> Negative, desc is a free-text field and may contain spaces, parenthesis,
>>> etc. Each model may set desc differently btw, so given my change request
>>> for the comparison, we might end up with "POWER7 v2.1" on that
>>> particular PVR.
>>
>> These patches are for spapr and spapr-supported CPUs have short nice names
>> in @desc. But ok, may be that's a wrong idea.
>>
>>
>>>> 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?
>>>
>>> 970 comes to mind? Anyway, this was just a more direct way to address
>>> the issues raised by Prerna. If you guys don't see the need to enforce
>>> these naming rules beyond a supported list of POWER CPUs then we can
>>> strip it down further, possibly falling back to a fixed
>>> "PowerPC,UNKNOWN" rather than trying to construct a name.
>>
>>
>> The direct way would be to finish what the series started and assign
>> reasonable fw_name values to every existing family class (970, cell,
>> power6, rs64?).
> 
> I agree :). Then there's no need for magic fw_name generation anymore and we have a very obvious code path, making the code simple.
> 
>> There is very limited set of spapr CPU families, they are all there (except
>> power7+ but I'll have a patch for that too) and new CPU family will require
>> a new class anyway (so we will put fw_name there when we'll be adding the
>> class) OR we implement "compatibility mode" which will use one of existing
>> classes so we get a correct fw_name either way.
> 
> Well, I think it makes sense to always provide a fw_name field, regardless of whether that particular CPU works with sPAPR or not. We could use the same field for ePAPR too for example.

So does ePAPR have the same or similar naming rules? Could you supply us
with verified e500 etc. names so that it's not pure speculation on my part?

Andreas

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

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

[Alexander Graf]

On 30.08.2013, at 16:00, Andreas Färber wrote:

> Am 30.08.2013 15:54, schrieb Alexander Graf:
>> 
>> On 30.08.2013, at 08:05, Alexey Kardashevskiy wrote:
>> 
>>> On 08/29/2013 03:30 PM, Andreas Färber wrote:
>>>> Am 29.08.2013 06:29, schrieb Alexey Kardashevskiy:
>>>>> 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).
>>>> 
>>>> Negative, desc is a free-text field and may contain spaces, parenthesis,
>>>> etc. Each model may set desc differently btw, so given my change request
>>>> for the comparison, we might end up with "POWER7 v2.1" on that
>>>> particular PVR.
>>> 
>>> These patches are for spapr and spapr-supported CPUs have short nice names
>>> in @desc. But ok, may be that's a wrong idea.
>>> 
>>> 
>>>>> 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?
>>>> 
>>>> 970 comes to mind? Anyway, this was just a more direct way to address
>>>> the issues raised by Prerna. If you guys don't see the need to enforce
>>>> these naming rules beyond a supported list of POWER CPUs then we can
>>>> strip it down further, possibly falling back to a fixed
>>>> "PowerPC,UNKNOWN" rather than trying to construct a name.
>>> 
>>> 
>>> The direct way would be to finish what the series started and assign
>>> reasonable fw_name values to every existing family class (970, cell,
>>> power6, rs64?).
>> 
>> I agree :). Then there's no need for magic fw_name generation anymore and we have a very obvious code path, making the code simple.
>> 
>>> There is very limited set of spapr CPU families, they are all there (except
>>> power7+ but I'll have a patch for that too) and new CPU family will require
>>> a new class anyway (so we will put fw_name there when we'll be adding the
>>> class) OR we implement "compatibility mode" which will use one of existing
>>> classes so we get a correct fw_name either way.
>> 
>> Well, I think it makes sense to always provide a fw_name field, regardless of whether that particular CPU works with sPAPR or not. We could use the same field for ePAPR too for example.
> 
> So does ePAPR have the same or similar naming rules? Could you supply us
> with verified e500 etc. names so that it's not pure speculation on my part?

I don't see any rules on the cpu node naming in ePAPR, but if I look at the variety of namings in Linux's dts's I don't think we have to worry overly much, as long as we keep the names simple. I think in almost all cases sub-powerpc-class_name == fw_name is a pretty reasonable assumption.


Alex

kvm $ grep -R PowerPC arch/powerpc/boot/dts | sed -n 's/.*PowerPC,\(.*\) {/\1/p' | sort | uniq5121@0
5125@0
5200@0
603e  /* Really 8241 */
7447
7448@0
750CL@0
8241@0
8247@0
8248@0
8272@0
8308@0
8313@0
8315@0
8323@0
8347@0
8349@0
8360@0
8377@0
8378@0
8379@0
8536@0
8540@0
8541@0
8544@0
8548@0
8555@0
8560@0
8568@0
8569@0
8572@0
8572@1
860@0
8610@0
8641@0
8641@1
866@0
875@0
885@0
broadway@0
BSC9131@0
e500mc@0
e500mc@1
e500mc@2
e500mc@3
e500mc@4
e500mc@5
e500mc@6
e500mc@7
e5500@0
e5500@1
e5500@2
e5500@3
e6500@0
e6500@10
e6500@12
e6500@14
e6500@16
e6500@18
e6500@2
e6500@20
e6500@22
e6500@4
e6500@6
e6500@8
gekko@0
P1010@0
P1020@0
P1020@1
P1021@0
P1021@1
P1022@0
P1022@1
P1023@0
P1023@1
P2020@0
P2020@1

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

[Alexander Graf]

On 16.08.2013, at 00:35, 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.

Could we just mandate the fw_name field to always be set for all classes instead?


Alex

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

[Andreas Färber]

Am 30.08.2013 15:21, schrieb Alexander Graf:
> 
> On 16.08.2013, at 00:35, 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.
> 
> Could we just mandate the fw_name field to always be set for all classes instead?

Sure, we can assert it. But we would then need to set fw_name for the
various 970 families at least, which I have been using with pseries in
the past. Cell and POWER6 are TODO so I'm not concerned about them. Not
sure about RS64 that Alexey mentioned - I wouldn't be able to test.
Would be bad to regress and abort with CPU models that were working okay
before.

Andreas

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

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

[Alexander Graf]

On 30.08.2013, at 15:26, Andreas Färber wrote:

> Am 30.08.2013 15:21, schrieb Alexander Graf:
>> 
>> On 16.08.2013, at 00:35, 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.
>> 
>> Could we just mandate the fw_name field to always be set for all classes instead?
> 
> Sure, we can assert it. But we would then need to set fw_name for the
> various 970 families at least, which I have been using with pseries in
> the past. Cell and POWER6 are TODO so I'm not concerned about them. Not
> sure about RS64 that Alexey mentioned - I wouldn't be able to test.
> Would be bad to regress and abort with CPU models that were working okay
> before.

Well, we could just manually add a sensible fw_name field to every class. There aren't all that many, no? Worst case we follow the exact same pattern that you were suggesting in code, just that we manually put the names in the classes :).

At the end of the day, I would think this makes the code easier to understand.


Alex

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

[Alexey Kardashevskiy]

On 08/30/2013 11:26 PM, Andreas Färber wrote:
> Am 30.08.2013 15:21, schrieb Alexander Graf:
>>
>> On 16.08.2013, at 00:35, 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.
>>
>> Could we just mandate the fw_name field to always be set for all classes instead?
> 
> Sure, we can assert it. But we would then need to set fw_name for the
> various 970 families at least, which I have been using with pseries in
> the past. Cell and POWER6 are TODO so I'm not concerned about them. Not
> sure about RS64 that Alexey mentioned - I wouldn't be able to test.
> Would be bad to regress and abort with CPU models that were working okay
> before.

If we generated fw_name as it would have been done by the current helpers,
how would anything regress?



-- 
Alexey

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

[Andreas Färber]

Am 30.08.2013 15:53, schrieb Alexey Kardashevskiy:
> On 08/30/2013 11:26 PM, Andreas Färber wrote:
>> Am 30.08.2013 15:21, schrieb Alexander Graf:
>>>
>>> On 16.08.2013, at 00:35, 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.
>>>
>>> Could we just mandate the fw_name field to always be set for all classes instead?
>>
>> Sure, we can assert it. But we would then need to set fw_name for the
>> various 970 families at least, which I have been using with pseries in
>> the past. Cell and POWER6 are TODO so I'm not concerned about them. Not
>> sure about RS64 that Alexey mentioned - I wouldn't be able to test.
>> Would be bad to regress and abort with CPU models that were working okay
>> before.
> 
> If we generated fw_name as it would have been done by the current helpers,
> how would anything regress?

Well, before, you were talking about assigning "reasonable fw_name
values", and that would for me require knowledge of which value is
reasonable (i.e., correct) for a certain CPU, and I don't have boards to
check most of them.

My proposal would be to just do it for the sPAPR-compatible (not sure if
that is what you meant with "supported"?) CPUs (because that's where we
intend to use it) and mark the ones we don't know exactly with /* ??? */
as done elsewhere in ppc code.

Andreas

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

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

[Alexander Graf]

On 30.08.2013, at 15:58, Andreas Färber wrote:

> Am 30.08.2013 15:53, schrieb Alexey Kardashevskiy:
>> On 08/30/2013 11:26 PM, Andreas Färber wrote:
>>> Am 30.08.2013 15:21, schrieb Alexander Graf:
>>>> 
>>>> On 16.08.2013, at 00:35, 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.
>>>> 
>>>> Could we just mandate the fw_name field to always be set for all classes instead?
>>> 
>>> Sure, we can assert it. But we would then need to set fw_name for the
>>> various 970 families at least, which I have been using with pseries in
>>> the past. Cell and POWER6 are TODO so I'm not concerned about them. Not
>>> sure about RS64 that Alexey mentioned - I wouldn't be able to test.
>>> Would be bad to regress and abort with CPU models that were working okay
>>> before.
>> 
>> If we generated fw_name as it would have been done by the current helpers,
>> how would anything regress?
> 
> Well, before, you were talking about assigning "reasonable fw_name
> values", and that would for me require knowledge of which value is
> reasonable (i.e., correct) for a certain CPU, and I don't have boards to
> check most of them.
> 
> My proposal would be to just do it for the sPAPR-compatible (not sure if
> that is what you meant with "supported"?) CPUs (because that's where we
> intend to use it) and mark the ones we don't know exactly with /* ??? */
> as done elsewhere in ppc code.

Yup, we could even specifically just name them fw_name = "PowerPC,UNKNOWN"; whenever we don't know better as you were suggesting earlier.


Alex

[Qemu-devel] [PATCH v2 3/4] spapr: Improve device tree CPU node for -cpu host with unknown OF name

[Andreas Färber]

Whenever DeviceClass::fw_name is not available, derive it from the CPU's
type name, resorting to the parent's type in case of -cpu host, and fill
it in for that class in a PAPR-compliant way with "PowerPC," prefix.

Reported-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/ppc/spapr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6d984dc..0e9be32 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -344,6 +344,10 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
             const char *typename;
 
             typename = object_class_get_name(oc);
+            if (kvm_enabled() &&
+                strcmp(typename, "host-" TYPE_POWERPC_CPU) == 0) {
+                typename = object_class_get_name(object_class_get_parent(oc));
+            }
             nodename = g_strndup(typename,
                                  strlen(typename) - strlen("-" TYPE_POWERPC_CPU));
             dc->fw_name = g_strdup_printf("PowerPC,%s", nodename);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 3/4] spapr: Improve device tree CPU node for -cpu host with unknown OF name

[Andreas Färber]

Am 16.08.2013 00:35, schrieb Andreas Färber:
> Whenever DeviceClass::fw_name is not available, derive it from the CPU's
> type name, resorting to the parent's type in case of -cpu host, and fill
> it in for that class in a PAPR-compliant way with "PowerPC," prefix.

Ugh, obviously forgot to edit the commit message after splitting this
off from the preceding patch... ;)

Andreas

> 
> Reported-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/ppc/spapr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6d984dc..0e9be32 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -344,6 +344,10 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>              const char *typename;
>  
>              typename = object_class_get_name(oc);
> +            if (kvm_enabled() &&
> +                strcmp(typename, "host-" TYPE_POWERPC_CPU) == 0) {
> +                typename = object_class_get_name(object_class_get_parent(oc));
> +            }
>              nodename = g_strndup(typename,
>                                   strlen(typename) - strlen("-" TYPE_POWERPC_CPU));
>              dc->fw_name = g_strdup_printf("PowerPC,%s", nodename);
> 


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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 3/4] spapr: Improve device tree CPU node for -cpu host with unknown OF name

[Alexey Kardashevskiy]

On 08/16/2013 08:35 AM, Andreas Färber wrote:
> Whenever DeviceClass::fw_name is not available, derive it from the CPU's
> type name, resorting to the parent's type in case of -cpu host, and fill
> it in for that class in a PAPR-compliant way with "PowerPC," prefix.
> 
> Reported-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/ppc/spapr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6d984dc..0e9be32 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -344,6 +344,10 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>              const char *typename;
>  
>              typename = object_class_get_name(oc);
> +            if (kvm_enabled() &&
> +                strcmp(typename, "host-" TYPE_POWERPC_CPU) == 0) {
> +                typename = object_class_get_name(object_class_get_parent(oc));
> +            }
>              nodename = g_strndup(typename,
>                                   strlen(typename) - strlen("-" TYPE_POWERPC_CPU));
>              dc->fw_name = g_strdup_printf("PowerPC,%s", nodename);
> 

You could use dc->desc of a parent, would be simpler, no?



-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 3/4] spapr: Improve device tree CPU node for -cpu host with unknown OF name

[Andreas Färber]

Am 29.08.2013 06:33, schrieb Alexey Kardashevskiy:
> On 08/16/2013 08:35 AM, Andreas Färber wrote:
>> Whenever DeviceClass::fw_name is not available, derive it from the CPU's
>> type name, resorting to the parent's type in case of -cpu host, and fill
>> it in for that class in a PAPR-compliant way with "PowerPC," prefix.
>>
>> Reported-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  hw/ppc/spapr.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 6d984dc..0e9be32 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -344,6 +344,10 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>              const char *typename;
>>  
>>              typename = object_class_get_name(oc);
>> +            if (kvm_enabled() &&
>> +                strcmp(typename, "host-" TYPE_POWERPC_CPU) == 0) {
>> +                typename = object_class_get_name(object_class_get_parent(oc));
>> +            }
>>              nodename = g_strndup(typename,
>>                                   strlen(typename) - strlen("-" TYPE_POWERPC_CPU));
>>              dc->fw_name = g_strdup_printf("PowerPC,%s", nodename);
>>
> 
> You could use dc->desc of a parent, would be simpler, no?

Answered on 2/4.

Andreas

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

[Qemu-devel] [PATCH v2 4/4] spapr: Suppress underscores in device tree CPU node

[Andreas Färber]

PAPR requires that PowerPC,<name> shall not contain underscores, so skip
any underscores in the type name.

Reported-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/ppc/spapr.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0e9be32..137e060 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -264,7 +264,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
     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)};
-    int i, smt = kvmppc_smt_threads();
+    int i, j, smt = kvmppc_smt_threads();
     unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
 
     fdt = g_malloc0(FDT_MAX_SIZE);
@@ -350,6 +350,17 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
             }
             nodename = g_strndup(typename,
                                  strlen(typename) - strlen("-" TYPE_POWERPC_CPU));
+            for (i = j = 0; j < strlen(nodename); i++, j++) {
+                if (nodename[j] == '_') {
+                    j++;
+                }
+                if (j > i) {
+                    nodename[i] = nodename[j];
+                }
+            }
+            if (j > i) {
+                nodename[i] = '\0';
+            }
             dc->fw_name = g_strdup_printf("PowerPC,%s", nodename);
             g_free(nodename);
         }
-- 
1.8.1.4

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 4/4] spapr: Suppress underscores in device tree CPU node

[Alexey Kardashevskiy]

On 08/16/2013 08:35 AM, Andreas Färber wrote:
> PAPR requires that PowerPC,<name> shall not contain underscores, so skip
> any underscores in the type name.

Again, as this is for sPAPR only and spapr-supported CPUs have fw_name, why
to bother with "_" at all? Where can it possibly come from?

Sorry if I am asking something really stupid, it is just that the solution
for the pretty trivial problem looks bigger that I'd expect :)


> Reported-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/ppc/spapr.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0e9be32..137e060 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -264,7 +264,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>      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)};
> -    int i, smt = kvmppc_smt_threads();
> +    int i, j, smt = kvmppc_smt_threads();
>      unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
>  
>      fdt = g_malloc0(FDT_MAX_SIZE);
> @@ -350,6 +350,17 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>              }
>              nodename = g_strndup(typename,
>                                   strlen(typename) - strlen("-" TYPE_POWERPC_CPU));
> +            for (i = j = 0; j < strlen(nodename); i++, j++) {
> +                if (nodename[j] == '_') {
> +                    j++;
> +                }
> +                if (j > i) {
> +                    nodename[i] = nodename[j];
> +                }
> +            }
> +            if (j > i) {
> +                nodename[i] = '\0';
> +            }
>              dc->fw_name = g_strdup_printf("PowerPC,%s", nodename);
>              g_free(nodename);
>          }
> 


-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 4/4] spapr: Suppress underscores in device tree CPU node

[Andreas Färber]

Am 29.08.2013 06:39, schrieb Alexey Kardashevskiy:
> On 08/16/2013 08:35 AM, Andreas Färber wrote:
>> PAPR requires that PowerPC,<name> shall not contain underscores, so skip
>> any underscores in the type name.
> 
> Again, as this is for sPAPR only and spapr-supported CPUs have fw_name, why
> to bother with "_" at all? Where can it possibly come from?

Example given on 2/4. In the end it depends on how strict we want to be
and how we construct the node name.

Andeas

> Sorry if I am asking something really stupid, it is just that the solution
> for the pretty trivial problem looks bigger that I'd expect :)

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