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

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

On Thu, Mar 17, 2022, 2:09 AM Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:

> From: Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>
> Sent: Monday, March 14, 2022 8:14 PM
>
> On Mon, Mar 14, 2022 at 3:22 AM Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
> >
> > > From: Lengyel, Tamas <tamas.lengyel@xxxxxxxxx>
> > > Sent: Friday, March 11, 2022 2:45 AM
> > >
> > > 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 didn't get the rationale here. Before this patch the interruptibility state is
> > not saved/restored thus I suppose after reset it will be cleared thus aligned
> > with RFLAGS.IF=0. Can you elaborate a bit how exactly above problem is
> > caused?
>
> The problem is that the interruptibility state is not cleared and thus
> isn't aligned with RFLAGS.IF=0 after RFLAGS is reset. They go out of
> sync leading to the failed vmentry. The interruptibility state needs
> to be included in the hvm hw cpu struct for it to get re-aligned
> during reset to avoid the failed vmentry.

I'm still confused here. The interruptibility state should have bit 0 as 1
after a STI instruction is executed (RFLAGS.IF=1). Saving/restoring it
still doesn't match RFLAGS.IF=0 after vm fork reset. So I didn't understand
how this patch actually fixes the problem.

I think I see where the confusion is. We are saving the context of the parent vm and restoring it in the fork during a reset. That's what a reset is. So by including the field in the struct means it will be reset to be in sync with RFLAGS of the parent vm. Right now only the RFLAGS is copied from the parent and interruptibility state isn't touched at all.

Also if there is a real problem shouldn't we just reset the interruptbility
state to match RFLAGS.IF=0?

Yes, exactly what this patch achieves. Looking at it more I think the rest of the non-register cpu state should similarly be included so those would get reset too (activity & pending dbg).

>
> >
> > >
> > > Include the interruptibility state information in the public hvm_hw_cpu
> struct
> > > so that the CPU can be safely saved/restored.
> > >
> > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > > ---
> > >  xen/arch/x86/hvm/hvm.c                 |  9 +++++----
> > >  xen/arch/x86/hvm/vmx/vmx.c             |  4 ++++
> > >  xen/arch/x86/include/asm/hvm/hvm.h     | 26
> >
> > Why is this change only applied to vmx instead of svm?
>
> VM forking is implemented only for vmx, thus this change is only
> relevant where a VM would be immediately reset after a STI

but the ops is generic and SVM already has the related callbacks.

> instruction. Normal VM save/restore/migration doesn't attempt to
> capture a VM state immediately after STI thus it's not relevant for
> SVM.
>

Can you elaborate why save/restore/migration won't happen
right after STI while it does for vm fork?

This is just based on my observation that noone has encountered this issue in the long life of Xen. If I'm wrong and this cornercase could be encountered during normal routes I can wire in SVM too. I ran into this with vm forks because we are resetting the forks very very often (thousands of times per second) under various execution paths with the fuzzer and one happened to hit reset just after STI.

Another question I would be interested to hear from the maintainers is in regards to the hvm context compat macros. Right now they differentiate between hvm hw cpu struct versions based on size. So since this patch doesn't change the size how is that supposed to work? Or if there are more then two versions of the struct? The compat version never changes?

Tamas