[Qemu-devel] [RFC PATCH] target-ppc: Register CPU class per family only when needed

[Qemu-devel] [RFC PATCH] target-ppc: Register CPU class per family only when needed

[Alexey Kardashevskiy]

At the moment when running in KVM mode, QEMU registers "host" class to
match the current CPU PVR value. It also registers another CPU class
with a CPU family name os if we run QEMU on POWER7 machine, "host" and
"POWER7" classes are created, this way we can always use "-cpu POWER7"
on the actual POWER7 machine.

The existing code uses DeviceClass::desc field of the CPU class as
a source for the class name; it was pointed out that it is wrong to use
user-visible string as a type name.

This adds a common CPU class name into PowerPCCPUClass struct.
This makes registration of a CPU named after the family conditional -
PowerPCCPUClass::common_cpu_name has to be non-zero. Only POWER7/POWER8
families have this field initialized by now.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target-ppc/cpu-qom.h        |  1 +
 target-ppc/kvm.c            | 11 ++++++-----
 target-ppc/translate_init.c |  2 ++
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 6967a80..4b471d7 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -55,6 +55,7 @@ typedef struct PowerPCCPUClass {
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
 
+    const char *common_cpu_name;
     uint32_t pvr;
     bool (*pvr_match)(struct PowerPCCPUClass *pcc, uint32_t pvr);
     uint64_t pcr_mask;
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index b479471..3f2df65 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2221,7 +2221,6 @@ static int kvm_ppc_register_host_cpu_type(void)
     };
     uint32_t host_pvr = mfpvr();
     PowerPCCPUClass *pvr_pcc;
-    DeviceClass *dc;
 
     pvr_pcc = ppc_cpu_class_by_pvr(host_pvr);
     if (pvr_pcc == NULL) {
@@ -2235,10 +2234,12 @@ static int kvm_ppc_register_host_cpu_type(void)
 
     /* Register generic family CPU class for a family */
     pvr_pcc = ppc_cpu_get_family_class(pvr_pcc);
-    dc = DEVICE_CLASS(pvr_pcc);
-    type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
-    type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU, dc->desc);
-    type_register(&type_info);
+    if (pvr_pcc->common_cpu_name) {
+        type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
+        type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU,
+                                         pvr_pcc->common_cpu_name);
+        type_register(&type_info);
+    }
 
     return 0;
 }
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index df1a62c..3d0be66 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8117,6 +8117,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
     dc->fw_name = "PowerPC,POWER7";
     dc->desc = "POWER7";
     dc->props = powerpc_servercpu_properties;
+    pcc->common_cpu_name = "POWER7";
     pcc->pvr_match = ppc_pvr_match_power7;
     pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
     pcc->init_proc = init_proc_POWER7;
@@ -8193,6 +8194,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
     dc->fw_name = "PowerPC,POWER8";
     dc->desc = "POWER8";
     dc->props = powerpc_servercpu_properties;
+    pcc->common_cpu_name = "POWER8";
     pcc->pvr_match = ppc_pvr_match_power8;
     pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
     pcc->init_proc = init_proc_POWER8;
-- 
2.0.0

Re: [Qemu-devel] [RFC PATCH] target-ppc: Register CPU class per family only when needed

[Alexander Graf]

On 05.03.15 02:56, Alexey Kardashevskiy wrote:
> At the moment when running in KVM mode, QEMU registers "host" class to
> match the current CPU PVR value. It also registers another CPU class
> with a CPU family name os if we run QEMU on POWER7 machine, "host" and
> "POWER7" classes are created, this way we can always use "-cpu POWER7"
> on the actual POWER7 machine.
> 
> The existing code uses DeviceClass::desc field of the CPU class as
> a source for the class name; it was pointed out that it is wrong to use
> user-visible string as a type name.
> 
> This adds a common CPU class name into PowerPCCPUClass struct.
> This makes registration of a CPU named after the family conditional -
> PowerPCCPUClass::common_cpu_name has to be non-zero. Only POWER7/POWER8
> families have this field initialized by now.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

