[Qemu-devel] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Qemu-devel] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Andreas Färber]

This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid
double-registration of types:

  Registering `POWER5+-powerpc64-cpu' which already exists

Taking the textual description of a CPU type as part of a new type name
is plain wrong, and so is unconditionally registering a new type here.

Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-ppc/kvm.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 1edf2b5..9d614ef 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2160,18 +2160,6 @@ bool kvmppc_has_cap_fixup_hcalls(void)
     return cap_fixup_hcalls;
 }
 
-static PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc)
-{
-    ObjectClass *oc = OBJECT_CLASS(pcc);
-
-    while (oc && !object_class_is_abstract(oc)) {
-        oc = object_class_get_parent(oc);
-    }
-    assert(oc);
-
-    return POWERPC_CPU_CLASS(oc);
-}
-
 static int kvm_ppc_register_host_cpu_type(void)
 {
     TypeInfo type_info = {
@@ -2181,7 +2169,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) {
@@ -2192,14 +2179,6 @@ static int kvm_ppc_register_host_cpu_type(void)
     }
     type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
     type_register(&type_info);
-
-    /* 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);
-
     return 0;
 }
 
-- 
2.3.0

Re: [Qemu-devel] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Andreas Färber]

Am 02.03.2015 um 14:37 schrieb Alexander Graf:
> On 01.03.15 01:31, Andreas Färber wrote:
>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid
>> double-registration of types:
>>
>>   Registering `POWER5+-powerpc64-cpu' which already exists
>>
>> Taking the textual description of a CPU type as part of a new type name
>> is plain wrong, and so is unconditionally registering a new type here.
>>
>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Doesn't this break p8 support?

Maybe, but p5 support was in longer and this is definitely a regression
and really really wrong. If you know a way to fix it without handing it
back to the IBM guys for more thought, feel free to give it a shot.

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] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Alexander Graf]

On 02.03.15 14:42, Andreas Färber wrote:
> Am 02.03.2015 um 14:37 schrieb Alexander Graf:
>> On 01.03.15 01:31, Andreas Färber wrote:
>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid
>>> double-registration of types:
>>>
>>>   Registering `POWER5+-powerpc64-cpu' which already exists
>>>
>>> Taking the textual description of a CPU type as part of a new type name
>>> is plain wrong, and so is unconditionally registering a new type here.
>>>
>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>
>> Doesn't this break p8 support?
> 
> Maybe, but p5 support was in longer and this is definitely a regression
> and really really wrong. If you know a way to fix it without handing it
> back to the IBM guys for more thought, feel free to give it a shot.

I honestly don't fully remember what this was about. Wasn't this our
special KVM class that we use to create a compatible cpu type on the fly?

Alexey, please take a look at it.


Alex

> 
> Andreas
> 

Re: [Qemu-devel] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Andreas Färber]

Am 02.03.2015 um 14:51 schrieb Alexander Graf:
> On 02.03.15 14:42, Andreas Färber wrote:
>> Am 02.03.2015 um 14:37 schrieb Alexander Graf:
>>> On 01.03.15 01:31, Andreas Färber wrote:
>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid
>>>> double-registration of types:
>>>>
>>>>   Registering `POWER5+-powerpc64-cpu' which already exists
>>>>
>>>> Taking the textual description of a CPU type as part of a new type name
>>>> is plain wrong, and so is unconditionally registering a new type here.
>>>>
>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> Cc: qemu-stable@nongnu.org
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>
>>> Doesn't this break p8 support?
>>
>> Maybe, but p5 support was in longer and this is definitely a regression
>> and really really wrong. If you know a way to fix it without handing it
>> back to the IBM guys for more thought, feel free to give it a shot.
> 
> I honestly don't fully remember what this was about. Wasn't this our
> special KVM class that we use to create a compatible cpu type on the fly?

No, the class I create on the fly is a few lines above:

    pvr_pcc = ppc_cpu_class_by_pvr(host_pvr);
    if (pvr_pcc == NULL) {
        pvr_pcc = ppc_cpu_class_by_pvr_mask(host_pvr);
    }
    if (pvr_pcc == NULL) {
        return -1;
    }
    type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
    type_register(&type_info);

