Re: [Qemu-ppc] POWER8 <-> POWER8E migration changed/broken in ppc-for.2.10. Intended?

[Alexey Kardashevskiy]

On 07/07/17 16:21, David Gibson wrote:
> On Fri, Jul 07, 2017 at 01:13:49PM +1000, Alexey Kardashevskiy wrote:
>> On 07/07/17 12:09, David Gibson wrote:
>>> On Tue, Jul 04, 2017 at 08:20:12AM -0300, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 06/22/2017 09:46 PM, Alexey Kardashevskiy wrote:
>>>>> On 22/06/17 18:19, David Gibson wrote:
>>>>>> On Thu, Jun 22, 2017 at 09:08:55AM +0200, Thomas Huth wrote:
>>>>>>> On 22.06.2017 05:22, Alexey Kardashevskiy wrote:
>>>>>>>> On 22/06/17 12:49, Alexey Kardashevskiy wrote:
>>>>>>>>> On 22/06/17 11:45, Alexey Kardashevskiy wrote:
>>>>>>>>>> On 22/06/17 07:19, Daniel Henrique Barboza wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> Yesterday I noticed that the migration between POWER8 and POWER8E 
>>>>>>>>>>> hosts
>>>>>>>>>>> isn't working
>>>>>>>>>>> as before. These are the machines that I've been using for DRC 
>>>>>>>>>>> testing and
>>>>>>>>>>> I discovered
>>>>>>>>>>> this behavior when trying to test the DRC cleanup series (v5).
>>>>>>>>>>>
>>>>>>>>>>> What happens in that the migration seems to be completed - no 
>>>>>>>>>>> errors on
>>>>>>>>>>> both source and
>>>>>>>>>>> destination, destination status marked as 'running' - but the guest 
>>>>>>>>>>> at dest
>>>>>>>>>>> looks
>>>>>>>>>>> frozen/inactive. Migrating from POWER8 to POWER8E or the other way 
>>>>>>>>>>> around
>>>>>>>>>>> presents the same behavior.
>>>>>>>>>>>
>>>>>>>>>>> This is the command line I'm executing:
>>>>>>>>>>>
>>>>>>>>>>> sudo ./qemu-system-ppc64 -name migrate_qemu -boot strict=on -device
>>>>>>>>>>> nec-usb-xhci,id=usb,bus=pci.0,addr=0xf -device
>>>>>>>>>>> spapr-vscsi,id=scsi0,reg=0x2000 -smp
>>>>>>>>>>> 1,maxcpus=4,sockets=4,cores=1,threads=1 --machine
>>>>>>>>>>> pseries,accel=kvm,kvm-type=HV,usb=off,dump-guest-core=off -m
>>>>>>>>>>> 4G,slots=32,maxmem=32G -drive
>>>>>>>>>>> file=/home/danielhb/vm_imgs/ubuntu1704.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none
>>>>>>>>>>> -device
>>>>>>>>>>> virtio-blk-pci,scsi=off,bus=pci.0,addr=0x2,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
>>>>>>>>>>> -nographic
>>>>>>>>>>>
>>>>>>>>>>> I've tracked the problem down to the commit b0517f54ef7d, "ppc: 
>>>>>>>>>>> Rework CPU
>>>>>>>>>>> compatibility testing across migration". If I revert this commit, 
>>>>>>>>>>> migration
>>>>>>>>>>> works as before
>>>>>>>>>>> between these hosts.
>>>>>>>>>> With b0517f54ef7d, CPU state is not marked dirty and SPRs (all 
>>>>>>>>>> registers in
>>>>>>>>>> fact) are not put to KVM at the end of migration, hence the freeze, 
>>>>>>>>>> this
>>>>>>>>>> has something to do with cpu_synchronize_state() called from 
>>>>>>>>>> ppc_set_compat().
>>>>>>>>> No, I was not right, please ignore.
>>>>>>>>
>>>>>>>> This hack fixes it:
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>>>>> index f2f7c531bc..7ec4146b2b 100644
>>>>>>>> --- a/target/ppc/kvm.c
>>>>>>>> +++ b/target/ppc/kvm.c
>>>>>>>> @@ -919,6 +919,7 @@ static int kvm_put_vpa(CPUState *cs)
>>>>>>>>       return 0;
>>>>>>>>   }
>>>>>>>>   #endif /* TARGET_PPC64 */
>>>>>>>> +static uint32_t mfpvr(void);
>>>>>>>>
>>>>>>>>   int kvmppc_put_books_sregs(PowerPCCPU *cpu)
>>>>>>>>   {
>>>>>>>> @@ -927,7 +928,7 @@ int kvmppc_put_books_sregs(PowerPCCPU *cpu)
>>>>>>>>       int i;
>>>>>>>>
>>>>>>>>       sregs.pvr = env->spr[SPR_PVR];
>>>>>>>> -
>>>>>>>> +    sregs.pvr = mfpvr();
>>>>>>>>       sregs.u.s.sdr1 = env->spr[SPR_SDR1];
>>>>>>>>
>>>>>>>>       /* Sync SLB */
>>>>>>> In case you consider to submit this patch: Please add a check for
>>>>>>> !kvmppc_is_pr() so that this is only done for KVM-HV.
>>>>>>>
>>>>>>> But I think the right way to hack this is somewhere in machine.c
>>>>>>> instead, eg. in cpu_post_load().
>>>>>> Is the problem is just that we're not considering POWER8 to be
>>>>>> compatible enough with POWER8E, then we should be altering the
>>>>>> pvr_match function so it does.
>>>>>
>>>>> The problem is that the KVM HV expects the exact host's PVR in
>>>>> KVM_SET_SREGS, other than that POWER8 == POWER8E pvr_match-wise afaict.
>>>>>
>>>>> And Paul suggested that this will bite us again when we start really
>>>>> worrying about migrating POWER8 to POWER9 so we should probably drop that
>>>>> PVR check in the KVM at all, in meanwhile we should set PVR to the
>>>>> destination host PVR value sometime after all these pvr_match() checks, 
>>>>> the
>>>>> place above was just my first guess (which is usually incorrect, I know 
>>>>> :) )
>>>>>
>>>>
>>>> I have no problems with this solution - we can re-insert the line
>>>>
>>>>         env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
>>>>
>>>>
>>>> after all the pvr_checks were made.
>>>>
>>>> Is this an acceptable workaround or do we need to dig further? Alexey, do
>>>> you want to send this patch?
>>>
>>> I'm very uneasy with this approach.  For one thing it relies on
>>> knowing if we're using HV or PR KVM, which we should be avoiding.
>>
>> In this matter they are quite different - one can emulate PVR, the other
>> cannot. I'd say we need yet another KVM capability, like KVM_CAP_PPC_PVR or
>> KVM_CAP_PPC_COMPAT and act different in QEMU.
> 
> Hmm.  Well, we could already tell the difference in qemu by seeing if
> the PVR we get back from KVM is the one we put into it.
> 
>>> But more importantly, it denies the kernel an opportunity to assess if
>>> the CPU it's hosting on really is a good enough approximation of the
>>> one that qemu really wants.
>>
>> The assessment is done by the KVM_REG_PPC_ARCH_COMPAT handler in HV KVM,
>> looks sufficient.
> 
> Well, except apparently it isn't since it's rejecting the legitimate
> case of a POWER8 to POWER8E migration.

