Re: [PATCH v5] x86/vmx: add hvm functions to get/set non-register state

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Apr 7, 2022 at 11:49 PM Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
>
> > From: Lengyel, Tamas <tamas.lengyel@xxxxxxxxx>
> > Sent: Friday, March 25, 2022 9:33 PM
> >
> > 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 adding a new pair of hvm functions to get/set the guest
> > non-register state so that the overall vCPU state remains in sync.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > ---
> > v5: Switch to internal-only hvm funcs instead of adding to hvm_hw_cpu
> > ---
> >  xen/arch/x86/hvm/vmx/vmx.c         | 32 ++++++++++++++++++++++++
> >  xen/arch/x86/include/asm/hvm/hvm.h | 40
> > ++++++++++++++++++++++++++++++
> >  xen/arch/x86/mm/mem_sharing.c      | 11 +++++++-
> >  3 files changed, 82 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index c075370f64..2685da16c8 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -1334,6 +1334,36 @@ static void cf_check vmx_set_interrupt_shadow(
> >      __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
> >  }
> >
> > +static void cf_check vmx_get_nonreg_state(struct vcpu *v,
> > +    struct hvm_vcpu_nonreg_state *nrs)
> > +{
> > +    vmx_vmcs_enter(v);
> > +
> > +    __vmread(GUEST_ACTIVITY_STATE, &nrs->vmx.activity_state);
> > +    __vmread(GUEST_INTERRUPTIBILITY_INFO, &nrs-
> > >vmx.interruptibility_info);
> > +    __vmread(GUEST_PENDING_DBG_EXCEPTIONS, &nrs->vmx.pending_dbg);
> > +
> > +    if ( cpu_has_vmx_virtual_intr_delivery )
> > +        __vmread(GUEST_INTR_STATUS, &nrs->vmx.interrupt_status);
>
> There lacks of explanation somewhere how those states are selected.
> Your discussion with Andrew leaves me the impression that Andrew sees
> more issues in general save/restore path while you only want to deal with
> the requirements for your own usage. But according to v1 your usage only
> cares about the interruptiblity info. This implies that v5 is kind of in a 
> state
> between your original intention and what Andrew actually wants...

These fields are all guest non-register states so they are not
completely arbitrary. True that at v1 only the interruptibility info
was observed to be causing issues when it goes out-of-sync after a
reset. Since then pending_dbg was also noted to be needing a reset
under some circumstances. So at this point I see no reason to wait to
include the other values in the reset. If you have an insight into why
those fields don't need to be kept in sync with the rest of the vCPU
state, please share.

As for the save/restore path concerns I don't really have a clear
insight into what is needed to fix it. Furthermore the proposed sanity
checking on these values that would be legitimately needed for
save/restore are just pure overhead for our use-case. So the two paths
are better left separate in any case.

Tamas