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

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

On Thu, Mar 17, 2022 at 9:56 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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".

Correct, there could be other instances where the interruptibility
state could go out of sync with RFLAGS, executing STI and then
resetting only the register state to the pre-STI parent is just one I
stumbled into.

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

Not sure if this would be the right spot to do that since that's all
VMX specific and this is the common hvm code.

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

Ack.

Tamas