So, if no matching class is returned, we never reach the offending code.

Here, a second type with the same parent was being created in the
kvm_ppc_register_host_cpu_type() function that is supposed to create
that host CPU type. Why? The host CPU type by definition should already
have the right PVR taken from the host. kvmppc_host_cpu_class_init():

    /* Now fix up the class with information we can query from the host */
    pcc->pvr = mfpvr();

> Alexey, please take a look at it.

Thanks,
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] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Alexey Kardashevskiy]

On 03/03/2015 12:51 AM, Alexander Graf wrote:
>
>
> On 02.03.15 14:42, Andreas Färber wrote:
>> Am 02.03.2015 um 14:37 schrieb Alexander Graf:
>>> On 01.03.15 01:31, Andreas Färber wrote:
>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid
>>>> double-registration of types:
>>>>
>>>>    Registering `POWER5+-powerpc64-cpu' which already exists
>>>>
>>>> Taking the textual description of a CPU type as part of a new type name
>>>> is plain wrong, and so is unconditionally registering a new type here.
>>>>
>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> Cc: qemu-stable@nongnu.org
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>
>>> Doesn't this break p8 support?
>>
>> Maybe, but p5 support was in longer and this is definitely a regression
>> and really really wrong. If you know a way to fix it without handing it
>> back to the IBM guys for more thought, feel free to give it a shot.
>
> I honestly don't fully remember what this was about. Wasn't this our
> special KVM class that we use to create a compatible cpu type on the fly?
>
> Alexey, please take a look at it.


I sent a note yesterday :-/ Here it is again:

With this revert, running qemu with HV KVM and -cpu POWER7 fails on real 
POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of 
POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the first 
place. QEMU looks at classes first, and if not found - at aliases, so this 
worked.

I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias for 
POWER5+_v2.1 (or POWER5+_0.0).



-- 
Alexey

Re: [Qemu-devel] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Alexander Graf]

On 03.03.15 01:42, Alexey Kardashevskiy wrote:
> On 03/03/2015 12:51 AM, Alexander Graf wrote:
>>
>>
>> On 02.03.15 14:42, Andreas Färber wrote:
>>> Am 02.03.2015 um 14:37 schrieb Alexander Graf:
>>>> On 01.03.15 01:31, Andreas Färber wrote:
>>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid
>>>>> double-registration of types:
>>>>>
>>>>>    Registering `POWER5+-powerpc64-cpu' which already exists
>>>>>
>>>>> Taking the textual description of a CPU type as part of a new type
>>>>> name
>>>>> is plain wrong, and so is unconditionally registering a new type here.
>>>>>
>>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> Cc: qemu-stable@nongnu.org
>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>
>>>> Doesn't this break p8 support?
>>>
>>> Maybe, but p5 support was in longer and this is definitely a regression
>>> and really really wrong. If you know a way to fix it without handing it
>>> back to the IBM guys for more thought, feel free to give it a shot.
>>
>> I honestly don't fully remember what this was about. Wasn't this our
>> special KVM class that we use to create a compatible cpu type on the fly?
>>
>> Alexey, please take a look at it.
> 
> 
> I sent a note yesterday :-/ Here it is again:
> 
> With this revert, running qemu with HV KVM and -cpu POWER7 fails on real
> POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of
> POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the
> first place. QEMU looks at classes first, and if not found - at aliases,
> so this worked.
> 
> I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias
> for POWER5+_v2.1 (or POWER5+_0.0).

Care to send a patch?


Alex

Re: [Qemu-devel] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Alexey Kardashevskiy]

