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

[Paolo Bonzini <pbonzini@redhat.com>]

On 14/09/2018 16:31, Liran Alon wrote:
>> 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.
> 
> I’m not sure I agree with this.
> 1) Newer kernels should change struct only by utilizing unused fields in current struct
> or enlarging it with extra fields. It should not change the meaning of existing fields.

Newer kernels will return a larger size, which is stored in
env->nested_state_len, and the file format depends on it:

> +        VMSTATE_VBUFFER_UINT32(env.nested_state, X86CPU, 
> +                               0, NULL, 
> +                               env.nested_state_len), 

>>
>> 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.
> 
> I agree it is possible to define more hflags and to translate kvm_nested_flags->flags to flags in hflags and vice-versa.
> However, I am not sure I understand the extra value provided by doing so.
> I think it makes more sense to rename struct kvm_nested_state to struct nested_state and
> use this as a generic interface to get/set nested_state for all hypervisors.
> If a given hypervisor, such as HVF, needs to store different blob than KVM VMX, it can just add another struct field to
> the union-part of struct kvm_nested_state.
> This way, the code that handles the save/restore of nested_state can remain generic for all hypervisors.

I agree that the value is small, but it's there.  For example, the
debugging commands support AMD nested paging, and sharing the flags
means that it works for KVM too, not just TCG.  So I'd prefer to do it
the same way for Intel too.

Paolo