[v4] x86/crash: Fix double NMI shootdown bug

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

Posted by Sean Christopherson 2 years, 9 months ago

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(-)

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
-- 
2.38.1.584.g0f3c55d4c2-goog

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

Posted by Andrew Cooper 2 years, 9 months ago

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.

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

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

Posted by Sean Christopherson 2 years, 9 months ago

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

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

Posted by Andrew Cooper 2 years, 9 months ago

On 01/12/2022 23:04, Sean Christopherson wrote:
> 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.
> 		 */

LGTM.

~Andrew