Re: [PATCH] x86/hvm: Include interruptibility state in hvm_hw_cpu

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2022 19:44, Tamas K Lengyel wrote:
> During VM fork resetting a failed vmentry has been observed when the reset
> is performed immediately after a STI instruction executed. This is due to
> the guest interruptibility state in the VMCS being modified by STI but the
> subsequent reset removes the IF bit from FLAGS, causing the failed vmentry.

I first thought this was backwards, but after re-reading a couple of
times I think the issue is merely with you stating this as if this
was what always happens, while it really depends on the state that
the VM is being reset to. I think it would further be helpful if you
made clear that other interruptibility state could also cause issues
when not properly restored. One way to express this would be to
simply insert "e.g." ahead of "a STI instruction".

> @@ -1155,6 +1154,8 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>      v->arch.dr6   = ctxt.dr6;
>      v->arch.dr7   = ctxt.dr7;
>  
> +    hvm_set_interrupt_shadow(v, ctxt.interruptibility_info);

Setting reserved bits as well as certain combinations of bits will
cause VM entry to fail. I think it would be nice to report this as
an error here rather than waiting for the VM entry failure.

> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -720,6 +720,22 @@ static inline int hvm_vmtrace_reset(struct vcpu *v)
>      return -EOPNOTSUPP;
>  }
>  
> +static inline unsigned long hvm_get_interrupt_shadow(struct vcpu *v)

unsigned long here and ...

> +{
> +    if ( hvm_funcs.get_interrupt_shadow )
> +        return alternative_call(hvm_funcs.get_interrupt_shadow, v);
> +
> +    return -EOPNOTSUPP;
> +}
> +
> +static inline void
> +hvm_set_interrupt_shadow(struct vcpu *v, unsigned long val)

... here are not in line with the hooks' types. Same for the stubs
further down then.

> +{
> +    if ( hvm_funcs.set_interrupt_shadow )
> +        alternative_vcall(hvm_funcs.set_interrupt_shadow, v, val);
> +}
> +
> +
>  /*

Please don't insert multiple successive blank lines.

Jan