LGTM. Andreas, do you agree?


Alex


> ---
>  target-ppc/cpu-qom.h        |  1 +
>  target-ppc/kvm.c            | 11 ++++++-----
>  target-ppc/translate_init.c |  2 ++
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 6967a80..4b471d7 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -55,6 +55,7 @@ typedef struct PowerPCCPUClass {
>      DeviceRealize parent_realize;
>      void (*parent_reset)(CPUState *cpu);
>  
> +    const char *common_cpu_name;
>      uint32_t pvr;
>      bool (*pvr_match)(struct PowerPCCPUClass *pcc, uint32_t pvr);
>      uint64_t pcr_mask;
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index b479471..3f2df65 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2221,7 +2221,6 @@ static int kvm_ppc_register_host_cpu_type(void)
>      };
>      uint32_t host_pvr = mfpvr();
>      PowerPCCPUClass *pvr_pcc;
> -    DeviceClass *dc;
>  
>      pvr_pcc = ppc_cpu_class_by_pvr(host_pvr);
>      if (pvr_pcc == NULL) {
> @@ -2235,10 +2234,12 @@ static int kvm_ppc_register_host_cpu_type(void)
>  
>      /* Register generic family CPU class for a family */
>      pvr_pcc = ppc_cpu_get_family_class(pvr_pcc);
> -    dc = DEVICE_CLASS(pvr_pcc);
> -    type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
> -    type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU, dc->desc);
> -    type_register(&type_info);
> +    if (pvr_pcc->common_cpu_name) {
> +        type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
> +        type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU,
> +                                         pvr_pcc->common_cpu_name);
> +        type_register(&type_info);
> +    }
>  
>      return 0;
>  }
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index df1a62c..3d0be66 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -8117,6 +8117,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>      dc->fw_name = "PowerPC,POWER7";
>      dc->desc = "POWER7";
>      dc->props = powerpc_servercpu_properties;
> +    pcc->common_cpu_name = "POWER7";
>      pcc->pvr_match = ppc_pvr_match_power7;
>      pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
>      pcc->init_proc = init_proc_POWER7;
> @@ -8193,6 +8194,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>      dc->fw_name = "PowerPC,POWER8";
>      dc->desc = "POWER8";
>      dc->props = powerpc_servercpu_properties;
> +    pcc->common_cpu_name = "POWER8";
>      pcc->pvr_match = ppc_pvr_match_power8;
>      pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
>      pcc->init_proc = init_proc_POWER8;
> 

Re: [Qemu-devel] [RFC PATCH] target-ppc: Register CPU class per family only when needed

[Alexey Kardashevskiy]

On 03/06/2015 12:17 AM, Alexander Graf wrote:
>
>
> On 05.03.15 02:56, Alexey Kardashevskiy wrote:
>> At the moment when running in KVM mode, QEMU registers "host" class to
>> match the current CPU PVR value. It also registers another CPU class
>> with a CPU family name os if we run QEMU on POWER7 machine, "host" and
>> "POWER7" classes are created, this way we can always use "-cpu POWER7"
>> on the actual POWER7 machine.
>>
>> The existing code uses DeviceClass::desc field of the CPU class as
>> a source for the class name; it was pointed out that it is wrong to use
>> user-visible string as a type name.
>>
>> This adds a common CPU class name into PowerPCCPUClass struct.
>> This makes registration of a CPU named after the family conditional -
>> PowerPCCPUClass::common_cpu_name has to be non-zero. Only POWER7/POWER8
>> families have this field initialized by now.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> LGTM. Andreas, do you agree?


Ping?


