Re: [PATCH] target/i386: kvm: Fix when nested state is needed for migration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 4 Jul 2019, at 18:29, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> 
> On 04/07/19 16:31, Liran Alon wrote:
>> 
>> 
>>> On 2 Jul 2019, at 19:39, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>>> 
>>> On 25/06/19 01:05, Liran Alon wrote:
>>>> When vCPU is in VMX operation and enters SMM mode,
>>>> it temporarily exits VMX operation but KVM maintained nested-state
>>>> still stores the VMXON region physical address, i.e. even when the
>>>> vCPU is in SMM mode then (nested_state->hdr.vmx.vmxon_pa != -1ull).
>>>> 
>>>> Therefore, there is no need to explicitly check for
>>>> KVM_STATE_NESTED_SMM_VMXON to determine if it is necessary
>>>> to save nested-state as part of migration stream.
>>>> 
>>>> In addition, destination must enable eVMCS if it is enabled on
>>>> source as specified by the KVM_STATE_NESTED_EVMCS flag, even if
>>>> the VMXON region is not set. Thus, change the code to require saving
>>>> nested-state as part of migration stream in case it is set.
>>>> 
>>>> Reviewed-by: Karl Heubaum <karl.heubaum@xxxxxxxxxx>
>>>> Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
>>>> ---
>>>> target/i386/machine.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/target/i386/machine.c b/target/i386/machine.c
>>>> index 851b249d1a39..e7d72faf9e24 100644
>>>> --- a/target/i386/machine.c
>>>> +++ b/target/i386/machine.c
>>>> @@ -999,7 +999,7 @@ static bool vmx_nested_state_needed(void *opaque)
>>>> 
>>>>    return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
>>>>            ((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
>>>> -             (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
>>>> +             (nested_state->flags & KVM_STATE_NESTED_EVMCS)));
>>>> }
>>>> 
>>>> static const VMStateDescription vmstate_vmx_nested_state = {
>>>> 
>>> 
>>> Queued, thanks.
>>> 
>>> Paolo
>> 
>> Actually Paolo after I have created KVM patch
>> ("KVM: nVMX: Change KVM_STATE_NESTED_EVMCS to signal vmcs12 is copied from eVMCS”)
>> I think I realised that KVM_STATE_NESTED_EVMCS is actually not a requirement for nested-state to be sent.
>> I suggest to replace this commit with another one that just change vmx_nested_state_needed() to return true
>> In case format is FORMAT_VMX and vmxon_pa != -1ull and that’s it.
>> 
>> As anyway, QEMU provisioned on destination side is going to enable the relevant eVMCS capability.
>> I’m going to send another series that refines QEMU nested-migration a bit more so I will do it along the way.
>> But I think this patch should be un-queued. Sorry for realizing this later but at least it’s before it was merged to master :)
> 
> Replaced with
> 
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 851b249d1a..704ba6de46 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -997,9 +997,8 @@ static bool vmx_nested_state_needed(void *opaque)
> {
>     struct kvm_nested_state *nested_state = opaque;
> 
> -    return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
> -            ((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
> -             (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
> +    return (nested_state->format == KVM_STATE_NESTED_FORMAT_VMX &&
> +            nested_state->hdr.vmx.vmxon_pa != -1ull);
> }
> 
> static const VMStateDescription vmstate_vmx_nested_state = {
> 
> and dropped the last paragraph of the commit message.
> 
> Paolo
> 

Yep that’s what I wanted to do.
Thanks. I have some more patches pending anyway but unrelated to this. This now seems good to me.

-Liran






[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux