On 14/09/2018 11:54, Liran Alon wrote:
>> 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?
By defining more HF flags. I'd rather avoid having multiple ways to
store the same thing, in case for example in the future HVF implements
nested virt.
> 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.
It is indeed simpler, but it adds unnecessary KVM specificities.
>>> +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.
You're right (I got confused with my implementation).
There is still a problem, however, in that the same input stream would
be parsed differently depending on the kernel version. In particular,
if in the future the maximum nested state size grows, you break all
existing save files.
Paolo
