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

[Liran Alon]

> On 8 Nov 2018, at 19:02, Paolo Bonzini <address@hidden> wrote:
> 
> On 08/11/2018 10:57, Liran Alon wrote:
>> 
>> 
>>> On 8 Nov 2018, at 11:50, Paolo Bonzini <address@hidden> wrote:
>>> 
>>> On 08/11/2018 01:45, Jim Mattson wrote:
>>>> I have no attachments to the current design. I had used a data[] blob,
>>>> because I didn't think userspace would have any need to know what was
>>>> in there. However, I am now seeing the error of my ways. For example,
>>>> the userspace instruction emulator needs to know the contents of the
>>>> vmcs12 to emulate instructions when in guest mode.
>>> 
>>> Yeah, we're probably going to have to document the KVM vmcs12 structure,
>>> possibly moving it to uapi.  But that's a different thing from
>>> save/restore state, which can use the 4K or 8K data[] blob.
>>> 
>>> Paolo
>> 
>> But regardless of if we document vmcs12 or not, the current blob we
>> have today should be separated to well-defined blobs/structs (cached_vmcs12
>> and cached_shadow_vmcs12) and each blob should have a relevant flag that 
>> specifies
>> it is valid (saved by kernel or requested to be restored by userspace).
>> Additional future nested-state should be added as additional
>> well-defined blobs/structs with appropriate flags.
> 
> We are somewhat limited by the ABI of the released kernel versions, and
> it's unlikely that the structure will change in the short term.  So I
> think it's okay if we just define that the first 4K of data are the VMCS
> and the second 8K are the shadow VMCS, whenever format=0 in the
> kvm_nested_state struct.
> 
> Paolo

So what I plan to do is indeed to define first 4K of data as vmcs12 and next 4K 
as shadow_vmcs12.
I will also define each of them in a separate VMState subsection that each will 
have it’s own .needed()
method that will decide if it’s relevant to send it based on kvm_state.size.
vmcs12 and shadow_vmcs12 will be put in a struct which is unioned with a struct
of 2 pages buffer to clearly indicate that one well-defined struct is used for 
VMX and the other for SVM.
(based on kvm_state.format)

In addition, I will change KVM_{GET,SET}_NESTED_STATE documentation to indicate 
that
future nested state fields should be added at the end of struct and weather 
they are valid should
be determined by a flag in kvm_state.flags.
QEMU will handle these new states in the future by adding more VMState 
subsections that have
.needed() method based on appropriate flag in kvm_state.flags.
The struct itself that userspace use against it’s local kernel will be 
determined based on KVM_CAPs.

If there are no objections, I will write the needed patches for QEMU (and the 
documentation patch for KVM).
(And throw away the current VMState technique I used in this version of the 
patch :P)

-Liran

> 
>> Then, in QEMU, each such well-defined blob/struct should have it’s own 
>> subsection with a relevant .needed() method.
>> This will allow us to preserve required backwards compatibility.
>> 
>> Agreed?
>> 
>> -Liran
>> 
>