diff mbox series

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

Message ID 20221130233650.1404148-3-seanjc@google.com
State New
Headers
Series x86/crash: Fix double NMI shootdown bug |

Commit Message

Sean Christopherson Nov. 30, 2022, 11:36 p.m. UTC 
  Set GIF=1 prior to disabling SVM to ensure that INIT is recognized if the
kernel is disabling SVM in an emergency, e.g. if the kernel is about to
jump into a crash kernel or may reboot without doing a full CPU RESET.
If GIF is left cleared, the new kernel (or firmware) will be unabled to
awaken APs.  Eat faults on STGI (due to EFER.SVME=0) as it's possible
that SVM could be disabled via NMI shootdown between reading EFER.SVME
and executing STGI.

Link: https://lore.kernel.org/all/cbcb6f35-e5d7-c1c9-4db9-fe5cc4de579a@amd.com
Cc: stable@vger.kernel.org
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/virtext.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Sean Christopherson Dec. 1, 2022, 11:04 p.m. UTC | #1 
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
diff mbox series

Patch

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.
+		 */
+		asm_volatile_goto("1: stgi\n\t"
+				  _ASM_EXTABLE(1b, %l[fault])
+				  ::: "memory" : fault);
+fault:
+		wrmsrl(MSR_EFER, efer & ~EFER_SVME);
+	}
 }
 
 /** Makes sure SVM is disabled, if it is supported on the CPU