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

[Prerna Saxena <prerna@linux.vnet.ibm.com>]

On 08/08/2013 04:04 PM, Andreas Färber wrote:
> Am 08.08.2013 09:26, schrieb Prerna Saxena:
>>
>> From: Prerna Saxena <prerna@linux.vnet.ibm.com>
>> 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 "POWER7_V2.3@0"
>>
>> 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,POWER7@0/...
>> /proc/device-tree/cpus/PowerPC,POWER7@4/...
>>
>> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> 
> 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,CPU-NAME@...
>> +     * 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?

I dont know what context lead to this observation.
However, PAPR mentions the following nomenclature guideline:

"The value of this property shall be of the form: “PowerPC,<name>”,
where <name> is the name of the processor chip which may be displayed to
the user. <name> shall not contain underscores."

I think this name guideline will hold good for all PAPR compliant
processors.

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

Would this be the same use-case as reflected by: ppc_cpu_aliases.alias ?
If so, do we really need a separate field to convey the same information ?

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

If this is not a PAPR-compliant CPU, I dont think the PAPR naming
convention is of any good.
I havent worked with non-PAPR cpus. Is the device tree for such CPUs
generated by routines in hw/ppc/spapr.c ? Or do they have custom
routines to generate appropriate device tree nodes ?

Regards,
-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India