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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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