>
>
> Alex
>
>
>> ---
>>   target-ppc/cpu-qom.h        |  1 +
>>   target-ppc/kvm.c            | 11 ++++++-----
>>   target-ppc/translate_init.c |  2 ++
>>   3 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
>> index 6967a80..4b471d7 100644
>> --- a/target-ppc/cpu-qom.h
>> +++ b/target-ppc/cpu-qom.h
>> @@ -55,6 +55,7 @@ typedef struct PowerPCCPUClass {
>>       DeviceRealize parent_realize;
>>       void (*parent_reset)(CPUState *cpu);
>>
>> +    const char *common_cpu_name;
>>       uint32_t pvr;
>>       bool (*pvr_match)(struct PowerPCCPUClass *pcc, uint32_t pvr);
>>       uint64_t pcr_mask;
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index b479471..3f2df65 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -2221,7 +2221,6 @@ static int kvm_ppc_register_host_cpu_type(void)
>>       };
>>       uint32_t host_pvr = mfpvr();
>>       PowerPCCPUClass *pvr_pcc;
>> -    DeviceClass *dc;
>>
>>       pvr_pcc = ppc_cpu_class_by_pvr(host_pvr);
>>       if (pvr_pcc == NULL) {
>> @@ -2235,10 +2234,12 @@ static int kvm_ppc_register_host_cpu_type(void)
>>
>>       /* Register generic family CPU class for a family */
>>       pvr_pcc = ppc_cpu_get_family_class(pvr_pcc);
>> -    dc = DEVICE_CLASS(pvr_pcc);
>> -    type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
>> -    type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU, dc->desc);
>> -    type_register(&type_info);
>> +    if (pvr_pcc->common_cpu_name) {
>> +        type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
>> +        type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU,
>> +                                         pvr_pcc->common_cpu_name);
>> +        type_register(&type_info);
>> +    }
>>
>>       return 0;
>>   }
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index df1a62c..3d0be66 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -8117,6 +8117,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>>       dc->fw_name = "PowerPC,POWER7";
>>       dc->desc = "POWER7";
>>       dc->props = powerpc_servercpu_properties;
>> +    pcc->common_cpu_name = "POWER7";
>>       pcc->pvr_match = ppc_pvr_match_power7;
>>       pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
>>       pcc->init_proc = init_proc_POWER7;
>> @@ -8193,6 +8194,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>>       dc->fw_name = "PowerPC,POWER8";
>>       dc->desc = "POWER8";
>>       dc->props = powerpc_servercpu_properties;
>> +    pcc->common_cpu_name = "POWER8";
>>       pcc->pvr_match = ppc_pvr_match_power8;
>>       pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
>>       pcc->init_proc = init_proc_POWER8;
>>


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] target-ppc: Register CPU class per family only when needed

[Andreas Färber]

Am 16.03.2015 um 05:58 schrieb Alexey Kardashevskiy:
> On 03/06/2015 12:17 AM, Alexander Graf wrote:
>> On 05.03.15 02:56, Alexey Kardashevskiy wrote:
>>> At the moment when running in KVM mode, QEMU registers "host" class to
>>> match the current CPU PVR value. It also registers another CPU class
>>> with a CPU family name os if we run QEMU on POWER7 machine, "host" and
>>> "POWER7" classes are created, this way we can always use "-cpu POWER7"
>>> on the actual POWER7 machine.
>>>
>>> The existing code uses DeviceClass::desc field of the CPU class as
>>> a source for the class name; it was pointed out that it is wrong to use
>>> user-visible string as a type name.
>>>
>>> This adds a common CPU class name into PowerPCCPUClass struct.
>>> This makes registration of a CPU named after the family conditional -
>>> PowerPCCPUClass::common_cpu_name has to be non-zero. Only POWER7/POWER8
>>> families have this field initialized by now.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> LGTM. Andreas, do you agree?
> 
> 
> Ping?

No, I don't agree. Inventing a new class field just to distinguish
POWER7/POWER8 here seems like a weird idea, and the code placement is
not fixed either.

