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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

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

Thanks
Kevin