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

[Liran Alon]

> On 14 Sep 2018, at 18:08, Paolo Bonzini <address@hidden> wrote:
> 
> 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), 
> 

Oh. I thought that QEMU will just receive to dest buffer only what was sent 
from source buffer.
I didn’t know that it also enforces that the sizes of the source and dest 
buffer are equal.
(I thought that dest_buffer_size only needed to be >= src_buffer_size).

Anyway, my intention here was that QEMU will only enforce (dest_buffer_size >= 
source_buffer_size)
and if so, receive source buffer into destination buffer.
Is there a simple way to do this in QEMU’s VMSTATE framework without 
implementing custom save/load callbacks?

>>> 
>>> 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.

Can you explain this example? I’m not sure I follow.
In the solution I proposed above, the nested_state fixed-size header is also 
shared between hypervisors.
Thus, flags here are shared exactly the same as hflags are shared.
Only the blob union-part is not necessarely shared between hypervisors (Which 
makes sense as it may differ). 
Am I missing something trivial?

-Liran

> 
> Paolo