I gathered that you want -cpu POWER7 and -cpu POWER8 to work on POWER8
hardware and -cpu POWER7 on POWER7, for migration purposes, correct?

What exact PVRs have you tested on and why does it not work without
those types despite the PVR masking? To investigate I need a test case.

Is this just a question of the generic family type being abstract and
needing an updated PVR value? Which other fields are actually used?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC PATCH] target-ppc: Register CPU class per family only when needed

[Alexey Kardashevskiy]

On 03/16/2015 09:40 PM, Andreas Färber wrote:
> Am 16.03.2015 um 05:58 schrieb Alexey Kardashevskiy:
>> On 03/06/2015 12:17 AM, Alexander Graf wrote:
>>> On 05.03.15 02:56, Alexey Kardashevskiy wrote:
>>>> At the moment when running in KVM mode, QEMU registers "host" class to
>>>> match the current CPU PVR value. It also registers another CPU class
>>>> with a CPU family name os if we run QEMU on POWER7 machine, "host" and
>>>> "POWER7" classes are created, this way we can always use "-cpu POWER7"
>>>> on the actual POWER7 machine.
>>>>
>>>> The existing code uses DeviceClass::desc field of the CPU class as
>>>> a source for the class name; it was pointed out that it is wrong to use
>>>> user-visible string as a type name.
>>>>
>>>> This adds a common CPU class name into PowerPCCPUClass struct.
>>>> This makes registration of a CPU named after the family conditional -
>>>> PowerPCCPUClass::common_cpu_name has to be non-zero. Only POWER7/POWER8
>>>> families have this field initialized by now.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>
>>> LGTM. Andreas, do you agree?
>>
>>
>> Ping?
>
> No, I don't agree. Inventing a new class field just to distinguish
> POWER7/POWER8 here seems like a weird idea,

As weird as PVR itself :)

> and the code placement is not fixed either.

What is wrong with the code placement?


> I gathered that you want -cpu POWER7 and -cpu POWER8 to work on POWER8
> hardware and -cpu POWER7 on POWER7, for migration purposes, correct?
>
> What exact PVRs have you tested on and why does it not work without
> those types despite the PVR masking? To investigate I need a test case.

The real host is 003f 0201. -cpu POWER7 will fail without my patches as 
POWER7 is alias of 003f 0203.

Or real host 004b 0201 - -cpu POWER8 will try 004d 0100 and fail.


> Is this just a question of the generic family type being abstract and
> needing an updated PVR value?

May be. That could help too I suppose.

> Which other fields are actually used?

Sorry, used where? :)



-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] target-ppc: Register CPU class per family only when needed

[Alexey Kardashevskiy]

Adding David to this old conversation.

On 03/17/2015 09:47 AM, Alexey Kardashevskiy wrote:
> On 03/16/2015 09:40 PM, Andreas Färber wrote:
>> Am 16.03.2015 um 05:58 schrieb Alexey Kardashevskiy:
>>> On 03/06/2015 12:17 AM, Alexander Graf wrote:
>>>> On 05.03.15 02:56, Alexey Kardashevskiy wrote:
>>>>> At the moment when running in KVM mode, QEMU registers "host" class to
>>>>> match the current CPU PVR value. It also registers another CPU class
>>>>> with a CPU family name os if we run QEMU on POWER7 machine, "host" and
>>>>> "POWER7" classes are created, this way we can always use "-cpu POWER7"
>>>>> on the actual POWER7 machine.
>>>>>
>>>>> The existing code uses DeviceClass::desc field of the CPU class as
>>>>> a source for the class name; it was pointed out that it is wrong to use
>>>>> user-visible string as a type name.
>>>>>
>>>>> This adds a common CPU class name into PowerPCCPUClass struct.
>>>>> This makes registration of a CPU named after the family conditional -
>>>>> PowerPCCPUClass::common_cpu_name has to be non-zero. Only POWER7/POWER8
>>>>> families have this field initialized by now.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>
>>>> LGTM. Andreas, do you agree?
>>>
>>>
>>> Ping?
>>
>> No, I don't agree. Inventing a new class field just to distinguish
>> POWER7/POWER8 here seems like a weird idea,
>
> As weird as PVR itself :)
>
>> and the code placement is not fixed either.
>
> What is wrong with the code placement?
>
>
>> I gathered that you want -cpu POWER7 and -cpu POWER8 to work on POWER8
>> hardware and -cpu POWER7 on POWER7, for migration purposes, correct?
>>
>> What exact PVRs have you tested on and why does it not work without
>> those types despite the PVR masking? To investigate I need a test case.
>
> The real host is 003f 0201. -cpu POWER7 will fail without my patches as
> POWER7 is alias of 003f 0203.
>
> Or real host 004b 0201 - -cpu POWER8 will try 004d 0100 and fail.
>
>
>> Is this just a question of the generic family type being abstract and
>> needing an updated PVR value?
>
> May be. That could help too I suppose.
>
>> Which other fields are actually used?
>
> Sorry, used where? :)
>
>
>


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] target-ppc: Register CPU class per family only when needed

