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

[Paolo Bonzini]

On 15/09/2018 22:48, Liran Alon wrote:
> 
> 
>> 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).

It's much less smarter than that. :)  If env.nested_state_len is 12345,
it reads 12345 bytes.  There's no attempt at type checking.

> Is there a simple way to do this in QEMU’s VMSTATE framework without 
> implementing custom save/load callbacks?

You can store the source code size and use VMSTATE_VALIDATE to test it
against the KVM capability.


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

There is already a "hypervisor" (TCG) that implements AMD nested paging,
so it makes sense to reuse the place that TCG uses for its hflags.

Paolo