KVM_REG_PPC_ARCH_COMPAT does not reject it, it works fine. Setting exact
PVR via KVM_SET_SREGS fails, this is different.


>>> My intention here - right since the early days of KVM HV, was that
>>> qemu would always put the PVR it really wanted into the sregs field.
>>> KVM PR would then emulate a specific CPU based on that.  HV, not
>>> having that option, would instead assess if the requested CPU was
>>> close enough to the host CPU to get away with it.
>>>
>>> But I guess, since qemu's haphazard handling of compat modes meant
>>> that didn't really happen until now, we never properly tested that.
>>>
>>> So, really I think the correct fix is in the kernel - to make it
>>> accept a POWER8E PVR when hosted on a POWER8 and vice versa.
>>
>> What will we do for the power8 -> power9 migration?
> 
> Same thing, ideally.  We know POWER9 has a POWER8 compatibility mode,
> so it's acceptable to set a POWER8 PVR on a POWER9 host.


Set P8's PVR to HV KVM on a P9 host, then read it back and see it is still
P9's PVR - what is the point in such API? Or the point is in avoiding
having differentiation between PR and HV in QEMU? :)


> Cross-checking against what userspace has set the compatibility mode
> to is optional.
> 
>>>  But,
>>> given the kernels that are out there, I guess we'll need a workaround
>>> in qemu as well.  Ugh.
>>>
>>
>>
> 
> 
> 
> 


-- 
Alexey

signature.asc
Description: OpenPGP digital signature