[Alexey Kardashevskiy]

On 07/08/2015 04:37 PM, Alexey Kardashevskiy wrote:
> Adding David to this old conversation.

This is the patch:
http://patchwork.ozlabs.org/patch/446544/



>
> On 03/17/2015 09:47 AM, Alexey Kardashevskiy wrote:
>> On 03/16/2015 09:40 PM, Andreas Färber wrote:
>>> Am 16.03.2015 um 05:58 schrieb Alexey Kardashevskiy:
>>>> On 03/06/2015 12:17 AM, Alexander Graf wrote:
>>>>> On 05.03.15 02:56, Alexey Kardashevskiy wrote:
>>>>>> At the moment when running in KVM mode, QEMU registers "host" class to
>>>>>> match the current CPU PVR value. It also registers another CPU class
>>>>>> with a CPU family name os if we run QEMU on POWER7 machine, "host" and
>>>>>> "POWER7" classes are created, this way we can always use "-cpu POWER7"
>>>>>> on the actual POWER7 machine.
>>>>>>
>>>>>> The existing code uses DeviceClass::desc field of the CPU class as
>>>>>> a source for the class name; it was pointed out that it is wrong to use
>>>>>> user-visible string as a type name.
>>>>>>
>>>>>> This adds a common CPU class name into PowerPCCPUClass struct.
>>>>>> This makes registration of a CPU named after the family conditional -
>>>>>> PowerPCCPUClass::common_cpu_name has to be non-zero. Only POWER7/POWER8
>>>>>> families have this field initialized by now.
>>>>>>
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>
>>>>> LGTM. Andreas, do you agree?
>>>>
>>>>
>>>> Ping?
>>>
>>> No, I don't agree. Inventing a new class field just to distinguish
>>> POWER7/POWER8 here seems like a weird idea,
>>
>> As weird as PVR itself :)
>>
>>> and the code placement is not fixed either.
>>
>> What is wrong with the code placement?
>>
>>
>>> I gathered that you want -cpu POWER7 and -cpu POWER8 to work on POWER8
>>> hardware and -cpu POWER7 on POWER7, for migration purposes, correct?
>>>
>>> What exact PVRs have you tested on and why does it not work without
>>> those types despite the PVR masking? To investigate I need a test case.
>>
>> The real host is 003f 0201. -cpu POWER7 will fail without my patches as
>> POWER7 is alias of 003f 0203.
>>
>> Or real host 004b 0201 - -cpu POWER8 will try 004d 0100 and fail.
>>
>>
>>> Is this just a question of the generic family type being abstract and
>>> needing an updated PVR value?
>>
>> May be. That could help too I suppose.
>>
>>> Which other fields are actually used?
>>
>> Sorry, used where? :)
>>
>>
>>
>
>


-- 
Alexey