Re: [PATCH v4 2/4] x86/virt: Force GIF=1 prior to disabling SVM (for reboot flows)

Sean Christopherson
2022-12-01T23:04:20Z

On Thu, Dec 01, 2022, Andrew Cooper wrote:
> On 30/11/2022 23:36, Sean Christopherson wrote:
> > diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> > index 8757078d4442..0acb14806a74 100644
> > --- a/arch/x86/include/asm/virtext.h
> > +++ b/arch/x86/include/asm/virtext.h
> > @@ -126,7 +126,18 @@ static inline void cpu_svm_disable(void)
> >
> > wrmsrl(MSR_VM_HSAVE_PA, 0);
> > rdmsrl(MSR_EFER, efer);
> > - wrmsrl(MSR_EFER, efer & ~EFER_SVME);
> > + if (efer & EFER_SVME) {
> > + /*
> > + * Force GIF=1 prior to disabling SVM, e.g. to ensure INIT and
> > + * NMI aren't blocked. Eat faults on STGI, as it #UDs if SVM
> > + * isn't enabled and SVM can be disabled by an NMI callback.
>
> I'd be tempted to tweak this for clarity.
>
> How about "We don't know the state of GIF, and if NMIs are enabled,
> there is a race condition where EFER.SVME can be cleared behind our
> back.  Ignore #UD, and force GIF=1 in case INIT/NMI are currently
> blocked."  ?
>
> The STGI can't actually #UD on real hardware, because SKINIT and SVM
> exist in identical sets of parts, but it can #UD in principle in a VM
> which doesn't offer emulate SKINIT.

Ah, right, "may #UD", not "will #UD". And despite writing this, I also keep
forgetting why forcing GIF is even necessary. How about?

/*
* Force GIF=1 prior to disabling SVM to ensure INIT and NMI
* aren't blocked, e.g. if a fatal error occurred between CLGI
* and STGI. Note, STGI may #UD if SVM is disabled from NMI
* context between reading EFER and executing STGI. In that
* case, GIF must already be set, otherwise the NMI would have
* been blocked, so just eat the fault.
*/

> Given that we are in cpu_svm_disable(), there's also
> MSR_VM_CR.INIT_REDIRECTION to consider, but perhaps that's better left
> to the series which adds SKINIT support.
>
> ~Andrew