On 03/04/2015 07:43 AM, Alexander Graf wrote:
>
>
> On 03.03.15 01:42, Alexey Kardashevskiy wrote:
>> On 03/03/2015 12:51 AM, Alexander Graf wrote:
>>>
>>>
>>> On 02.03.15 14:42, Andreas Färber wrote:
>>>> Am 02.03.2015 um 14:37 schrieb Alexander Graf:
>>>>> On 01.03.15 01:31, Andreas Färber wrote:
>>>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid
>>>>>> double-registration of types:
>>>>>>
>>>>>>     Registering `POWER5+-powerpc64-cpu' which already exists
>>>>>>
>>>>>> Taking the textual description of a CPU type as part of a new type
>>>>>> name
>>>>>> is plain wrong, and so is unconditionally registering a new type here.
>>>>>>
>>>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>> Cc: qemu-stable@nongnu.org
>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>>
>>>>> Doesn't this break p8 support?
>>>>
>>>> Maybe, but p5 support was in longer and this is definitely a regression
>>>> and really really wrong. If you know a way to fix it without handing it
>>>> back to the IBM guys for more thought, feel free to give it a shot.
>>>
>>> I honestly don't fully remember what this was about. Wasn't this our
>>> special KVM class that we use to create a compatible cpu type on the fly?
>>>
>>> Alexey, please take a look at it.
>>
>>
>> I sent a note yesterday :-/ Here it is again:
>>
>> With this revert, running qemu with HV KVM and -cpu POWER7 fails on real
>> POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of
>> POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the
>> first place. QEMU looks at classes first, and if not found - at aliases,
>> so this worked.
>>
>> I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias
>> for POWER5+_v2.1 (or POWER5+_0.0).
>
> Care to send a patch?

I wonder if Andreas has a better solution to my initial problem - he 
obviously won't like the proposed patch :)




-- 
Alexey

Re: [Qemu-devel] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Andreas Färber]

Am 03.03.2015 um 23:14 schrieb Alexey Kardashevskiy:
> On 03/04/2015 07:43 AM, Alexander Graf wrote:
>> On 03.03.15 01:42, Alexey Kardashevskiy wrote:
>>> On 03/03/2015 12:51 AM, Alexander Graf wrote:
>>>> On 02.03.15 14:42, Andreas Färber wrote:
>>>>> Am 02.03.2015 um 14:37 schrieb Alexander Graf:
>>>>>> On 01.03.15 01:31, Andreas Färber wrote:
>>>>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to
>>>>>>> avoid
>>>>>>> double-registration of types:
>>>>>>>
>>>>>>>     Registering `POWER5+-powerpc64-cpu' which already exists
>>>>>>>
>>>>>>> Taking the textual description of a CPU type as part of a new type
>>>>>>> name
>>>>>>> is plain wrong, and so is unconditionally registering a new type
>>>>>>> here.
>>>>>>>
>>>>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>> Cc: qemu-stable@nongnu.org
>>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>>>
>>>>>> Doesn't this break p8 support?
>>>>>
>>>>> Maybe, but p5 support was in longer and this is definitely a
>>>>> regression
>>>>> and really really wrong. If you know a way to fix it without
>>>>> handing it
>>>>> back to the IBM guys for more thought, feel free to give it a shot.
>>>>
>>>> I honestly don't fully remember what this was about. Wasn't this our
>>>> special KVM class that we use to create a compatible cpu type on the
>>>> fly?
>>>>
>>>> Alexey, please take a look at it.
>>>
>>>
>>> I sent a note yesterday :-/ Here it is again:
>>>
>>> With this revert, running qemu with HV KVM and -cpu POWER7 fails on real
>>> POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of
>>> POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the
>>> first place. QEMU looks at classes first, and if not found - at aliases,
>>> so this worked.
>>>
>>> I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias
>>> for POWER5+_v2.1 (or POWER5+_0.0).
>>
>> Care to send a patch?
> 
> I wonder if Andreas has a better solution to my initial problem - he
> obviously won't like the proposed patch :)

Quite predictable, am I? ;)

Could you please explain in detail what problem you are seeing on POWER8
without this patch?

>From your new patch it rather sounds as if this was totally unrelated to
-cpu host and a new KVM-only feature, reinforcing my feeling that my
function is the wrong place for your code.

Also, as I pointed out, the description cannot safely be used as part of
the type name, as it may contain prohibited characters, so this still
needs fixing.

And for sure, if registering new types is indeed needed here, then a
check is needed for whether that type already exists and appropriate
error handling. I just don't understand why that is needed at all with
-cpu host taking the PVR as you say is needed here.

If you can precisely tell me what it is that you need then I'd be happy
to cook up a patch.

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] [PATCH] Revert "target-ppc: Create versionless CPU class per family if KVM"

[Alexey Kardashevskiy]

