Re: [PATCH v2] x86/vmx: save guest non-register state in hvm_hw_cpu

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Mon, Mar 21, 2022 at 8:16 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 17.03.2022 16:57, Tamas K Lengyel wrote:
> > During VM forking and resetting a failed vmentry has been observed due
> > to the guest non-register state going out-of-sync with the guest register
> > state. For example, a VM fork reset right after a STI instruction can 
> > trigger
> > the failed entry. This is due to the guest non-register state not being 
> > saved
> > from the parent VM, thus the reset operation only copies the register state.
> >
> > Fix this by including the guest non-register state in hvm_hw_cpu so that 
> > when
> > its copied from the parent VM the vCPU state remains in sync.
> >
> > SVM is not currently wired-in as VM forking is VMX only and saving 
> > non-register
> > state during normal save/restore/migration operation hasn't been needed.
>
> Given that it was pointed out that e.g. STI- and MOV-SS-shadow aren't
> VMX specific and also aren't impossible to hit with ordinary save /
> restore / migrate, I'm not convinced of this argumentation. But of
> course fixing VMX alone is better than nothing. However, ...
>
> > @@ -166,6 +167,11 @@ struct hvm_hw_cpu {
> >  #define XEN_X86_FPU_INITIALISED         (1U<<_XEN_X86_FPU_INITIALISED)
> >      uint32_t flags;
> >      uint32_t pad0;
> > +
> > +    /* non-register state */
> > +    uint32_t activity_state;
> > +    uint32_t interruptibility_state;
> > +    uint64_t pending_dbg;
> >  };
>
> ... these fields now represent vendor state in a supposedly vendor
> independent structure. Besides my wish to see this represented in
> field naming (thus at least making provisions for SVM to gain
> similar support; perhaps easiest would be to include these in a
> sub-structure with a field name of "vmx"), I wonder in how far cross-
> vendor migration was taken into consideration. As long as the fields
> are zero / ignored, things wouldn't be worse than before your
> change, but I think it wants spelling out that the SVM counterpart(s)
> may not be added by way of making a VMX/SVM union.

I wasn't aware cross-vendor migration is even a thing. But adding a
vmx sub-structure seems to me a workable route, we would perhaps just
need an extra field that specifies where the fields were taken
(vmx/svm) and ignore them if the place where the restore is taking
place doesn't match where the save happened. That would be equivalent
to how migration works today. Thoughts?

Tamas