Re: [Qemu-devel] [PATCH 2/2] KVM: i386: Add support for save and restore nested state

[Liran Alon]

>On 14/09/2018 09:16, Paolo Bonzini wrote:
>Heh, I was going to send a similar patch.  However, things are a bit
>more complex for two reason.
>
>First, I'd prefer to reuse the hflags and hflags2 fields that we already
>have, and only store the VMCS blob in the subsection.  For example,
>HF_SVMI_MASK is really the same as HF_GUEST_MASK in KVM source code and
>KVM_STATE_NESTED_GUEST_MODE in the nested virt state.
>

Do you mean you intend to only save/restore the “vmx” field in struct 
kvm_nested_state?
(That is, struct kvm_vmx_nested_state)
If yes, that is problematic as kvm_nested_state.flags also hold other flags 
besides KVM_STATE_NESTED_GUEST_MODE.
How do you expect to save/restore for example the 
vmx->nested.nested_run_pending flag that is specified in 
KVM_STATE_NESTED_RUN_PENDING?

In addition, why is it important to avoid save/restore the entire 
kvm_nested_state struct?
It seems to simplify things to just save/restore the entire struct.

>More important, this:
>
>>
>> +static int nested_state_post_load(void *opaque, int version_id)
>> +{
>> +    X86CPU *cpu = opaque;
>> +    CPUX86State *env = &cpu->env;
>> +
>> +    /*
>> +     * Verify that the size specified in given struct is set
>> +     * to no more than the size that our kernel support
>> +     */
>> +    if (env->nested_state->size > env->nested_state_len) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static bool nested_state_needed(void *opaque)
>
>doesn't work if nested_state_len differs between source and destination,
>and could overflow the nested_state buffer if nested_state_len is larger
>on the source.

This is not accurate.
I have actually given a lot of thought to this aspect in the patch.

The above post_load() method actually prevents an overflow to happen on dest.
Note that env->nested_state_len is not passed as part of migration stream.
It is only set by local kernel KVM_CAP_NESTED_STATE.

Therefore, we allow the following migrations to execute successfully:
1) Migration from a source with smaller KVM_CAP_NESTED_STATE to dest with a 
bigger one.
The above post_load() check will succeed as size specified in migrated 
nested_state->size
is smaller than dest KVM_CAP_NESTED_STATE (stored in env->nested_state_len).
2) Migration from source to dest when they both have same KVM_CAP_NESTED_STATE 
size.
Obvious.
3) Migration from source to dest when source have a bigger KVM_CAP_NESTED_STATE 
than dest.
This will only succeed in case size specified in nested_state->size is smaller 
than dest KVM_CAP_NESTED_STATE.

-Liran

>
>I'll send my version today or next Monday.
>
>Thanks,
>
>Paolo
>