On 03/05/2015 01:55 AM, Andreas Färber wrote:
> Am 03.03.2015 um 23:14 schrieb Alexey Kardashevskiy:
>> On 03/04/2015 07:43 AM, Alexander Graf wrote:
>>> On 03.03.15 01:42, Alexey Kardashevskiy wrote:
>>>> On 03/03/2015 12:51 AM, Alexander Graf wrote:
>>>>> On 02.03.15 14:42, Andreas Färber wrote:
>>>>>> Am 02.03.2015 um 14:37 schrieb Alexander Graf:
>>>>>>> On 01.03.15 01:31, Andreas Färber wrote:
>>>>>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to
>>>>>>>> avoid
>>>>>>>> double-registration of types:
>>>>>>>>
>>>>>>>>      Registering `POWER5+-powerpc64-cpu' which already exists
>>>>>>>>
>>>>>>>> Taking the textual description of a CPU type as part of a new type
>>>>>>>> name
>>>>>>>> is plain wrong, and so is unconditionally registering a new type
>>>>>>>> here.
>>>>>>>>
>>>>>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>>> Cc: qemu-stable@nongnu.org
>>>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>>>>
>>>>>>> Doesn't this break p8 support?
>>>>>>
>>>>>> Maybe, but p5 support was in longer and this is definitely a
>>>>>> regression
>>>>>> and really really wrong. If you know a way to fix it without
>>>>>> handing it
>>>>>> back to the IBM guys for more thought, feel free to give it a shot.
>>>>>
>>>>> I honestly don't fully remember what this was about. Wasn't this our
>>>>> special KVM class that we use to create a compatible cpu type on the
>>>>> fly?
>>>>>
>>>>> Alexey, please take a look at it.
>>>>
>>>>
>>>> I sent a note yesterday :-/ Here it is again:
>>>>
>>>> With this revert, running qemu with HV KVM and -cpu POWER7 fails on real
>>>> POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of
>>>> POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the
>>>> first place. QEMU looks at classes first, and if not found - at aliases,
>>>> so this worked.
>>>>
>>>> I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias
>>>> for POWER5+_v2.1 (or POWER5+_0.0).
>>>
>>> Care to send a patch?
>>
>> I wonder if Andreas has a better solution to my initial problem - he
>> obviously won't like the proposed patch :)
>
> Quite predictable, am I? ;)
>
> Could you please explain in detail what problem you are seeing on POWER8
> without this patch?
>
>  From your new patch it rather sounds as if this was totally unrelated to
> -cpu host and a new KVM-only feature, reinforcing my feeling that my
> function is the wrong place for your code.
>
> Also, as I pointed out, the description cannot safely be used as part of
> the type name, as it may contain prohibited characters, so this still
> needs fixing.

So I can duplicate the CPU family name in PowerPCCPUClass. Which will 
always be the same as DeviceClass::desc. Well, it may be the right thing to do.


> And for sure, if registering new types is indeed needed here, then a
> check is needed for whether that type already exists and appropriate
> error handling.

I'll cook a patch for this.

 > I just don't understand why that is needed at all with
> -cpu host taking the PVR as you say is needed here.
>
> If you can precisely tell me what it is that you need then I'd be happy
> to cook up a patch.

I thought I did... I need a way to run QEMU under HV KVM with a CPU name, 
just like this - -cpu POWER7 on _any_ real POWER7 CPU (v2.1, v2.3 or even 
POWER7+). Simply because all CPUs from the family behave the same. But HV 
KVM cannot virtualize PVR, it is a hardware limitation. So this is what my 
original patch fixed/bandaid'd.

The original request for -cpu POWER7 vs. -cpu host came from libvirt for 
migration purposes - afair the issue was that the destination QEMU must not 
start if it is POWER8-host and the source is POWER7-host so trying -cpu 
POWER7 will fail on POWER8 (but -cpu host would work and this would be 
wrong); but migration between POWER7 2.1 and POWER7 2.3 should still work.

And in general there is no good reason why -cpu POWER7 cannot work on any 
POWER7 CPU.

I could try adding a dynamic alias for "POWER7" to "host" but at the moment 
aliases are static so new dynamic class seemed simpler.


-- 
Alexey