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

[Dr. David Alan Gilbert]

* Liran Alon (address@hidden) wrote:
> 
> 
> > 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.
> > 

(Sorry, I was out last week).

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

Using subsections is good and makes it pretty clear on loading if
there's a problem; it also makes it good when you add new subsections
in new versions.  It can be tricky getting the logic for the .needed
functions right, so that you don't write out new .needed sections for
old configs and thus can allow yourself to migrate to an older
qemu/kernel.

Dave

> -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
> >> 
> > 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK