Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Huang, Kai]

On Tue, 2024-04-09 at 14:29 +0300, Kirill A. Shutemov wrote:
> Depending on setup, TDX guests might be allowed to clear CR4.MCE.
> Attempt to clear it leads to #VE.
> 
> Use alternatives to keep the flag during kexec for TDX guests.
> 
> The change doesn't affect non-TDX-guest environments.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Sean Christopherson]

On Tue, Apr 09, 2024, Kirill A. Shutemov wrote:
> Depending on setup, TDX guests might be allowed to clear CR4.MCE.
> Attempt to clear it leads to #VE.
> 
> Use alternatives to keep the flag during kexec for TDX guests.
> 
> The change doesn't affect non-TDX-guest environments.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/kernel/relocate_kernel_64.S | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> index 56cab1bb25f5..8e2037d78a1f 100644
> --- a/arch/x86/kernel/relocate_kernel_64.S
> +++ b/arch/x86/kernel/relocate_kernel_64.S
> @@ -5,6 +5,8 @@
>   */
>  
>  #include <linux/linkage.h>
> +#include <linux/stringify.h>
> +#include <asm/alternative.h>
>  #include <asm/page_types.h>
>  #include <asm/kexec.h>
>  #include <asm/processor-flags.h>
> @@ -145,11 +147,17 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>  	 * Set cr4 to a known state:
>  	 *  - physical address extension enabled
>  	 *  - 5-level paging, if it was enabled before
> +	 *  - Machine check exception on TDX guest, if it was enabled before.
> +	 *    Clearing MCE might not allowed in TDX guests, depending on setup.
>  	 */
>  	movl	$X86_CR4_PAE, %eax
>  	testq	$X86_CR4_LA57, %r13
>  	jz	1f
>  	orl	$X86_CR4_LA57, %eax
> +1:
> +	testq	$X86_CR4_MCE, %r13
> +	jz	1f
> +	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST

The TEST+Jcc+OR sequences are rather odd, and require way more instructions and
thus way more copy+paste than is necessary.

	movl	$X86_CR4_LA57, %eax
	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
	andl	%r13d, %eax
	orl	$X86_CR4_PAE, %eax
	movq	%rax, %cr4

Then preserving new bits unconditionally only requires adding the flag to the
initial move, and feature-dependent bits only need a single ALTERNATIVE line.

And there's no branches, blazing fast kexec! ;-)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Kirill A. Shutemov]

On Tue, Apr 09, 2024 at 07:22:24AM -0700, Sean Christopherson wrote:
> On Tue, Apr 09, 2024, Kirill A. Shutemov wrote:
> > Depending on setup, TDX guests might be allowed to clear CR4.MCE.
> > Attempt to clear it leads to #VE.
> > 
> > Use alternatives to keep the flag during kexec for TDX guests.
> > 
> > The change doesn't affect non-TDX-guest environments.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  arch/x86/kernel/relocate_kernel_64.S | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> > index 56cab1bb25f5..8e2037d78a1f 100644
> > --- a/arch/x86/kernel/relocate_kernel_64.S
> > +++ b/arch/x86/kernel/relocate_kernel_64.S
> > @@ -5,6 +5,8 @@
> >   */
> >  
> >  #include <linux/linkage.h>
> > +#include <linux/stringify.h>
> > +#include <asm/alternative.h>
> >  #include <asm/page_types.h>
> >  #include <asm/kexec.h>
> >  #include <asm/processor-flags.h>
> > @@ -145,11 +147,17 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
> >  	 * Set cr4 to a known state:
> >  	 *  - physical address extension enabled
> >  	 *  - 5-level paging, if it was enabled before
> > +	 *  - Machine check exception on TDX guest, if it was enabled before.
> > +	 *    Clearing MCE might not allowed in TDX guests, depending on setup.
> >  	 */
> >  	movl	$X86_CR4_PAE, %eax
> >  	testq	$X86_CR4_LA57, %r13
> >  	jz	1f
> >  	orl	$X86_CR4_LA57, %eax
> > +1:
> > +	testq	$X86_CR4_MCE, %r13
> > +	jz	1f
> > +	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
> 
> The TEST+Jcc+OR sequences are rather odd, and require way more instructions and
> thus way more copy+paste than is necessary.
> 
> 	movl	$X86_CR4_LA57, %eax
> 	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
> 	andl	%r13d, %eax
> 	orl	$X86_CR4_PAE, %eax
> 	movq	%rax, %cr4
> 
> Then preserving new bits unconditionally only requires adding the flag to the
> initial move, and feature-dependent bits only need a single ALTERNATIVE line.

Thanks! It is much better.

> And there's no branches, blazing fast kexec! ;-)

kexec/sec STONKS! :D

Updated patch is below.

From 6be428e3b1c6fb494b2c48ba6a7c133514a0b2b4 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Fri, 10 Feb 2023 12:53:11 +0300
Subject: [PATCHv10.1 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

Depending on setup, TDX guests might be allowed to clear CR4.MCE.
Attempt to clear it leads to #VE.

Use alternatives to keep the flag during kexec for TDX guests.

The change doesn't affect non-TDX-guest environments.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/kernel/relocate_kernel_64.S | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 56cab1bb25f5..90246d544eb1 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -5,6 +5,8 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/stringify.h>
+#include <asm/alternative.h>
 #include <asm/page_types.h>
 #include <asm/kexec.h>
 #include <asm/processor-flags.h>
@@ -143,14 +145,15 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 
 	/*
 	 * Set cr4 to a known state:
-	 *  - physical address extension enabled
 	 *  - 5-level paging, if it was enabled before
+	 *  - Machine check exception on TDX guest, if it was enabled before.
+	 *    Clearing MCE might not allowed in TDX guests, depending on setup.
+	 *  - physical address extension enabled
 	 */
-	movl	$X86_CR4_PAE, %eax
-	testq	$X86_CR4_LA57, %r13
-	jz	1f
-	orl	$X86_CR4_LA57, %eax
-1:
+	movl	$X86_CR4_LA57, %eax
+	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
+	andl	%r13d, %eax
+	orl	$X86_CR4_PAE, %eax
 	movq	%rax, %cr4
 
 	jmp 1f
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Borislav Petkov]

On Tue, Apr 09, 2024 at 06:26:05PM +0300, Kirill A. Shutemov wrote:
> From 6be428e3b1c6fb494b2c48ba6a7c133514a0b2b4 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Fri, 10 Feb 2023 12:53:11 +0300
> Subject: [PATCHv10.1 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest
> 
> Depending on setup, TDX guests might be allowed to clear CR4.MCE.
> Attempt to clear it leads to #VE.
> 
> Use alternatives to keep the flag during kexec for TDX guests.
> 
> The change doesn't affect non-TDX-guest environments.

This is all fine and dandy but nothing explains *why* TDX needs this
special dance.

Why can't TDX do the usual CR4.MCE diddling like the normal kernel
during init and needs to do that here immediately?

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/kernel/relocate_kernel_64.S | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> index 56cab1bb25f5..90246d544eb1 100644
> --- a/arch/x86/kernel/relocate_kernel_64.S
> +++ b/arch/x86/kernel/relocate_kernel_64.S
> @@ -5,6 +5,8 @@
>   */
>  
>  #include <linux/linkage.h>
> +#include <linux/stringify.h>
> +#include <asm/alternative.h>
>  #include <asm/page_types.h>
>  #include <asm/kexec.h>
>  #include <asm/processor-flags.h>
> @@ -143,14 +145,15 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>  
>  	/*
>  	 * Set cr4 to a known state:
> -	 *  - physical address extension enabled
>  	 *  - 5-level paging, if it was enabled before
> +	 *  - Machine check exception on TDX guest, if it was enabled before.
> +	 *    Clearing MCE might not allowed in TDX guests, depending on setup.

			 ... might not be allowed ...

> +	 *  - physical address extension enabled
>  	 */
> -	movl	$X86_CR4_PAE, %eax
> -	testq	$X86_CR4_LA57, %r13
> -	jz	1f
> -	orl	$X86_CR4_LA57, %eax
> -1:
> +	movl	$X86_CR4_LA57, %eax
> +	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
> +	andl	%r13d, %eax

%r13 needs a comment here that it contains %cr4 read above in
relocate_kernel()

> +	orl	$X86_CR4_PAE, %eax
>  	movq	%rax, %cr4
>  
>  	jmp 1f
> -- 
>   Kiryl Shutsemau / Kirill A. Shutemov

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Kirill A. Shutemov]

On Sun, Apr 28, 2024 at 07:11:11PM +0200, Borislav Petkov wrote:
> On Tue, Apr 09, 2024 at 06:26:05PM +0300, Kirill A. Shutemov wrote:
> > From 6be428e3b1c6fb494b2c48ba6a7c133514a0b2b4 Mon Sep 17 00:00:00 2001
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > Date: Fri, 10 Feb 2023 12:53:11 +0300
> > Subject: [PATCHv10.1 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest
> > 
> > Depending on setup, TDX guests might be allowed to clear CR4.MCE.
> > Attempt to clear it leads to #VE.
> > 
> > Use alternatives to keep the flag during kexec for TDX guests.
> > 
> > The change doesn't affect non-TDX-guest environments.
> 
> This is all fine and dandy but nothing explains *why* TDX needs this
> special dance.
> 
> Why can't TDX do the usual CR4.MCE diddling like the normal kernel
> during init and needs to do that here immediately?

As I mentioned above, clearing CR4.MCE triggers #VE. It is quirk of the
platform.

There's plan to allow it in newer TDX modules, but kernel still has to
assume we cannot touch it in TDX guest case.

> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  arch/x86/kernel/relocate_kernel_64.S | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> > index 56cab1bb25f5..90246d544eb1 100644
> > --- a/arch/x86/kernel/relocate_kernel_64.S
> > +++ b/arch/x86/kernel/relocate_kernel_64.S
> > @@ -5,6 +5,8 @@
> >   */
> >  
> >  #include <linux/linkage.h>
> > +#include <linux/stringify.h>
> > +#include <asm/alternative.h>
> >  #include <asm/page_types.h>
> >  #include <asm/kexec.h>
> >  #include <asm/processor-flags.h>
> > @@ -143,14 +145,15 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
> >  
> >  	/*
> >  	 * Set cr4 to a known state:
> > -	 *  - physical address extension enabled
> >  	 *  - 5-level paging, if it was enabled before
> > +	 *  - Machine check exception on TDX guest, if it was enabled before.
> > +	 *    Clearing MCE might not allowed in TDX guests, depending on setup.
> 
> 			 ... might not be allowed ...
> 

Oopsie. Thanks.

> > +	 *  - physical address extension enabled
> >  	 */
> > -	movl	$X86_CR4_PAE, %eax
> > -	testq	$X86_CR4_LA57, %r13
> > -	jz	1f
> > -	orl	$X86_CR4_LA57, %eax
> > -1:
> > +	movl	$X86_CR4_LA57, %eax
> > +	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
> > +	andl	%r13d, %eax
> 
> %r13 needs a comment here that it contains %cr4 read above in
> relocate_kernel()

Okay.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Borislav Petkov]

On Mon, Apr 29, 2024 at 04:17:38PM +0300, Kirill A. Shutemov wrote:
> As I mentioned above, clearing CR4.MCE triggers #VE. It is quirk of the
> platform.

You mean when identity_mapped() runs as part of a kexec-ed kernel, it
might clear CR4.MCE and that would trigger the #VE?

So, if that is correct, you basically want to *preserve* the CR4.MCE
setting across kexec?

But then __mcheck_cpu_init_generic() will go and set it
unconditionally.

So what exactly is the correct flow here?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Kirill A. Shutemov]

On Mon, Apr 29, 2024 at 04:45:08PM +0200, Borislav Petkov wrote:
> On Mon, Apr 29, 2024 at 04:17:38PM +0300, Kirill A. Shutemov wrote:
> > As I mentioned above, clearing CR4.MCE triggers #VE. It is quirk of the
> > platform.
> 
> You mean when identity_mapped() runs as part of a kexec-ed kernel, it
> might clear CR4.MCE and that would trigger the #VE?

Yes, that's what happens in current upstream.

> So, if that is correct, you basically want to *preserve* the CR4.MCE
> setting across kexec?

Yes.

> But then __mcheck_cpu_init_generic() will go and set it
> unconditionally.

__mcheck_cpu_init_generic() will not change anything in this case as the
bit is already set. Everything is hunky-dory.

> So what exactly is the correct flow here?

TDX guest has CR4.MCE set from time 0 and it has to stay this way all the
time including kexec flow.

We have already modified early boot code to preserve CR4.MCE. See
77a512e35db7 ("x86/boot: Avoid #VE during boot for TDX platforms").
The patch extends it to kexec flow.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Borislav Petkov]

On Mon, Apr 29, 2024 at 06:16:54PM +0300, Kirill A. Shutemov wrote:
> Yes, that's what happens in current upstream.

Let's rewrite that commit message then:

"TDX guests run with MCA enabled (CR4.MCE=1b) from the very start. If
that bit is cleared during CR4 register reprogramming during boot or
kexec flows, a #VE exception will be raised which the guest kernel
cannot handle that early.

Therefore, make sure the CR4.MCE setting is preserved over kexec too and
avoid raising any #VEs."

without that bit about TDX guests might be allowed to clear CR4.MCE
which is only confusing and unnecessary.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:29:57PM +0300, Kirill A. Shutemov wrote:
> +1:
> +	testq	$X86_CR4_MCE, %r13
> +	jz	1f
> +	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
>  1:

Please add the below patch to your set. Those same-number labels are
just abominable.

Thx.

---
From: "Borislav Petkov (AMD)" <bp@alien8.de>
Date: Tue, 30 Apr 2024 15:00:16 +0200
Subject: [PATCH] x86/relocate_kernel: Use named labels for less confusion

That identity_mapped() function was loving that "1" label to the point
of completely confusing its readers.

Use named labels in each place for clarity.

No functional changes.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/kernel/relocate_kernel_64.S | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 8e2037d78a1f..0077c9e562a7 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -152,13 +152,15 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	 */
 	movl	$X86_CR4_PAE, %eax
 	testq	$X86_CR4_LA57, %r13
-	jz	1f
+	jz	no_la57
 	orl	$X86_CR4_LA57, %eax
-1:
+no_la57:
+
 	testq	$X86_CR4_MCE, %r13
-	jz	1f
+	jz	mca_off
 	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
-1:
+mca_off:
+
 	movq	%rax, %cr4
 
 	jmp 1f
@@ -173,9 +175,9 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	 * used by kexec. Flush the caches before copying the kernel.
 	 */
 	testq	%r12, %r12
-	jz 1f
+	jz sme_off
 	wbinvd
-1:
+sme_off:
 
 	movq	%rcx, %r11
 	call	swap_pages
@@ -195,7 +197,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	 */
 
 	testq	%r11, %r11
-	jnz 1f
+	jnz relocate
 	xorl	%eax, %eax
 	xorl	%ebx, %ebx
 	xorl    %ecx, %ecx
@@ -216,7 +218,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	ret
 	int3
 
-1:
+relocate:
 	popq	%rdx
 	leaq	PAGE_SIZE(%r10), %rsp
 	ANNOTATE_RETPOLINE_SAFE
-- 
2.43.0

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Kirill A. Shutemov]

On Tue, Apr 30, 2024 at 03:03:23PM +0200, Borislav Petkov wrote:
> On Tue, Apr 09, 2024 at 02:29:57PM +0300, Kirill A. Shutemov wrote:
> > +1:
> > +	testq	$X86_CR4_MCE, %r13
> > +	jz	1f
> > +	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
> >  1:
> 
> Please add the below patch to your set. Those same-number labels are
> just abominable.
> 
> Thx.
> 
> ---
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> Date: Tue, 30 Apr 2024 15:00:16 +0200
> Subject: [PATCH] x86/relocate_kernel: Use named labels for less confusion
> 
> That identity_mapped() function was loving that "1" label to the point
> of completely confusing its readers.
> 
> Use named labels in each place for clarity.
> 
> No functional changes.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
>  arch/x86/kernel/relocate_kernel_64.S | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> index 8e2037d78a1f..0077c9e562a7 100644
> --- a/arch/x86/kernel/relocate_kernel_64.S
> +++ b/arch/x86/kernel/relocate_kernel_64.S
> @@ -152,13 +152,15 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>  	 */
>  	movl	$X86_CR4_PAE, %eax
>  	testq	$X86_CR4_LA57, %r13
> -	jz	1f
> +	jz	no_la57
>  	orl	$X86_CR4_LA57, %eax
> -1:
> +no_la57:

I assume all of these new labels have to be prefixed with ".L", right?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Borislav Petkov]

On Tue, Apr 30, 2024 at 05:49:08PM +0300, Kirill A. Shutemov wrote:
> I assume all of these new labels have to be prefixed with ".L", right?

Oh yes, please.

Thx!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 05/18] x86/kexec: Keep CR4.MCE set during kexec for TDX guest

[Borislav Petkov]

On Thu, May 02, 2024 at 03:22:29PM +0200, Borislav Petkov wrote:
> On Tue, Apr 30, 2024 at 05:49:08PM +0300, Kirill A. Shutemov wrote:
> > I assume all of these new labels have to be prefixed with ".L", right?
> 
> Oh yes, please.

Here's a fixed version:

---
From: "Borislav Petkov (AMD)" <bp@alien8.de>
Date: Tue, 30 Apr 2024 15:00:16 +0200
Subject: [PATCH] x86/relocate_kernel: Use named labels for less confusion

That identity_mapped() functions was loving that "1" label to the point
of completely confusing its readers.

Use named labels in each place for clarity.

No functional changes.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/kernel/relocate_kernel_64.S | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 8e2037d78a1f..7f70707c7372 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -152,13 +152,15 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	 */
 	movl	$X86_CR4_PAE, %eax
 	testq	$X86_CR4_LA57, %r13
-	jz	1f
+	jz	.Lno_la57
 	orl	$X86_CR4_LA57, %eax
-1:
+.Lno_la57:
+
 	testq	$X86_CR4_MCE, %r13
-	jz	1f
+	jz	.Lmca_off
 	ALTERNATIVE "", __stringify(orl $X86_CR4_MCE, %eax), X86_FEATURE_TDX_GUEST
-1:
+.Lmca_off:
+
 	movq	%rax, %cr4
 
 	jmp 1f
@@ -173,9 +175,9 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	 * used by kexec. Flush the caches before copying the kernel.
 	 */
 	testq	%r12, %r12
-	jz 1f
+	jz .Lsme_off
 	wbinvd
-1:
+.Lsme_off:
 
 	movq	%rcx, %r11
 	call	swap_pages
@@ -195,7 +197,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	 */
 
 	testq	%r11, %r11
-	jnz 1f
+	jnz .Lrelocate
 	xorl	%eax, %eax
 	xorl	%ebx, %ebx
 	xorl    %ecx, %ecx
@@ -216,7 +218,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	ret
 	int3
 
-1:
+.Lrelocate:
 	popq	%rdx
 	leaq	PAGE_SIZE(%r10), %rsp
 	ANNOTATE_RETPOLINE_SAFE
-- 
2.43.0

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v4 0/4] x86/snp: Add kexec support

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

The patchset adds bits and pieces to get kexec (and crashkernel) work on
SNP guest.

v4:
- Rebased to current tip/master.
- Reviewed-bys from Sathya.
- Remove snp_kexec_unprep_rom_memory() as it is not needed any more as 
  SEV-SNP code is not validating the ROM range in probe_roms() anymore.
- Fix kernel test robot build error/warnings.

v3:
- Rebased;
- moved Keep page tables that maps E820_TYPE_ACPI patch to Kirill's tdx
  guest kexec patch series.
- checking the md attribute instead of checking the efi_setup for
  detecting if running under kexec kernel.
- added new sev_es_enabled() function.
- skip video memory access in decompressor for SEV-ES/SNP systems to 
  prevent guest termination as boot stage2 #VC handler does not handle
  MMIO.

v2:
- address zeroing of unaccepted memory table mappings at all page table levels
  adding phys_pte_init(), phys_pud_init() and phys_p4d_init().
- include skip efi_arch_mem_reserve() in case of kexec as part of this 
  patch set.
- rename last_address_shd_kexec to a more appropriate 
  kexec_last_address_to_make_private.
- remove duplicate code shared with TDX and use common interfaces
  defined for SNP and TDX for kexec/kdump.
- remove set_pte_enc() dependency on pg_level_to_pfn() and make the 
  function simpler.
- rename unshare_pte() to make_pte_private().
- clarify and make the comment for using kexec_last_address_to_make_private  
  more understandable.
- general cleanup. 

Ashish Kalra (4):
  efi/x86: skip efi_arch_mem_reserve() in case of kexec.
  x86/sev: add sev_es_enabled() function.
  x86/boot/compressed: Skip Video Memory access in Decompressor for
    SEV-ES/SNP.
  x86/snp: Convert shared memory back to private on kexec

 arch/x86/boot/compressed/misc.c |   6 +-
 arch/x86/boot/compressed/misc.h |   1 +
 arch/x86/boot/compressed/sev.c  |   5 +
 arch/x86/boot/compressed/sev.h  |   2 +
 arch/x86/include/asm/sev.h      |   4 +
 arch/x86/kernel/sev.c           | 161 ++++++++++++++++++++++++++++++++
 arch/x86/mm/mem_encrypt_amd.c   |   3 +
 arch/x86/platform/efi/quirks.c  |  20 +++-
 8 files changed, 198 insertions(+), 4 deletions(-)

-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v4 1/4] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

For kexec use case, need to use and stick to the EFI memmap passed
from the first kernel via boot-params/setup data, hence,
skip efi_arch_mem_reserve() during kexec.

Additionally during SNP guest kexec testing discovered that EFI memmap
is corrupted during chained kexec. kexec_enter_virtual_mode() during
late init will remap the efi_memmap physical pages allocated in
efi_arch_mem_reserve() via memblock & then subsequently cause random
EFI memmap corruption once memblock is freed/teared-down.

Suggested-by: Dave Young <dyoung@redhat.com>
[Dave Young: checking the md attribute instead of checking the efi_setup]
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/platform/efi/quirks.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index f0cc00032751..982f5e50a4b3 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -258,12 +258,28 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
 	int num_entries;
 	void *new;
 
-	if (efi_mem_desc_lookup(addr, &md) ||
-	    md.type != EFI_BOOT_SERVICES_DATA) {
+	/*
+	 * For kexec use case, we need to use the EFI memmap passed from the first
+	 * kernel via setup data, so we need to skip this.
+	 * Additionally kexec_enter_virtual_mode() during late init will remap
+	 * the efi_memmap physical pages allocated here via memboot & then
+	 * subsequently cause random EFI memmap corruption once memblock is freed.
+	 */
+
+	if (efi_mem_desc_lookup(addr, &md)) {
 		pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
 		return;
 	}
 
+	if (md.type != EFI_BOOT_SERVICES_DATA) {
+		pr_err("Skip reserving non EFI Boot Service Data memory for %pa\n", &addr);
+		return;
+	}
+
+	/* Kexec copied the efi memmap from the first kernel, thus skip the case */
+	if (md.attribute & EFI_MEMORY_RUNTIME)
+		return;
+
 	if (addr + size > md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT)) {
 		pr_err("Region spans EFI memory descriptors, %pa\n", &addr);
 		return;
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v4 2/4] x86/sev: add sev_es_enabled() function.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Add sev_es_enabled() function to detect if SEV-ES
support is enabled.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/boot/compressed/sev.c | 5 +++++
 arch/x86/boot/compressed/sev.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index ec71846d28c9..4ae4cc51e6b8 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -134,6 +134,11 @@ bool sev_snp_enabled(void)
 	return sev_status & MSR_AMD64_SEV_SNP_ENABLED;
 }
 
+bool sev_es_enabled(void)
+{
+	return sev_status & MSR_AMD64_SEV_ES_ENABLED;
+}
+
 static void __page_state_change(unsigned long paddr, enum psc_op op)
 {
 	u64 val;
diff --git a/arch/x86/boot/compressed/sev.h b/arch/x86/boot/compressed/sev.h
index fc725a981b09..5008c80e66e6 100644
--- a/arch/x86/boot/compressed/sev.h
+++ b/arch/x86/boot/compressed/sev.h
@@ -11,11 +11,13 @@
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 
 bool sev_snp_enabled(void);
+bool sev_es_enabled(void);
 void snp_accept_memory(phys_addr_t start, phys_addr_t end);
 
 #else
 
 static inline bool sev_snp_enabled(void) { return false; }
+static inline bool sev_es_enabled(void) { return false; }
 static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
 
 #endif
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 2/4] x86/sev: add sev_es_enabled() function.

[Borislav Petkov]

On Tue, Apr 09, 2024 at 08:42:38PM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Add sev_es_enabled() function to detect if SEV-ES
> support is enabled.

And use it exactly once?

Nah, use sev_status directly.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v4 3/4] x86/boot/compressed: Skip Video Memory access in Decompressor for SEV-ES/SNP.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Accessing guest video memory/RAM during kernel decompressor
causes guest termination as boot stage2 #VC handler for
SEV-ES/SNP systems does not support MMIO handling.

This issue is observed with SEV-ES/SNP guest kexec as
kexec -c adds screen_info to the boot parameters
passed to the kexec kernel, which causes console output to
be dumped to both video and serial.

As the decompressor output gets cleared really fast, it is
preferable to get the console output only on serial, hence,
skip accessing video RAM during decompressor stage to
prevent guest termination.

Serial console output during decompressor stage works as
boot stage2 #VC handler already supports handling port I/O.

Suggested-by: Thomas Lendacy <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/boot/compressed/misc.c | 6 ++++--
 arch/x86/boot/compressed/misc.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index b70e4a21c15f..47b4db200e1f 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -427,8 +427,10 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
 		vidport = 0x3d4;
 	}
 
-	lines = boot_params_ptr->screen_info.orig_video_lines;
-	cols = boot_params_ptr->screen_info.orig_video_cols;
+	if (!sev_es_enabled()) {
+		lines = boot_params_ptr->screen_info.orig_video_lines;
+		cols = boot_params_ptr->screen_info.orig_video_cols;
+	}
 
 	init_default_io_ops();
 
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index b353a7be380c..3c12ca987554 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -37,6 +37,7 @@
 #include <asm/desc_defs.h>
 
 #include "tdx.h"
+#include "sev.h"
 
 #define BOOT_CTYPE_H
 #include <linux/acpi.h>
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v4 4/4] x86/snp: Convert shared memory back to private on kexec

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

SNP guests allocate shared buffers to perform I/O. It is done by
allocating pages normally from the buddy allocator and converting them
to shared with set_memory_decrypted().

The second kernel has no idea what memory is converted this way. It only
sees E820_TYPE_RAM.

Accessing shared memory via private mapping will cause unrecoverable RMP
page-faults.

On kexec walk direct mapping and convert all shared memory back to
private. It makes all RAM private again and second kernel may use it
normally. Additionally for SNP guests convert all bss decrypted section
pages back to private and switch back ROM regions to shared so that
their revalidation does not fail during kexec kernel boot.

The conversion occurs in two steps: stopping new conversions and
unsharing all memory. In the case of normal kexec, the stopping of
conversions takes place while scheduling is still functioning. This
allows for waiting until any ongoing conversions are finished. The
second step is carried out when all CPUs except one are inactive and
interrupts are disabled. This prevents any conflicts with code that may
access shared memory.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/include/asm/sev.h    |   4 +
 arch/x86/kernel/sev.c         | 161 ++++++++++++++++++++++++++++++++++
 arch/x86/mm/mem_encrypt_amd.c |   3 +
 3 files changed, 168 insertions(+)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 7f57382afee4..78d40d08d201 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -229,6 +229,8 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end);
 u64 snp_get_unsupported_features(u64 status);
 u64 sev_get_status(void);
 void sev_show_status(void);
+void snp_kexec_unshare_mem(void);
+void snp_kexec_stop_conversion(bool crash);
 #else
 static inline void sev_es_ist_enter(struct pt_regs *regs) { }
 static inline void sev_es_ist_exit(void) { }
@@ -258,6 +260,8 @@ static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
 static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
 static inline u64 sev_get_status(void) { return 0; }
 static inline void sev_show_status(void) { }
+static inline void snp_kexec_unshare_mem(void) { }
+static inline void snp_kexec_stop_conversion(bool crash) { }
 #endif
 
 #ifdef CONFIG_KVM_AMD_SEV
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 38ad066179d8..17f616963beb 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -42,6 +42,8 @@
 #include <asm/apic.h>
 #include <asm/cpuid.h>
 #include <asm/cmdline.h>
+#include <asm/pgtable.h>
+#include <asm/set_memory.h>
 
 #define DR7_RESET_VALUE        0x400
 
@@ -92,6 +94,9 @@ static struct ghcb *boot_ghcb __section(".data");
 /* Bitmap of SEV features supported by the hypervisor */
 static u64 sev_hv_features __ro_after_init;
 
+/* Last address to be switched to private during kexec */
+static unsigned long kexec_last_addr_to_make_private;
+
 /* #VC handler runtime per-CPU data */
 struct sev_es_runtime_data {
 	struct ghcb ghcb_page;
@@ -913,6 +918,162 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end)
 	set_pages_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE);
 }
 
+static bool set_pte_enc(pte_t *kpte, int level, void *va)
+{
+	pte_t new_pte;
+
+	if (pte_none(*kpte))
+		return false;
+
+	/*
+	 * Change the physical page attribute from C=0 to C=1. Flush the
+	 * caches to ensure that data gets accessed with the correct C-bit.
+	 */
+	if (pte_present(*kpte))
+		clflush_cache_range(va, page_level_size(level));
+
+	new_pte = __pte(cc_mkenc(pte_val(*kpte)));
+	set_pte_atomic(kpte, new_pte);
+
+	return true;
+}
+
+static bool make_pte_private(pte_t *pte, unsigned long addr, int pages, int level)
+{
+	struct sev_es_runtime_data *data;
+	struct ghcb *ghcb;
+
+	data = this_cpu_read(runtime_data);
+	ghcb = &data->ghcb_page;
+
+	/* Check for GHCB for being part of a PMD range. */
+	if ((unsigned long)ghcb >= addr &&
+	    (unsigned long)ghcb <= (addr + (pages * PAGE_SIZE))) {
+		/*
+		 * Ensure that the current cpu's GHCB is made private
+		 * at the end of unshared loop so that we continue to use the
+		 * optimized GHCB protocol and not force the switch to
+		 * MSR protocol till the very end.
+		 */
+		pr_debug("setting boot_ghcb to NULL for this cpu ghcb\n");
+		kexec_last_addr_to_make_private = addr;
+		return true;
+	}
+
+	if (!set_pte_enc(pte, level, (void *)addr))
+		return false;
+
+	snp_set_memory_private(addr, pages);
+
+	return true;
+}
+
+static void unshare_all_memory(void)
+{
+	unsigned long addr, end;
+
+	/*
+	 * Walk direct mapping and convert all shared memory back to private,
+	 */
+
+	addr = PAGE_OFFSET;
+	end  = PAGE_OFFSET + get_max_mapped();
+
+	while (addr < end) {
+		unsigned long size;
+		unsigned int level;
+		pte_t *pte;
+
+		pte = lookup_address(addr, &level);
+		size = page_level_size(level);
+
+		/*
+		 * pte_none() check is required to skip physical memory holes in direct mapped.
+		 */
+		if (pte && pte_decrypted(*pte) && !pte_none(*pte)) {
+			int pages = size / PAGE_SIZE;
+
+			if (!make_pte_private(pte, addr, pages, level)) {
+				pr_err("Failed to unshare range %#lx-%#lx\n",
+				       addr, addr + size);
+			}
+
+		}
+
+		addr += size;
+	}
+	__flush_tlb_all();
+
+}
+
+static void unshare_all_bss_decrypted_memory(void)
+{
+	unsigned long vaddr, vaddr_end;
+	unsigned int level;
+	unsigned int npages;
+	pte_t *pte;
+
+	vaddr = (unsigned long)__start_bss_decrypted;
+	vaddr_end = (unsigned long)__start_bss_decrypted_unused;
+	npages = (vaddr_end - vaddr) >> PAGE_SHIFT;
+	for (; vaddr < vaddr_end; vaddr += PAGE_SIZE) {
+		pte = lookup_address(vaddr, &level);
+		if (!pte || !pte_decrypted(*pte) || pte_none(*pte))
+			continue;
+
+		set_pte_enc(pte, level, (void *)vaddr);
+	}
+	vaddr = (unsigned long)__start_bss_decrypted;
+	snp_set_memory_private(vaddr, npages);
+}
+
+/* Stop new private<->shared conversions */
+void snp_kexec_stop_conversion(bool crash)
+{
+	/*
+	 * Crash kernel reaches here with interrupts disabled: can't wait for
+	 * conversions to finish.
+	 *
+	 * If race happened, just report and proceed.
+	 */
+	bool wait_for_lock = !crash;
+
+	if (!stop_memory_enc_conversion(wait_for_lock))
+		pr_warn("Failed to finish shared<->private conversions\n");
+}
+
+void snp_kexec_unshare_mem(void)
+{
+	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
+		return;
+
+	unshare_all_memory();
+
+	unshare_all_bss_decrypted_memory();
+
+	if (kexec_last_addr_to_make_private) {
+		unsigned long size;
+		unsigned int level;
+		pte_t *pte;
+
+		/*
+		 * Switch to using the MSR protocol to change this cpu's
+		 * GHCB to private.
+		 * All the per-cpu GHCBs have been switched back to private,
+		 * so can't do any more GHCB calls to the hypervisor beyond
+		 * this point till the kexec kernel starts running.
+		 */
+		boot_ghcb = NULL;
+		sev_cfg.ghcbs_initialized = false;
+
+		pr_debug("boot ghcb 0x%lx\n", kexec_last_addr_to_make_private);
+		pte = lookup_address(kexec_last_addr_to_make_private, &level);
+		size = page_level_size(level);
+		set_pte_enc(pte, level, (void *)kexec_last_addr_to_make_private);
+		snp_set_memory_private(kexec_last_addr_to_make_private, (size / PAGE_SIZE));
+	}
+}
+
 static int snp_set_vmsa(void *va, bool vmsa)
 {
 	u64 attrs;
diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
index e7b67519ddb5..49c40c2ed809 100644
--- a/arch/x86/mm/mem_encrypt_amd.c
+++ b/arch/x86/mm/mem_encrypt_amd.c
@@ -468,6 +468,9 @@ void __init sme_early_init(void)
 	x86_platform.guest.enc_tlb_flush_required    = amd_enc_tlb_flush_required;
 	x86_platform.guest.enc_cache_flush_required  = amd_enc_cache_flush_required;
 
+	x86_platform.guest.enc_kexec_stop_conversion = snp_kexec_stop_conversion;
+	x86_platform.guest.enc_kexec_unshare_mem     = snp_kexec_unshare_mem;
+
 	/*
 	 * AMD-SEV-ES intercepts the RDMSR to read the X2APIC ID in the
 	 * parallel bringup low level code. That raises #VC which cannot be
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 4/4] x86/snp: Convert shared memory back to private on kexec

[kernel test robot]

Hi Ashish,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/master]
[also build test ERROR on linus/master next-20240410]
[cannot apply to tip/x86/core tip/x86/mm tip/auto-latest]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ashish-Kalra/efi-x86-skip-efi_arch_mem_reserve-in-case-of-kexec/20240410-044512
base:   tip/master
patch link:    https://lore.kernel.org/r/b24885f5495f6b8ba2f9e825fda9188fcbf28231.1712694667.git.ashish.kalra%40amd.com
patch subject: [PATCH v4 4/4] x86/snp: Convert shared memory back to private on kexec
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240410/202404102232.UKwWHSTE-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240410/202404102232.UKwWHSTE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404102232.UKwWHSTE-lkp@intel.com/

All errors (new ones prefixed by >>):

>> arch/x86/kernel/sev.c:993:14: error: call to undeclared function 'pte_decrypted'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     993 |                 if (pte && pte_decrypted(*pte) && !pte_none(*pte)) {
         |                            ^
   arch/x86/kernel/sev.c:1021:16: error: call to undeclared function 'pte_decrypted'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1021 |                 if (!pte || !pte_decrypted(*pte) || pte_none(*pte))
         |                              ^
>> arch/x86/kernel/sev.c:1041:7: error: call to undeclared function 'stop_memory_enc_conversion'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1041 |         if (!stop_memory_enc_conversion(wait_for_lock))
         |              ^
   3 errors generated.
--
>> arch/x86/mm/mem_encrypt_amd.c:471:21: error: no member named 'enc_kexec_stop_conversion' in 'struct x86_guest'
     471 |         x86_platform.guest.enc_kexec_stop_conversion = snp_kexec_stop_conversion;
         |         ~~~~~~~~~~~~~~~~~~ ^
>> arch/x86/mm/mem_encrypt_amd.c:472:21: error: no member named 'enc_kexec_unshare_mem' in 'struct x86_guest'
     472 |         x86_platform.guest.enc_kexec_unshare_mem     = snp_kexec_unshare_mem;
         |         ~~~~~~~~~~~~~~~~~~ ^
   2 errors generated.


vim +/pte_decrypted +993 arch/x86/kernel/sev.c

   970	
   971	static void unshare_all_memory(void)
   972	{
   973		unsigned long addr, end;
   974	
   975		/*
   976		 * Walk direct mapping and convert all shared memory back to private,
   977		 */
   978	
   979		addr = PAGE_OFFSET;
   980		end  = PAGE_OFFSET + get_max_mapped();
   981	
   982		while (addr < end) {
   983			unsigned long size;
   984			unsigned int level;
   985			pte_t *pte;
   986	
   987			pte = lookup_address(addr, &level);
   988			size = page_level_size(level);
   989	
   990			/*
   991			 * pte_none() check is required to skip physical memory holes in direct mapped.
   992			 */
 > 993			if (pte && pte_decrypted(*pte) && !pte_none(*pte)) {
   994				int pages = size / PAGE_SIZE;
   995	
   996				if (!make_pte_private(pte, addr, pages, level)) {
   997					pr_err("Failed to unshare range %#lx-%#lx\n",
   998					       addr, addr + size);
   999				}
  1000	
  1001			}
  1002	
  1003			addr += size;
  1004		}
  1005		__flush_tlb_all();
  1006	
  1007	}
  1008	
  1009	static void unshare_all_bss_decrypted_memory(void)
  1010	{
  1011		unsigned long vaddr, vaddr_end;
  1012		unsigned int level;
  1013		unsigned int npages;
  1014		pte_t *pte;
  1015	
  1016		vaddr = (unsigned long)__start_bss_decrypted;
  1017		vaddr_end = (unsigned long)__start_bss_decrypted_unused;
  1018		npages = (vaddr_end - vaddr) >> PAGE_SHIFT;
  1019		for (; vaddr < vaddr_end; vaddr += PAGE_SIZE) {
  1020			pte = lookup_address(vaddr, &level);
  1021			if (!pte || !pte_decrypted(*pte) || pte_none(*pte))
  1022				continue;
  1023	
  1024			set_pte_enc(pte, level, (void *)vaddr);
  1025		}
  1026		vaddr = (unsigned long)__start_bss_decrypted;
  1027		snp_set_memory_private(vaddr, npages);
  1028	}
  1029	
  1030	/* Stop new private<->shared conversions */
  1031	void snp_kexec_stop_conversion(bool crash)
  1032	{
  1033		/*
  1034		 * Crash kernel reaches here with interrupts disabled: can't wait for
  1035		 * conversions to finish.
  1036		 *
  1037		 * If race happened, just report and proceed.
  1038		 */
  1039		bool wait_for_lock = !crash;
  1040	
> 1041		if (!stop_memory_enc_conversion(wait_for_lock))
  1042			pr_warn("Failed to finish shared<->private conversions\n");
  1043	}
  1044	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 0/3] x86/snp: Add kexec support

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

The patchset adds bits and pieces to get kexec (and crashkernel) work on
SNP guest.

The series is based off of and tested against Kirill Shutemov's tree:
  https://github.com/intel/tdx.git guest-kexec

----

v5:
- Removed sev_es_enabled() function and using sev_status directly to
  check for SEV-ES/SEV-SNP guest.
- used --base option to generate patches to specify Kirill's TDX guest
  kexec patches as prerequisite patches to fix kernel test robot
  build errors.

v4:
- Rebased to current tip/master.
- Reviewed-bys from Sathya.
- Remove snp_kexec_unprep_rom_memory() as it is not needed any more as 
  SEV-SNP code is not validating the ROM range in probe_roms() anymore.
- Fix kernel test robot build error/warnings.

v3:
- Rebased;
- moved Keep page tables that maps E820_TYPE_ACPI patch to Kirill's tdx
  guest kexec patch series.
- checking the md attribute instead of checking the efi_setup for
  detecting if running under kexec kernel.
- added new sev_es_enabled() function.
- skip video memory access in decompressor for SEV-ES/SNP systems to 
  prevent guest termination as boot stage2 #VC handler does not handle
  MMIO.

v2:
- address zeroing of unaccepted memory table mappings at all page table levels
  adding phys_pte_init(), phys_pud_init() and phys_p4d_init().
- include skip efi_arch_mem_reserve() in case of kexec as part of this 
  patch set.
- rename last_address_shd_kexec to a more appropriate 
  kexec_last_address_to_make_private.
- remove duplicate code shared with TDX and use common interfaces
  defined for SNP and TDX for kexec/kdump.
- remove set_pte_enc() dependency on pg_level_to_pfn() and make the 
  function simpler.
- rename unshare_pte() to make_pte_private().
- clarify and make the comment for using kexec_last_address_to_make_private  
  more understandable.
- general cleanup. 


Ashish Kalra (3):
  efi/x86: skip efi_arch_mem_reserve() in case of kexec.
  x86/boot/compressed: Skip Video Memory access in Decompressor for
    SEV-ES/SNP.
  x86/snp: Convert shared memory back to private on kexec

 arch/x86/boot/compressed/misc.c |   6 +-
 arch/x86/include/asm/sev.h      |   4 +
 arch/x86/kernel/sev.c           | 161 ++++++++++++++++++++++++++++++++
 arch/x86/mm/mem_encrypt_amd.c   |   3 +
 arch/x86/platform/efi/quirks.c  |  20 +++-
 5 files changed, 190 insertions(+), 4 deletions(-)


base-commit: a18b42d8997abfd77aa1637c0de6850b0c30b1fe
prerequisite-patch-id: bd8e77f0f12223d21cb2f35b77bfcbdd9ad80b0f
prerequisite-patch-id: bfe2fa046349978ac1825275eb205acecfbc22f3
prerequisite-patch-id: 5e60d292457c7cd98fd3e45c23127e9463b56a69
prerequisite-patch-id: 1f97d0a2edb7509dd58276f628d1a4bda62c154c
prerequisite-patch-id: 8db559385c44e8b6670d74196e8d83d2dfad2f40
prerequisite-patch-id: cbdfea1e50ecb3b4cee3a25a27df4d35bd95d532
prerequisite-patch-id: 1cea0996e0dc3bb9f0059c927c405ca31003791e
prerequisite-patch-id: 469a0a3c78b0eca82527cd85e2205fb8fb89d645
prerequisite-patch-id: 2974ef211db5253d9782018e352d2a6ff0b0ef54
prerequisite-patch-id: 2cfffd80947941892421dae99b7fa0f9f9715884
prerequisite-patch-id: 466c2cb9f0a107bbd1dbd8526f4eff2bdb55f1ce
prerequisite-patch-id: d4966ae63e86d24b0bf578da4dae871cd9002b12
prerequisite-patch-id: fccde6f1fa385b5af0195f81fcb95acd71822428
prerequisite-patch-id: 16048ee15e392b0b9217b8923939b0059311abd2
prerequisite-patch-id: 5c9ae9aa294f72f63ae2c3551507dfbd92525803
prerequisite-patch-id: 758bdb686290c018cbd5b7d005354019f9d15248
prerequisite-patch-id: 4125b799fc9577b1a46427e45618fa0174f7a4b3
prerequisite-patch-id: 60760e0c98ab7ccd2ca22ae3e9f20ff5a94c6e91
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

For kexec use case, need to use and stick to the EFI memmap passed
from the first kernel via boot-params/setup data, hence,
skip efi_arch_mem_reserve() during kexec.

Additionally during SNP guest kexec testing discovered that EFI memmap
is corrupted during chained kexec. kexec_enter_virtual_mode() during
late init will remap the efi_memmap physical pages allocated in
efi_arch_mem_reserve() via memblock & then subsequently cause random
EFI memmap corruption once memblock is freed/teared-down.

Suggested-by: Dave Young <dyoung@redhat.com>
[Dave Young: checking the md attribute instead of checking the efi_setup]
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/platform/efi/quirks.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index f0cc00032751..982f5e50a4b3 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -258,12 +258,28 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
 	int num_entries;
 	void *new;
 
-	if (efi_mem_desc_lookup(addr, &md) ||
-	    md.type != EFI_BOOT_SERVICES_DATA) {
+	/*
+	 * For kexec use case, we need to use the EFI memmap passed from the first
+	 * kernel via setup data, so we need to skip this.
+	 * Additionally kexec_enter_virtual_mode() during late init will remap
+	 * the efi_memmap physical pages allocated here via memboot & then
+	 * subsequently cause random EFI memmap corruption once memblock is freed.
+	 */
+
+	if (efi_mem_desc_lookup(addr, &md)) {
 		pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
 		return;
 	}
 
+	if (md.type != EFI_BOOT_SERVICES_DATA) {
+		pr_err("Skip reserving non EFI Boot Service Data memory for %pa\n", &addr);
+		return;
+	}
+
+	/* Kexec copied the efi memmap from the first kernel, thus skip the case */
+	if (md.attribute & EFI_MEMORY_RUNTIME)
+		return;
+
 	if (addr + size > md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT)) {
 		pr_err("Region spans EFI memory descriptors, %pa\n", &addr);
 		return;
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Borislav Petkov]

On Mon, Apr 15, 2024 at 11:22:58PM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> For kexec use case, need to use and stick to the EFI memmap passed
> from the first kernel via boot-params/setup data, hence,
> skip efi_arch_mem_reserve() during kexec.

Please use this or similar scheme when formulating your commit messages.
This above is too laconic.

1. Prepare the context for the explanation briefly.

2. Explain the problem at hand.

3. "It happens because of <...>"

4. "Fix it by doing X"

5. "(Potentially do Y)."

And some of those above are optional depending on the issue being
explained.

For more detailed info, see
Documentation/process/submitting-patches.rst,
Section "2) Describe your changes".

> Additionally during SNP guest kexec testing discovered that EFI memmap
> is corrupted during chained kexec.

That sentence needs sanitization.

> kexec_enter_virtual_mode() during late init will remap the efi_memmap
> physical pages allocated in efi_arch_mem_reserve() via memblock & then

s/&/and/

This is not code. Please take a greater care when writing commit
messages - they're not write-only.

> subsequently cause random EFI memmap corruption once memblock is
> freed/teared-down.

"torn down"

> Suggested-by: Dave Young <dyoung@redhat.com>
> [Dave Young: checking the md attribute instead of checking the efi_setup]
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  arch/x86/platform/efi/quirks.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index f0cc00032751..982f5e50a4b3 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -258,12 +258,28 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>  	int num_entries;
>  	void *new;
>  
> -	if (efi_mem_desc_lookup(addr, &md) ||
> -	    md.type != EFI_BOOT_SERVICES_DATA) {
> +	/*
> +	 * For kexec use case, we need to use the EFI memmap passed from the first

Make all your text impersonal - no "we", "I", etc.

> +	 * kernel via setup data, so we need to skip this.

What exactly do we need to skip?

If the EFI memory descriptor lookup fails?

> +	 * Additionally kexec_enter_virtual_mode() during late init will remap
> +	 * the efi_memmap physical pages allocated here via memboot & then
> +	 * subsequently cause random EFI memmap corruption once memblock is freed.
> +	 */

Why is that comment here and what is its relevance to the line it is
above of?

> +	if (efi_mem_desc_lookup(addr, &md)) {
>  		pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
>  		return;
>  	}
>  
> +	if (md.type != EFI_BOOT_SERVICES_DATA) {
> +		pr_err("Skip reserving non EFI Boot Service Data memory for %pa\n", &addr);

What is this pr_err() useful for?

> +		return;
> +	}
> +
> +	/* Kexec copied the efi memmap from the first kernel, thus skip the case */

kexec? This is a generic function - what does it have to do with kexec?

The subject of this patch is:

Subject: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec

and yet, nothing skips this function - it adds a bunch of checks,
printks and early returns with the intent that those early returns
happen on kexec and thus the actual memremap doesn't happen there.

So it is some sort of: let's check things which will be true in
a kexec-ed kernel and thus avoid the function by returning early.

But I have no clue.

It sounds to me like you need to go back up, to the 10000ft view and
explain how exactly this efi_mem_reserve() causes trouble for the
kexec-ed kernel so that we can think of a proper solution, not some
random hackery.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Kalra, Ashish]

Hello Boris,

On 4/24/2024 9:48 AM, Borislav Petkov wrote:
> On Mon, Apr 15, 2024 at 11:22:58PM +0000, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> For kexec use case, need to use and stick to the EFI memmap passed
>> from the first kernel via boot-params/setup data, hence,
>> skip efi_arch_mem_reserve() during kexec.
> Please use this or similar scheme when formulating your commit messages.
> This above is too laconic.
>
> 1. Prepare the context for the explanation briefly.
>
> 2. Explain the problem at hand.
>
> 3. "It happens because of <...>"
>
> 4. "Fix it by doing X"
>
> 5. "(Potentially do Y)."
>
> And some of those above are optional depending on the issue being
> explained.
>
> For more detailed info, see
> Documentation/process/submitting-patches.rst,
> Section "2) Describe your changes".

Here is the more detailed description of the issue:

With SNP guest kexec and during nested guest kexec, observe the 
following efi memmap corruption :

[    0.000000] efi: EFI v2.7 by EDK II^M
[    0.000000] efi: SMBIOS=0x7e33f000 SMBIOS 3.0=0x7e33d000 
ACPI=0x7e57e000 ACPI 2.0=0x7e57e014 MEMATTR=0x7cc3c018 
Unaccepted=0x7c09e018 ^M
[    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:^M
[    0.000000] efi: mem03: [type=269370880|attr=0x0e42100e42180e41] 
range=[0x0486200e41038c18-0x200e898a0eee713ac17] (invalid)^M
[    0.000000] efi: mem04: [type=12336|attr=0x0e410686300e4105] 
range=[0x100e420000000176-0x8c290f26248d200e175] (invalid)^M
[    0.000000] efi: mem06: [type=1124304408|attr=0x000030b400000028] 
range=[0x0e51300e45280e77-0xb44ed2142f460c1e76] (invalid)^M
[    0.000000] efi: mem08: [type=68|attr=0x300e540583280e41] 
range=[0x0000011affff3cd8-0x486200e54b38c0bcd7] (invalid)^M
[    0.000000] efi: mem10: [type=1107529240|attr=0x0e42280e41300e41] 
range=[0x300e41058c280e42-0x38010ae54c5c328ee41] (invalid)^M
[    0.000000] efi: mem11: [type=189335566|attr=0x048d200e42038e18] 
range=[0x0000318c00000048-0xe42029228ce4200047] (invalid)^M
[    0.000000] efi: mem12: [type=239142534|attr=0x0000002400000b4b] 
range=[0x0e41380e0a7d700e-0x80f26238f22bfe500d] (invalid)^M
[    0.000000] efi: mem14: [type=239207055|attr=0x0e41300e43380e0a] 
range=[0x8c280e42048d200e-0xc70b028f2f27cc0a00d] (invalid)^M
[    0.000000] efi: mem15: [type=239210510|attr=0x00080e660b47080e] 
range=[0x0000324c0000001c-0xa78028634ce490001b] (invalid)^M
[    0.000000] efi: mem16: [type=4294848528|attr=0x0000329400000014] 
range=[0x0e410286100e4100-0x80f252036a218f20ff] (invalid)^M
[    0.000000] efi: mem19: [type=2250772033|attr=0x42180e42200e4328] 
range=[0x41280e0ab9020683-0xe0e538c28b39e62682] (invalid)^M
[    0.000000] efi: mem20: [type=16|   |  |  |  |  |  |  |  |  | |WB|  
|WC|  ] range=[0x00000008ffff4438-0xffff44340090333c437] (invalid)^M
[    0.000000] efi: mem22: [Reserved    |attr=0x000000c1ffff4420] 
range=[0xffff442400003398-0x1033a04240003f397] (invalid)^M
[    0.000000] efi: mem23: [type=1141080856|attr=0x080e41100e43180e] 
range=[0x280e66300e4b280e-0x440dc5ee7141f4c080d] (invalid)^M
[    0.000000] efi: mem25: [Reserved    |attr=0x0000000affff44a0] 
range=[0xffff44a400003428-0x1034304a400013427] (invalid)^M
[    0.000000] efi: mem28: [type=16|   |  |  |  |  |  |  |  |  | |WB|  
|WC|  ] range=[0x0000000affff4488-0xffff448400b034bc487] (invalid)^M
[    0.000000] efi: mem30: [Reserved    |attr=0x0000000affff4470] 
range=[0xffff447400003518-0x10352047400013517] (invalid)^M
[    0.000000] efi: mem33: [type=16|   |  |  |  |  |  |  |  |  | |WB|  
|WC|  ] range=[0x0000000affff4458-0xffff445400b035ac457] (invalid)^M
[    0.000000] efi: mem35: [type=269372416|attr=0x0e42100e42180e41] 
range=[0x0486200e44038c18-0x200e8b8a0eee823ac17] (invalid)^M
[    0.000000] efi: mem37: [type=2351435330|attr=0x0e42100e42180e42] 
range=[0x470783380e410686-0x2002b2a041c2141e685] (invalid)^M
[    0.000000] efi: mem38: [type=1093668417|attr=0x100e420000000270] 
range=[0x42100e42180e4220-0xfff366a4e421b78c21f] (invalid)^M
[    0.000000] efi: mem39: [type=76357646|attr=0x180e42200e42280e] 
range=[0x0e410686300e4105-0x4130f251a0710ae5104] (invalid)^M
[    0.000000] efi: mem40: [type=940444268|attr=0x0e42200e42280e41] 
range=[0x180e42200e42280e-0x300fc71c300b4f2480d] (invalid)^M
[    0.000000] efi: mem41: [MMIO        |attr=0x8c280e42048d200e] 
range=[0xffff479400003728-0x42138e0c87820292727] (invalid)^M
[    0.000000] efi: mem42: [type=1191674680|attr=0x0000004c0000000b] 
range=[0x300e41380e0a0246-0x470b0f26238f22b8245] (invalid)^M
[    0.000000] efi: mem43: [type=2010|attr=0x0301f00e4d078338] 
range=[0x45038e180e42028f-0xe4556bf118f282528e] (invalid)^M
[    0.000000] efi: mem44: [type=1109921345|attr=0x300e44000000006c] 
range=[0x44080e42100e4218-0xfff39254e42138ac217] (invalid)^M
[    0.000000] efi: mem45: [type=40|attr=0x0e41100e41180e0a] 
range=[0x0000008affff5228-0x4702400e53b3830d227] (invalid)^M
[    0.000000] efi: mem47: [type=1107529240|attr=0x42280e41300e4138] 
range=[0x300e44058c280e42-0xe0d049a435c728ee41] (invalid)^M

...

This EFI memap corruption is happening during efi_arch_mem_reserve() 
invocation with the previous kexec-ed kernel boot.

( efi_arch_mem_reserve() is invoked with the following call-stack: )

[    0.310010]  efi_arch_mem_reserve+0xb1/0x220^M
[    0.310686]  ? memblock_add_range+0x2a0/0x2e0^M
[    0.311382]  efi_mem_reserve+0x36/0x60^M
[    0.311973]  efi_bgrt_init+0x17d/0x1a0^M
[    0.312565]  ? __pfx_acpi_parse_bgrt+0x10/0x10^M
[    0.313265]  acpi_parse_bgrt+0x12/0x20^M
[    0.313858]  acpi_table_parse+0x77/0xd0^M
[    0.314463]  acpi_boot_init+0x362/0x630^M
[    0.315069]  setup_arch+0xa88/0xf80^M
[    0.315629]  start_kernel+0x68/0xa90^M
[    0.316194]  x86_64_start_reservations+0x1c/0x30^M
[    0.316921]  x86_64_start_kernel+0xbf/0x110^M
[    0.317582]  common_startup_64+0x13e/0x141^M
[    0.318231]  </TASK>^M

Now, efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory 
for EFI memory map and due to early allocation it uses memblock allocation.

Later in the boot flow, efi_enter_virtual_mode() calls 
kexec_enter_virtual_mode() in case of a kexec-ed kernel boot.

This function kexec_enter_virtual_mode() installs the new EFI memory map 
by calling efi_memmap_init_late() which remaps the efi_memmap physically 
allocated above in efi_arch_mem_reserve(), but please note that this 
remapping is still using memblock allocation.

Subsequently, when memblock is freed later in boot flow, the above 
remapped efi_memmap will have random corruption (similar to a 
use-after-free scenario).

This corrupted EFI memory map is then passed to the next kexec-ed kernel 
which causes a panic when trying to use the corrupted EFI memory map.

>> Additionally during SNP guest kexec testing discovered that EFI memmap
>> is corrupted during chained kexec.
> That sentence needs sanitization.
>
>> kexec_enter_virtual_mode() during late init will remap the efi_memmap
>> physical pages allocated in efi_arch_mem_reserve() via memblock & then
> s/&/and/
>
> This is not code. Please take a greater care when writing commit
> messages - they're not write-only.
>
>> subsequently cause random EFI memmap corruption once memblock is
>> freed/teared-down.
> "torn down"
>
>> Suggested-by: Dave Young <dyoung@redhat.com>
>> [Dave Young: checking the md attribute instead of checking the efi_setup]
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>   arch/x86/platform/efi/quirks.c | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index f0cc00032751..982f5e50a4b3 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -258,12 +258,28 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>>   	int num_entries;
>>   	void *new;
>>   
>> -	if (efi_mem_desc_lookup(addr, &md) ||
>> -	    md.type != EFI_BOOT_SERVICES_DATA) {
>> +	/*
>> +	 * For kexec use case, we need to use the EFI memmap passed from the first
> Make all your text impersonal - no "we", "I", etc.
>
>> +	 * kernel via setup data, so we need to skip this.
> What exactly do we need to skip?
>
> If the EFI memory descriptor lookup fails?
>
>> +	 * Additionally kexec_enter_virtual_mode() during late init will remap
>> +	 * the efi_memmap physical pages allocated here via memboot & then
>> +	 * subsequently cause random EFI memmap corruption once memblock is freed.
>> +	 */
> Why is that comment here and what is its relevance to the line it is
> above of?
>
>> +	if (efi_mem_desc_lookup(addr, &md)) {
>>   		pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
>>   		return;
>>   	}
>>   
>> +	if (md.type != EFI_BOOT_SERVICES_DATA) {
>> +		pr_err("Skip reserving non EFI Boot Service Data memory for %pa\n", &addr);
> What is this pr_err() useful for?
>
>> +		return;
>> +	}
>> +
>> +	/* Kexec copied the efi memmap from the first kernel, thus skip the case */
> kexec? This is a generic function - what does it have to do with kexec?
>
> The subject of this patch is:
>
> Subject: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec
>
> and yet, nothing skips this function - it adds a bunch of checks,
> printks and early returns with the intent that those early returns
> happen on kexec and thus the actual memremap doesn't happen there.
>
> So it is some sort of: let's check things which will be true in
> a kexec-ed kernel and thus avoid the function by returning early.
>
> But I have no clue.
>
> It sounds to me like you need to go back up, to the 10000ft view and
> explain how exactly this efi_mem_reserve() causes trouble for the
> kexec-ed kernel so that we can think of a proper solution, not some
> random hackery.

The above details explain why and how efi_arch_mem_reserve() causes 
trouble for the (nested) kexec-ed kernel, additionally, there is a 
another reason to skip efi_arch_mem_reserve() altogether for the kexec 
case, as for kexec use case we need to use the EFI memmap passed from 
the 1st kernel via setup_data and probably need to avoid any additional 
EFI memory map additions/updates.

Therefore, the first revision of this patch had the following code to 
skip efi_arch_mem_reserve():

void __init efi_arch_mem_reserve(..) {

+ if (efi_setup) + return;

But then based on upstream review/feedback, the second revision of this 
patch, updated the patch to check the md attribute of the EFI memory 
descriptor instead of checking for efi_setup for detecting if running 
under kexec kernel and the checking of the md attribute of the EFI 
memory descriptor introduces these additional checks and pr_err() which 
you commented on above.

Hopefully, the above detailed explanation captures the reason to skip 
efi_arch_mem_reserve() in case of (SNP) guest kexec, looking forward to 
your comments/feedback on the same for me to rework this patch 
(especially the commit message) and post it again.

Thanks, Ashish


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Kalra, Ashish]

 >It sounds to me like you need to go back up, to the 10000ft view and
>> explain how exactly this efi_mem_reserve() causes trouble for the
>> kexec-ed kernel so that we can think of a proper solution, not some
>> random hackery.
>
> The above details explain why and how efi_arch_mem_reserve() causes 
> trouble for the (nested) kexec-ed kernel, additionally, there is a 
> another reason to skip efi_arch_mem_reserve() altogether for the kexec 
> case, as for kexec use case we need to use the EFI memmap passed from 
> the 1st kernel via setup_data and probably need to avoid any 
> additional EFI memory map additions/updates.
>
> Therefore, the first revision of this patch had the following code to 
> skip efi_arch_mem_reserve():
>
> void __init efi_arch_mem_reserve(..) {
>
> + if (efi_setup) + return;
>
> But then based on upstream review/feedback, the second revision of 
> this patch, updated the patch to check the md attribute of the EFI 
> memory descriptor instead of checking for efi_setup for detecting if 
> running under kexec kernel and the checking of the md attribute of the 
> EFI memory descriptor introduces these additional checks and pr_err() 
> which you commented on above.
>
> Hopefully, the above detailed explanation captures the reason to skip 
> efi_arch_mem_reserve() in case of (SNP) guest kexec, looking forward 
> to your comments/feedback on the same for me to rework this patch 
> (especially the commit message) and post it again.

<snip>

I am actually going to rename this patch to something more appropriate like:

Fix EFI memory map corruption during SNP guest kexec

And in the patch itself, go back to skipping efi_arch_mem_reserve() by 
checking efi_setup to check for running under kexec kernel similar to 
how it used by efi_enter_virtual_mode().

Thanks, Ashish


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Borislav Petkov]

On Wed, Apr 24, 2024 at 04:17:09PM -0500, Kalra, Ashish wrote:
> With SNP guest kexec and during nested guest kexec, observe the following
> efi memmap corruption :

Before we delve any deeper here, lemme make sure I understand this
correctly:

* You're in a SNP guest and you're kexec-ing into a new kernel?

or

* You have a plain hypervisor which runs a non-CoCo guest and that guest
is a hypervisor too and it starts a level 2 guest and *in* *that* level
2 guest you kexec a kernel?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Kalra, Ashish]

On 4/26/2024 9:21 AM, Borislav Petkov wrote:

> On Wed, Apr 24, 2024 at 04:17:09PM -0500, Kalra, Ashish wrote:
>> With SNP guest kexec and during nested guest kexec, observe the following
>> efi memmap corruption :
> Before we delve any deeper here, lemme make sure I understand this
> correctly:
>
> * You're in a SNP guest and you're kexec-ing into a new kernel?

Yes this is the case, where i am in a SNP guest and kexec-ing into a new 
kernel.

I should have mentioned *chained* guest kexec above instead of nested 
guest kexec.

Thanks, Ashish


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Borislav Petkov]

On Fri, Apr 26, 2024 at 09:47:02AM -0500, Kalra, Ashish wrote:
> I should have mentioned *chained* guest kexec above instead of nested guest
> kexec.

What is a "chained guest kexec" now?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Kalra, Ashish]

On 4/26/2024 10:22 AM, Borislav Petkov wrote:

> On Fri, Apr 26, 2024 at 09:47:02AM -0500, Kalra, Ashish wrote:
>> I should have mentioned *chained* guest kexec above instead of nested guest
>> kexec.
> What is a "chained guest kexec" now?

"Chained guest kexec" is when we are in a guest and kexec-ing into a new 
kernel and then this kernel kexecs into another and so on ...

Thanks, Ashish


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Borislav Petkov]

On Fri, Apr 26, 2024 at 10:28:41AM -0500, Kalra, Ashish wrote:
> "Chained guest kexec" is when we are in a guest and kexec-ing into a new
> kernel and then this kernel kexecs into another and so on ...

Make sure to explain your terminology:

$ git grep -rE "chained.*kexec"
$

and there's nothing "chained" about it - you're simply kexec-ing in
a loop.

Please don't make it sound more complicated than it is.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 1/3] efi/x86: skip efi_arch_mem_reserve() in case of kexec.

[Kalra, Ashish]

On 4/26/2024 10:34 AM, Borislav Petkov wrote:

> On Fri, Apr 26, 2024 at 10:28:41AM -0500, Kalra, Ashish wrote:
>> "Chained guest kexec" is when we are in a guest and kexec-ing into a new
>> kernel and then this kernel kexecs into another and so on ...
> Make sure to explain your terminology:
>
> $ git grep -rE "chained.*kexec"
> $
>
> and there's nothing "chained" about it - you're simply kexec-ing in
> a loop.
>
> Please don't make it sound more complicated than it is.

I have posted another version of this patch series (v6) with 
restructured and updated commit message for this patch, so please review 
this v6 series.

Thanks, Ashish


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 2/3] x86/boot/compressed: Skip Video Memory access in Decompressor for SEV-ES/SNP.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Accessing guest video memory/RAM during kernel decompressor
causes guest termination as boot stage2 #VC handler for
SEV-ES/SNP systems does not support MMIO handling.

This issue is observed with SEV-ES/SNP guest kexec as
kexec -c adds screen_info to the boot parameters
passed to the kexec kernel, which causes console output to
be dumped to both video and serial.

As the decompressor output gets cleared really fast, it is
preferable to get the console output only on serial, hence,
skip accessing video RAM during decompressor stage to
prevent guest termination.

Serial console output during decompressor stage works as
boot stage2 #VC handler already supports handling port I/O.

Suggested-by: Thomas Lendacy <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/boot/compressed/misc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index b70e4a21c15f..3b9f96b3dbcc 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -427,8 +427,10 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
 		vidport = 0x3d4;
 	}
 
-	lines = boot_params_ptr->screen_info.orig_video_lines;
-	cols = boot_params_ptr->screen_info.orig_video_cols;
+	if (!(sev_status & MSR_AMD64_SEV_ES_ENABLED)) {
+		lines = boot_params_ptr->screen_info.orig_video_lines;
+		cols = boot_params_ptr->screen_info.orig_video_cols;
+	}
 
 	init_default_io_ops();
 
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 3/3] x86/snp: Convert shared memory back to private on kexec

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

SNP guests allocate shared buffers to perform I/O. It is done by
allocating pages normally from the buddy allocator and converting them
to shared with set_memory_decrypted().

The second kernel has no idea what memory is converted this way. It only
sees E820_TYPE_RAM.

Accessing shared memory via private mapping will cause unrecoverable RMP
page-faults.

On kexec walk direct mapping and convert all shared memory back to
private. It makes all RAM private again and second kernel may use it
normally. Additionally for SNP guests convert all bss decrypted section
pages back to private and switch back ROM regions to shared so that
their revalidation does not fail during kexec kernel boot.

The conversion occurs in two steps: stopping new conversions and
unsharing all memory. In the case of normal kexec, the stopping of
conversions takes place while scheduling is still functioning. This
allows for waiting until any ongoing conversions are finished. The
second step is carried out when all CPUs except one are inactive and
interrupts are disabled. This prevents any conflicts with code that may
access shared memory.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/include/asm/sev.h    |   4 +
 arch/x86/kernel/sev.c         | 161 ++++++++++++++++++++++++++++++++++
 arch/x86/mm/mem_encrypt_amd.c |   3 +
 3 files changed, 168 insertions(+)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 7f57382afee4..78d40d08d201 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -229,6 +229,8 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end);
 u64 snp_get_unsupported_features(u64 status);
 u64 sev_get_status(void);
 void sev_show_status(void);
+void snp_kexec_unshare_mem(void);
+void snp_kexec_stop_conversion(bool crash);
 #else
 static inline void sev_es_ist_enter(struct pt_regs *regs) { }
 static inline void sev_es_ist_exit(void) { }
@@ -258,6 +260,8 @@ static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
 static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
 static inline u64 sev_get_status(void) { return 0; }
 static inline void sev_show_status(void) { }
+static inline void snp_kexec_unshare_mem(void) { }
+static inline void snp_kexec_stop_conversion(bool crash) { }
 #endif
 
 #ifdef CONFIG_KVM_AMD_SEV
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 38ad066179d8..17f616963beb 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -42,6 +42,8 @@
 #include <asm/apic.h>
 #include <asm/cpuid.h>
 #include <asm/cmdline.h>
+#include <asm/pgtable.h>
+#include <asm/set_memory.h>
 
 #define DR7_RESET_VALUE        0x400
 
@@ -92,6 +94,9 @@ static struct ghcb *boot_ghcb __section(".data");
 /* Bitmap of SEV features supported by the hypervisor */
 static u64 sev_hv_features __ro_after_init;
 
+/* Last address to be switched to private during kexec */
+static unsigned long kexec_last_addr_to_make_private;
+
 /* #VC handler runtime per-CPU data */
 struct sev_es_runtime_data {
 	struct ghcb ghcb_page;
@@ -913,6 +918,162 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end)
 	set_pages_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE);
 }
 
+static bool set_pte_enc(pte_t *kpte, int level, void *va)
+{
+	pte_t new_pte;
+
+	if (pte_none(*kpte))
+		return false;
+
+	/*
+	 * Change the physical page attribute from C=0 to C=1. Flush the
+	 * caches to ensure that data gets accessed with the correct C-bit.
+	 */
+	if (pte_present(*kpte))
+		clflush_cache_range(va, page_level_size(level));
+
+	new_pte = __pte(cc_mkenc(pte_val(*kpte)));
+	set_pte_atomic(kpte, new_pte);
+
+	return true;
+}
+
+static bool make_pte_private(pte_t *pte, unsigned long addr, int pages, int level)
+{
+	struct sev_es_runtime_data *data;
+	struct ghcb *ghcb;
+
+	data = this_cpu_read(runtime_data);
+	ghcb = &data->ghcb_page;
+
+	/* Check for GHCB for being part of a PMD range. */
+	if ((unsigned long)ghcb >= addr &&
+	    (unsigned long)ghcb <= (addr + (pages * PAGE_SIZE))) {
+		/*
+		 * Ensure that the current cpu's GHCB is made private
+		 * at the end of unshared loop so that we continue to use the
+		 * optimized GHCB protocol and not force the switch to
+		 * MSR protocol till the very end.
+		 */
+		pr_debug("setting boot_ghcb to NULL for this cpu ghcb\n");
+		kexec_last_addr_to_make_private = addr;
+		return true;
+	}
+
+	if (!set_pte_enc(pte, level, (void *)addr))
+		return false;
+
+	snp_set_memory_private(addr, pages);
+
+	return true;
+}
+
+static void unshare_all_memory(void)
+{
+	unsigned long addr, end;
+
+	/*
+	 * Walk direct mapping and convert all shared memory back to private,
+	 */
+
+	addr = PAGE_OFFSET;
+	end  = PAGE_OFFSET + get_max_mapped();
+
+	while (addr < end) {
+		unsigned long size;
+		unsigned int level;
+		pte_t *pte;
+
+		pte = lookup_address(addr, &level);
+		size = page_level_size(level);
+
+		/*
+		 * pte_none() check is required to skip physical memory holes in direct mapped.
+		 */
+		if (pte && pte_decrypted(*pte) && !pte_none(*pte)) {
+			int pages = size / PAGE_SIZE;
+
+			if (!make_pte_private(pte, addr, pages, level)) {
+				pr_err("Failed to unshare range %#lx-%#lx\n",
+				       addr, addr + size);
+			}
+
+		}
+
+		addr += size;
+	}
+	__flush_tlb_all();
+
+}
+
+static void unshare_all_bss_decrypted_memory(void)
+{
+	unsigned long vaddr, vaddr_end;
+	unsigned int level;
+	unsigned int npages;
+	pte_t *pte;
+
+	vaddr = (unsigned long)__start_bss_decrypted;
+	vaddr_end = (unsigned long)__start_bss_decrypted_unused;
+	npages = (vaddr_end - vaddr) >> PAGE_SHIFT;
+	for (; vaddr < vaddr_end; vaddr += PAGE_SIZE) {
+		pte = lookup_address(vaddr, &level);
+		if (!pte || !pte_decrypted(*pte) || pte_none(*pte))
+			continue;
+
+		set_pte_enc(pte, level, (void *)vaddr);
+	}
+	vaddr = (unsigned long)__start_bss_decrypted;
+	snp_set_memory_private(vaddr, npages);
+}
+
+/* Stop new private<->shared conversions */
+void snp_kexec_stop_conversion(bool crash)
+{
+	/*
+	 * Crash kernel reaches here with interrupts disabled: can't wait for
+	 * conversions to finish.
+	 *
+	 * If race happened, just report and proceed.
+	 */
+	bool wait_for_lock = !crash;
+
+	if (!stop_memory_enc_conversion(wait_for_lock))
+		pr_warn("Failed to finish shared<->private conversions\n");
+}
+
+void snp_kexec_unshare_mem(void)
+{
+	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
+		return;
+
+	unshare_all_memory();
+
+	unshare_all_bss_decrypted_memory();
+
+	if (kexec_last_addr_to_make_private) {
+		unsigned long size;
+		unsigned int level;
+		pte_t *pte;
+
+		/*
+		 * Switch to using the MSR protocol to change this cpu's
+		 * GHCB to private.
+		 * All the per-cpu GHCBs have been switched back to private,
+		 * so can't do any more GHCB calls to the hypervisor beyond
+		 * this point till the kexec kernel starts running.
+		 */
+		boot_ghcb = NULL;
+		sev_cfg.ghcbs_initialized = false;
+
+		pr_debug("boot ghcb 0x%lx\n", kexec_last_addr_to_make_private);
+		pte = lookup_address(kexec_last_addr_to_make_private, &level);
+		size = page_level_size(level);
+		set_pte_enc(pte, level, (void *)kexec_last_addr_to_make_private);
+		snp_set_memory_private(kexec_last_addr_to_make_private, (size / PAGE_SIZE));
+	}
+}
+
 static int snp_set_vmsa(void *va, bool vmsa)
 {
 	u64 attrs;
diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
index e7b67519ddb5..49c40c2ed809 100644
--- a/arch/x86/mm/mem_encrypt_amd.c
+++ b/arch/x86/mm/mem_encrypt_amd.c
@@ -468,6 +468,9 @@ void __init sme_early_init(void)
 	x86_platform.guest.enc_tlb_flush_required    = amd_enc_tlb_flush_required;
 	x86_platform.guest.enc_cache_flush_required  = amd_enc_cache_flush_required;
 
+	x86_platform.guest.enc_kexec_stop_conversion = snp_kexec_stop_conversion;
+	x86_platform.guest.enc_kexec_unshare_mem     = snp_kexec_unshare_mem;
+
 	/*
 	 * AMD-SEV-ES intercepts the RDMSR to read the X2APIC ID in the
 	 * parallel bringup low level code. That raises #VC which cannot be
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v6 0/3] x86/snp: Add kexec support

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

The patchset adds bits and pieces to get kexec (and crashkernel) work on
SNP guest.

The series is based off of and tested against Kirill Shutemov's tree:
  https://github.com/intel/tdx.git guest-kexec

----

v6:
- Updated and restructured the commit message for patch 1/3 to
  explain the issue in detail.
- Updated inline comments in patch 1/3 to explain the issue in 
  detail.
- Moved back to checking efi_setup for detecting if running
  under kexec kernel.

v5:
- Removed sev_es_enabled() function and using sev_status directly to
  check for SEV-ES/SEV-SNP guest.
- used --base option to generate patches to specify Kirill's TDX guest
  kexec patches as prerequisite patches to fix kernel test robot
  build errors.

v4:
- Rebased to current tip/master.
- Reviewed-bys from Sathya.
- Remove snp_kexec_unprep_rom_memory() as it is not needed any more as 
  SEV-SNP code is not validating the ROM range in probe_roms() anymore.
- Fix kernel test robot build error/warnings.

v3:
- Rebased;
- moved Keep page tables that maps E820_TYPE_ACPI patch to Kirill's tdx
  guest kexec patch series.
- checking the md attribute instead of checking the efi_setup for
  detecting if running under kexec kernel.
- added new sev_es_enabled() function.
- skip video memory access in decompressor for SEV-ES/SNP systems to 
  prevent guest termination as boot stage2 #VC handler does not handle
  MMIO.

v2:
- address zeroing of unaccepted memory table mappings at all page table levels
  adding phys_pte_init(), phys_pud_init() and phys_p4d_init().
- include skip efi_arch_mem_reserve() in case of kexec as part of this 
  patch set.
- rename last_address_shd_kexec to a more appropriate 
  kexec_last_address_to_make_private.
- remove duplicate code shared with TDX and use common interfaces
  defined for SNP and TDX for kexec/kdump.
- remove set_pte_enc() dependency on pg_level_to_pfn() and make the 
  function simpler.
- rename unshare_pte() to make_pte_private().
- clarify and make the comment for using kexec_last_address_to_make_private  
  more understandable.
- general cleanup. 


Ashish Kalra (3):
  efi/x86: Fix EFI memory map corruption with kexec
  x86/boot/compressed: Skip Video Memory access in Decompressor for
    SEV-ES/SNP.
  x86/snp: Convert shared memory back to private on kexec

 arch/x86/boot/compressed/misc.c |   6 +-
 arch/x86/include/asm/sev.h      |   4 +
 arch/x86/kernel/sev.c           | 161 ++++++++++++++++++++++++++++++++
 arch/x86/mm/mem_encrypt_amd.c   |   3 +
 arch/x86/platform/efi/quirks.c  |  20 ++++
 5 files changed, 192 insertions(+), 2 deletions(-)


base-commit: 7fcd76de8a7bc12e930ef383a157ce99d711715d
prerequisite-patch-id: a911f230c2524bd791c47f62f17f0a93cbf726b6
prerequisite-patch-id: bfe2fa046349978ac1825275eb205acecfbc22f3
prerequisite-patch-id: 5e60d292457c7cd98fd3e45c23127e9463b56a69
prerequisite-patch-id: 1f97d0a2edb7509dd58276f628d1a4bda62c154c
prerequisite-patch-id: cbc2507b5c2810c3015aaf836d774d32f969c19a
prerequisite-patch-id: cbdfea1e50ecb3b4cee3a25a27df4d35bd95d532
prerequisite-patch-id: 99382c42348b9a076ba930eca0dfc9d000ec951d
prerequisite-patch-id: 469a0a3c78b0eca82527cd85e2205fb8fb89d645
prerequisite-patch-id: 2974ef211db5253d9782018e352d2a6ff0b0ef54
prerequisite-patch-id: 2cfffd80947941892421dae99b7fa0f9f9715884
prerequisite-patch-id: 466c2cb9f0a107bbd1dbd8526f4eff2bdb55f1ce
prerequisite-patch-id: d4966ae63e86d24b0bf578da4dae871cd9002b12
prerequisite-patch-id: fccde6f1fa385b5af0195f81fcb95acd71822428
prerequisite-patch-id: 16048ee15e392b0b9217b8923939b0059311abd2
prerequisite-patch-id: 5c9ae9aa294f72f63ae2c3551507dfbd92525803
prerequisite-patch-id: 758bdb686290c018cbd5b7d005354019f9d15248
prerequisite-patch-id: c85fd0bb6d183a40da73720eaa607481b1d51daf
prerequisite-patch-id: 60760e0c98ab7ccd2ca22ae3e9f20ff5a94c6e91
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v6 1/3] efi/x86: Fix EFI memory map corruption with kexec

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

With SNP guest kexec observe the following efi memmap corruption :

[    0.000000] efi: EFI v2.7 by EDK II
[    0.000000] efi: SMBIOS=0x7e33f000 SMBIOS 3.0=0x7e33d000 ACPI=0x7e57e000 ACPI 2.0=0x7e57e014 MEMATTR=0x7cc3c018 Unaccepted=0x7c09e018
[    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:
[    0.000000] efi: mem03: [type=269370880|attr=0x0e42100e42180e41] range=[0x0486200e41038c18-0x200e898a0eee713ac17] (invalid)
[    0.000000] efi: mem04: [type=12336|attr=0x0e410686300e4105] range=[0x100e420000000176-0x8c290f26248d200e175] (invalid)
[    0.000000] efi: mem06: [type=1124304408|attr=0x000030b400000028] range=[0x0e51300e45280e77-0xb44ed2142f460c1e76] (invalid)
[    0.000000] efi: mem08: [type=68|attr=0x300e540583280e41] range=[0x0000011affff3cd8-0x486200e54b38c0bcd7] (invalid)
[    0.000000] efi: mem10: [type=1107529240|attr=0x0e42280e41300e41] range=[0x300e41058c280e42-0x38010ae54c5c328ee41] (invalid)
[    0.000000] efi: mem11: [type=189335566|attr=0x048d200e42038e18] range=[0x0000318c00000048-0xe42029228ce4200047] (invalid)
[    0.000000] efi: mem12: [type=239142534|attr=0x0000002400000b4b] range=[0x0e41380e0a7d700e-0x80f26238f22bfe500d] (invalid)
[    0.000000] efi: mem14: [type=239207055|attr=0x0e41300e43380e0a] range=[0x8c280e42048d200e-0xc70b028f2f27cc0a00d] (invalid)
[    0.000000] efi: mem15: [type=239210510|attr=0x00080e660b47080e] range=[0x0000324c0000001c-0xa78028634ce490001b] (invalid)
[    0.000000] efi: mem16: [type=4294848528|attr=0x0000329400000014] range=[0x0e410286100e4100-0x80f252036a218f20ff] (invalid)
[    0.000000] efi: mem19: [type=2250772033|attr=0x42180e42200e4328] range=[0x41280e0ab9020683-0xe0e538c28b39e62682] (invalid)
[    0.000000] efi: mem20: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x00000008ffff4438-0xffff44340090333c437] (invalid)
[    0.000000] efi: mem22: [Reserved    |attr=0x000000c1ffff4420] range=[0xffff442400003398-0x1033a04240003f397] (invalid)
[    0.000000] efi: mem23: [type=1141080856|attr=0x080e41100e43180e] range=[0x280e66300e4b280e-0x440dc5ee7141f4c080d] (invalid)
[    0.000000] efi: mem25: [Reserved    |attr=0x0000000affff44a0] range=[0xffff44a400003428-0x1034304a400013427] (invalid)
[    0.000000] efi: mem28: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x0000000affff4488-0xffff448400b034bc487] (invalid)
[    0.000000] efi: mem30: [Reserved    |attr=0x0000000affff4470] range=[0xffff447400003518-0x10352047400013517] (invalid)
[    0.000000] efi: mem33: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x0000000affff4458-0xffff445400b035ac457] (invalid)
[    0.000000] efi: mem35: [type=269372416|attr=0x0e42100e42180e41] range=[0x0486200e44038c18-0x200e8b8a0eee823ac17] (invalid)
[    0.000000] efi: mem37: [type=2351435330|attr=0x0e42100e42180e42] range=[0x470783380e410686-0x2002b2a041c2141e685] (invalid)
[    0.000000] efi: mem38: [type=1093668417|attr=0x100e420000000270] range=[0x42100e42180e4220-0xfff366a4e421b78c21f] (invalid)
[    0.000000] efi: mem39: [type=76357646|attr=0x180e42200e42280e] range=[0x0e410686300e4105-0x4130f251a0710ae5104] (invalid)
[    0.000000] efi: mem40: [type=940444268|attr=0x0e42200e42280e41] range=[0x180e42200e42280e-0x300fc71c300b4f2480d] (invalid)
[    0.000000] efi: mem41: [MMIO        |attr=0x8c280e42048d200e] range=[0xffff479400003728-0x42138e0c87820292727] (invalid)
[    0.000000] efi: mem42: [type=1191674680|attr=0x0000004c0000000b] range=[0x300e41380e0a0246-0x470b0f26238f22b8245] (invalid)
[    0.000000] efi: mem43: [type=2010|attr=0x0301f00e4d078338] range=[0x45038e180e42028f-0xe4556bf118f282528e] (invalid)
[    0.000000] efi: mem44: [type=1109921345|attr=0x300e44000000006c] range=[0x44080e42100e4218-0xfff39254e42138ac217] (invalid)
...

This EFI memap corruption is happening with efi_arch_mem_reserve() invocation in case of kexec boot.

( efi_arch_mem_reserve() is invoked with the following call-stack: )

[    0.310010]  efi_arch_mem_reserve+0xb1/0x220
[    0.311382]  efi_mem_reserve+0x36/0x60
[    0.311973]  efi_bgrt_init+0x17d/0x1a0
[    0.313265]  acpi_parse_bgrt+0x12/0x20
[    0.313858]  acpi_table_parse+0x77/0xd0
[    0.314463]  acpi_boot_init+0x362/0x630
[    0.315069]  setup_arch+0xa88/0xf80
[    0.315629]  start_kernel+0x68/0xa90
[    0.316194]  x86_64_start_reservations+0x1c/0x30
[    0.316921]  x86_64_start_kernel+0xbf/0x110
[    0.317582]  common_startup_64+0x13e/0x141

efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory for
EFI memory map and due to early allocation it uses memblock allocation.

Later during boot, efi_enter_virtual_mode() calls kexec_enter_virtual_mode()
in case of a kexec-ed kernel boot.

This function kexec_enter_virtual_mode() installs the new EFI memory map by
calling efi_memmap_init_late() which remaps the efi_memmap physically allocated
in efi_arch_mem_reserve(), but this remapping is still using memblock allocation.

Subsequently, when memblock is freed later in boot flow, this remapped
efi_memmap will have random corruption (similar to a use-after-free scenario).

The corrupted EFI memory map is then passed to the next kexec-ed kernel
which causes a panic when trying to use the corrupted EFI memory map.

Fix this EFI memory map corruption by skipping efi_arch_mem_reserve() for kexec.

Additionally, skipping this function for kexec altogther makes sense
as for kexec use case need to use the the EFI memmap passed from first
kernel via setup_data and avoid any additional EFI memory map
additions/updates.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/platform/efi/quirks.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index f0cc00032751..af7126d9c540 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -258,6 +258,26 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
 	int num_entries;
 	void *new;
 
+	/*
+	 * efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory for
+	 * EFI memory map and due to early allocation it uses memblock allocation.
+	 * Later during boot, efi_enter_virtual_mode() calls kexec_enter_virtual_mode()
+	 * in case of a kexec-ed kernel boot. This function kexec_enter_virtual_mode()
+	 * installs the new EFI memory map by calling efi_memmap_init_late() which
+	 * remaps the efi_memmap physically allocated here in efi_arch_mem_reserve(),
+	 * but this remapping is still using memblock allocation.
+	 * Subsequently, when memblock is freed later in boot flow, this remapped
+	 * efi_memmap will have random corruption (similar to a use-after-free scenario).
+	 * The corrupted EFI memory map is then passed to the next kexec-ed kernel
+	 * which causes a panic when trying to use the corrupted EFI memory map.
+	 * Additionally, skipping this function for kexec altogther makes sense
+	 * as for kexec use case need to use the the EFI memmap passed from first
+	 * kernel via setup_data and avoid any additional EFI memory map
+	 * additions/updates.
+	 */
+	if (efi_setup)
+		return;
+
 	if (efi_mem_desc_lookup(addr, &md) ||
 	    md.type != EFI_BOOT_SERVICES_DATA) {
 		pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v6 1/3] efi/x86: Fix EFI memory map corruption with kexec

[Ruirui Yang]

On Fri, Apr 26, 2024 at 04:33:48PM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> With SNP guest kexec observe the following efi memmap corruption :
> 
> [    0.000000] efi: EFI v2.7 by EDK II
> [    0.000000] efi: SMBIOS=0x7e33f000 SMBIOS 3.0=0x7e33d000 ACPI=0x7e57e000 ACPI 2.0=0x7e57e014 MEMATTR=0x7cc3c018 Unaccepted=0x7c09e018
> [    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:
> [    0.000000] efi: mem03: [type=269370880|attr=0x0e42100e42180e41] range=[0x0486200e41038c18-0x200e898a0eee713ac17] (invalid)
> [    0.000000] efi: mem04: [type=12336|attr=0x0e410686300e4105] range=[0x100e420000000176-0x8c290f26248d200e175] (invalid)
> [    0.000000] efi: mem06: [type=1124304408|attr=0x000030b400000028] range=[0x0e51300e45280e77-0xb44ed2142f460c1e76] (invalid)
> [    0.000000] efi: mem08: [type=68|attr=0x300e540583280e41] range=[0x0000011affff3cd8-0x486200e54b38c0bcd7] (invalid)
> [    0.000000] efi: mem10: [type=1107529240|attr=0x0e42280e41300e41] range=[0x300e41058c280e42-0x38010ae54c5c328ee41] (invalid)
> [    0.000000] efi: mem11: [type=189335566|attr=0x048d200e42038e18] range=[0x0000318c00000048-0xe42029228ce4200047] (invalid)
> [    0.000000] efi: mem12: [type=239142534|attr=0x0000002400000b4b] range=[0x0e41380e0a7d700e-0x80f26238f22bfe500d] (invalid)
> [    0.000000] efi: mem14: [type=239207055|attr=0x0e41300e43380e0a] range=[0x8c280e42048d200e-0xc70b028f2f27cc0a00d] (invalid)
> [    0.000000] efi: mem15: [type=239210510|attr=0x00080e660b47080e] range=[0x0000324c0000001c-0xa78028634ce490001b] (invalid)
> [    0.000000] efi: mem16: [type=4294848528|attr=0x0000329400000014] range=[0x0e410286100e4100-0x80f252036a218f20ff] (invalid)
> [    0.000000] efi: mem19: [type=2250772033|attr=0x42180e42200e4328] range=[0x41280e0ab9020683-0xe0e538c28b39e62682] (invalid)
> [    0.000000] efi: mem20: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x00000008ffff4438-0xffff44340090333c437] (invalid)
> [    0.000000] efi: mem22: [Reserved    |attr=0x000000c1ffff4420] range=[0xffff442400003398-0x1033a04240003f397] (invalid)
> [    0.000000] efi: mem23: [type=1141080856|attr=0x080e41100e43180e] range=[0x280e66300e4b280e-0x440dc5ee7141f4c080d] (invalid)
> [    0.000000] efi: mem25: [Reserved    |attr=0x0000000affff44a0] range=[0xffff44a400003428-0x1034304a400013427] (invalid)
> [    0.000000] efi: mem28: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x0000000affff4488-0xffff448400b034bc487] (invalid)
> [    0.000000] efi: mem30: [Reserved    |attr=0x0000000affff4470] range=[0xffff447400003518-0x10352047400013517] (invalid)
> [    0.000000] efi: mem33: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x0000000affff4458-0xffff445400b035ac457] (invalid)
> [    0.000000] efi: mem35: [type=269372416|attr=0x0e42100e42180e41] range=[0x0486200e44038c18-0x200e8b8a0eee823ac17] (invalid)
> [    0.000000] efi: mem37: [type=2351435330|attr=0x0e42100e42180e42] range=[0x470783380e410686-0x2002b2a041c2141e685] (invalid)
> [    0.000000] efi: mem38: [type=1093668417|attr=0x100e420000000270] range=[0x42100e42180e4220-0xfff366a4e421b78c21f] (invalid)
> [    0.000000] efi: mem39: [type=76357646|attr=0x180e42200e42280e] range=[0x0e410686300e4105-0x4130f251a0710ae5104] (invalid)
> [    0.000000] efi: mem40: [type=940444268|attr=0x0e42200e42280e41] range=[0x180e42200e42280e-0x300fc71c300b4f2480d] (invalid)
> [    0.000000] efi: mem41: [MMIO        |attr=0x8c280e42048d200e] range=[0xffff479400003728-0x42138e0c87820292727] (invalid)
> [    0.000000] efi: mem42: [type=1191674680|attr=0x0000004c0000000b] range=[0x300e41380e0a0246-0x470b0f26238f22b8245] (invalid)
> [    0.000000] efi: mem43: [type=2010|attr=0x0301f00e4d078338] range=[0x45038e180e42028f-0xe4556bf118f282528e] (invalid)
> [    0.000000] efi: mem44: [type=1109921345|attr=0x300e44000000006c] range=[0x44080e42100e4218-0xfff39254e42138ac217] (invalid)
> ...
> 
> This EFI memap corruption is happening with efi_arch_mem_reserve() invocation in case of kexec boot.
> 
> ( efi_arch_mem_reserve() is invoked with the following call-stack: )
> 
> [    0.310010]  efi_arch_mem_reserve+0xb1/0x220
> [    0.311382]  efi_mem_reserve+0x36/0x60
> [    0.311973]  efi_bgrt_init+0x17d/0x1a0
> [    0.313265]  acpi_parse_bgrt+0x12/0x20
> [    0.313858]  acpi_table_parse+0x77/0xd0
> [    0.314463]  acpi_boot_init+0x362/0x630
> [    0.315069]  setup_arch+0xa88/0xf80
> [    0.315629]  start_kernel+0x68/0xa90
> [    0.316194]  x86_64_start_reservations+0x1c/0x30
> [    0.316921]  x86_64_start_kernel+0xbf/0x110
> [    0.317582]  common_startup_64+0x13e/0x141
> 
> efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory for
> EFI memory map and due to early allocation it uses memblock allocation.
> 
> Later during boot, efi_enter_virtual_mode() calls kexec_enter_virtual_mode()
> in case of a kexec-ed kernel boot.
> 
> This function kexec_enter_virtual_mode() installs the new EFI memory map by
> calling efi_memmap_init_late() which remaps the efi_memmap physically allocated
> in efi_arch_mem_reserve(), but this remapping is still using memblock allocation.
> 
> Subsequently, when memblock is freed later in boot flow, this remapped
> efi_memmap will have random corruption (similar to a use-after-free scenario).
> 
> The corrupted EFI memory map is then passed to the next kexec-ed kernel
> which causes a panic when trying to use the corrupted EFI memory map.
> 
> Fix this EFI memory map corruption by skipping efi_arch_mem_reserve() for kexec.
> 
> Additionally, skipping this function for kexec altogther makes sense
> as for kexec use case need to use the the EFI memmap passed from first
> kernel via setup_data and avoid any additional EFI memory map
> additions/updates.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  arch/x86/platform/efi/quirks.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index f0cc00032751..af7126d9c540 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -258,6 +258,26 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>  	int num_entries;
>  	void *new;
>  
> +	/*
> +	 * efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory for
> +	 * EFI memory map and due to early allocation it uses memblock allocation.
> +	 * Later during boot, efi_enter_virtual_mode() calls kexec_enter_virtual_mode()
> +	 * in case of a kexec-ed kernel boot. This function kexec_enter_virtual_mode()
> +	 * installs the new EFI memory map by calling efi_memmap_init_late() which
> +	 * remaps the efi_memmap physically allocated here in efi_arch_mem_reserve(),
> +	 * but this remapping is still using memblock allocation.
> +	 * Subsequently, when memblock is freed later in boot flow, this remapped
> +	 * efi_memmap will have random corruption (similar to a use-after-free scenario).
> +	 * The corrupted EFI memory map is then passed to the next kexec-ed kernel
> +	 * which causes a panic when trying to use the corrupted EFI memory map.
> +	 * Additionally, skipping this function for kexec altogther makes sense
> +	 * as for kexec use case need to use the the EFI memmap passed from first
> +	 * kernel via setup_data and avoid any additional EFI memory map
> +	 * additions/updates.
> +	 */
> +	if (efi_setup)
> +		return;
> +

efi_mem_reserve is used to reserve boot service memory eg. bgrt, but
it is not necessary for kexec boot, as there are no boot services in
kexec reboot at all after the 1st kernel ExitBootServices().

The UEFI memmap passed to kexec kernel includes not only the runtime
service memory map but also the boot service memory ranges which were
reserved by the 1st kernel with efi_mem_reserve, and those boot service
memory ranges have already been marked "EFI_MEMORY_RUNTIME" attribute. 

Take example of bgrt, the saved memory is there only for people to check
the bgrt image info via /sys/firmware/acpi/bgrt/*, and it is not used in
early boot phase by boot services.

Above is the reason why the efi_mem_reserve can be skipped for kexec
booting.  But as I suggested before I personally think that checking
EFI_MEMORY_RUNTIME attribute set or not looks better than checking
efi_setup.

>  	if (efi_mem_desc_lookup(addr, &md) ||
>  	    md.type != EFI_BOOT_SERVICES_DATA) {
>  		pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
> -- 
> 2.34.1
> 
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v6 1/3] efi/x86: Fix EFI memory map corruption with kexec

[Dave Young]

On Thu, 9 May 2024 at 17:56, Ruirui Yang <ruirui.yang@linux.dev> wrote:
>
> On Fri, Apr 26, 2024 at 04:33:48PM +0000, Ashish Kalra wrote:
> > From: Ashish Kalra <ashish.kalra@amd.com>
> >
> > With SNP guest kexec observe the following efi memmap corruption :
> >
> > [    0.000000] efi: EFI v2.7 by EDK II
> > [    0.000000] efi: SMBIOS=0x7e33f000 SMBIOS 3.0=0x7e33d000 ACPI=0x7e57e000 ACPI 2.0=0x7e57e014 MEMATTR=0x7cc3c018 Unaccepted=0x7c09e018
> > [    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:
> > [    0.000000] efi: mem03: [type=269370880|attr=0x0e42100e42180e41] range=[0x0486200e41038c18-0x200e898a0eee713ac17] (invalid)
> > [    0.000000] efi: mem04: [type=12336|attr=0x0e410686300e4105] range=[0x100e420000000176-0x8c290f26248d200e175] (invalid)
> > [    0.000000] efi: mem06: [type=1124304408|attr=0x000030b400000028] range=[0x0e51300e45280e77-0xb44ed2142f460c1e76] (invalid)
> > [    0.000000] efi: mem08: [type=68|attr=0x300e540583280e41] range=[0x0000011affff3cd8-0x486200e54b38c0bcd7] (invalid)
> > [    0.000000] efi: mem10: [type=1107529240|attr=0x0e42280e41300e41] range=[0x300e41058c280e42-0x38010ae54c5c328ee41] (invalid)
> > [    0.000000] efi: mem11: [type=189335566|attr=0x048d200e42038e18] range=[0x0000318c00000048-0xe42029228ce4200047] (invalid)
> > [    0.000000] efi: mem12: [type=239142534|attr=0x0000002400000b4b] range=[0x0e41380e0a7d700e-0x80f26238f22bfe500d] (invalid)
> > [    0.000000] efi: mem14: [type=239207055|attr=0x0e41300e43380e0a] range=[0x8c280e42048d200e-0xc70b028f2f27cc0a00d] (invalid)
> > [    0.000000] efi: mem15: [type=239210510|attr=0x00080e660b47080e] range=[0x0000324c0000001c-0xa78028634ce490001b] (invalid)
> > [    0.000000] efi: mem16: [type=4294848528|attr=0x0000329400000014] range=[0x0e410286100e4100-0x80f252036a218f20ff] (invalid)
> > [    0.000000] efi: mem19: [type=2250772033|attr=0x42180e42200e4328] range=[0x41280e0ab9020683-0xe0e538c28b39e62682] (invalid)
> > [    0.000000] efi: mem20: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x00000008ffff4438-0xffff44340090333c437] (invalid)
> > [    0.000000] efi: mem22: [Reserved    |attr=0x000000c1ffff4420] range=[0xffff442400003398-0x1033a04240003f397] (invalid)
> > [    0.000000] efi: mem23: [type=1141080856|attr=0x080e41100e43180e] range=[0x280e66300e4b280e-0x440dc5ee7141f4c080d] (invalid)
> > [    0.000000] efi: mem25: [Reserved    |attr=0x0000000affff44a0] range=[0xffff44a400003428-0x1034304a400013427] (invalid)
> > [    0.000000] efi: mem28: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x0000000affff4488-0xffff448400b034bc487] (invalid)
> > [    0.000000] efi: mem30: [Reserved    |attr=0x0000000affff4470] range=[0xffff447400003518-0x10352047400013517] (invalid)
> > [    0.000000] efi: mem33: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x0000000affff4458-0xffff445400b035ac457] (invalid)
> > [    0.000000] efi: mem35: [type=269372416|attr=0x0e42100e42180e41] range=[0x0486200e44038c18-0x200e8b8a0eee823ac17] (invalid)
> > [    0.000000] efi: mem37: [type=2351435330|attr=0x0e42100e42180e42] range=[0x470783380e410686-0x2002b2a041c2141e685] (invalid)
> > [    0.000000] efi: mem38: [type=1093668417|attr=0x100e420000000270] range=[0x42100e42180e4220-0xfff366a4e421b78c21f] (invalid)
> > [    0.000000] efi: mem39: [type=76357646|attr=0x180e42200e42280e] range=[0x0e410686300e4105-0x4130f251a0710ae5104] (invalid)
> > [    0.000000] efi: mem40: [type=940444268|attr=0x0e42200e42280e41] range=[0x180e42200e42280e-0x300fc71c300b4f2480d] (invalid)
> > [    0.000000] efi: mem41: [MMIO        |attr=0x8c280e42048d200e] range=[0xffff479400003728-0x42138e0c87820292727] (invalid)
> > [    0.000000] efi: mem42: [type=1191674680|attr=0x0000004c0000000b] range=[0x300e41380e0a0246-0x470b0f26238f22b8245] (invalid)
> > [    0.000000] efi: mem43: [type=2010|attr=0x0301f00e4d078338] range=[0x45038e180e42028f-0xe4556bf118f282528e] (invalid)
> > [    0.000000] efi: mem44: [type=1109921345|attr=0x300e44000000006c] range=[0x44080e42100e4218-0xfff39254e42138ac217] (invalid)
> > ...
> >
> > This EFI memap corruption is happening with efi_arch_mem_reserve() invocation in case of kexec boot.
> >
> > ( efi_arch_mem_reserve() is invoked with the following call-stack: )
> >
> > [    0.310010]  efi_arch_mem_reserve+0xb1/0x220
> > [    0.311382]  efi_mem_reserve+0x36/0x60
> > [    0.311973]  efi_bgrt_init+0x17d/0x1a0
> > [    0.313265]  acpi_parse_bgrt+0x12/0x20
> > [    0.313858]  acpi_table_parse+0x77/0xd0
> > [    0.314463]  acpi_boot_init+0x362/0x630
> > [    0.315069]  setup_arch+0xa88/0xf80
> > [    0.315629]  start_kernel+0x68/0xa90
> > [    0.316194]  x86_64_start_reservations+0x1c/0x30
> > [    0.316921]  x86_64_start_kernel+0xbf/0x110
> > [    0.317582]  common_startup_64+0x13e/0x141
> >
> > efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory for
> > EFI memory map and due to early allocation it uses memblock allocation.
> >
> > Later during boot, efi_enter_virtual_mode() calls kexec_enter_virtual_mode()
> > in case of a kexec-ed kernel boot.
> >
> > This function kexec_enter_virtual_mode() installs the new EFI memory map by
> > calling efi_memmap_init_late() which remaps the efi_memmap physically allocated
> > in efi_arch_mem_reserve(), but this remapping is still using memblock allocation.
> >
> > Subsequently, when memblock is freed later in boot flow, this remapped
> > efi_memmap will have random corruption (similar to a use-after-free scenario).
> >
> > The corrupted EFI memory map is then passed to the next kexec-ed kernel
> > which causes a panic when trying to use the corrupted EFI memory map.
> >
> > Fix this EFI memory map corruption by skipping efi_arch_mem_reserve() for kexec.
> >
> > Additionally, skipping this function for kexec altogther makes sense
> > as for kexec use case need to use the the EFI memmap passed from first
> > kernel via setup_data and avoid any additional EFI memory map
> > additions/updates.
> >
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  arch/x86/platform/efi/quirks.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> > index f0cc00032751..af7126d9c540 100644
> > --- a/arch/x86/platform/efi/quirks.c
> > +++ b/arch/x86/platform/efi/quirks.c
> > @@ -258,6 +258,26 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
> >       int num_entries;
> >       void *new;
> >
> > +     /*
> > +      * efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory for
> > +      * EFI memory map and due to early allocation it uses memblock allocation.
> > +      * Later during boot, efi_enter_virtual_mode() calls kexec_enter_virtual_mode()
> > +      * in case of a kexec-ed kernel boot. This function kexec_enter_virtual_mode()
> > +      * installs the new EFI memory map by calling efi_memmap_init_late() which
> > +      * remaps the efi_memmap physically allocated here in efi_arch_mem_reserve(),
> > +      * but this remapping is still using memblock allocation.
> > +      * Subsequently, when memblock is freed later in boot flow, this remapped
> > +      * efi_memmap will have random corruption (similar to a use-after-free scenario).
> > +      * The corrupted EFI memory map is then passed to the next kexec-ed kernel
> > +      * which causes a panic when trying to use the corrupted EFI memory map.
> > +      * Additionally, skipping this function for kexec altogther makes sense
> > +      * as for kexec use case need to use the the EFI memmap passed from first
> > +      * kernel via setup_data and avoid any additional EFI memory map
> > +      * additions/updates.
> > +      */
> > +     if (efi_setup)
> > +             return;
> > +
>
> efi_mem_reserve is used to reserve boot service memory eg. bgrt, but
> it is not necessary for kexec boot, as there are no boot services in
> kexec reboot at all after the 1st kernel ExitBootServices().
>
> The UEFI memmap passed to kexec kernel includes not only the runtime
> service memory map but also the boot service memory ranges which were
> reserved by the 1st kernel with efi_mem_reserve, and those boot service
> memory ranges have already been marked "EFI_MEMORY_RUNTIME" attribute.
>
> Take example of bgrt, the saved memory is there only for people to check
> the bgrt image info via /sys/firmware/acpi/bgrt/*, and it is not used in
> early boot phase by boot services.
>
> Above is the reason why the efi_mem_reserve can be skipped for kexec
> booting.  But as I suggested before I personally think that checking
> EFI_MEMORY_RUNTIME attribute set or not looks better than checking
> efi_setup.
>

I recently applied the linux.dev mail with my Chinese pinyin name  for
use when I do not have vpn access.   So just to clarify a bit, I'm the
same person here :)


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v6 1/3] efi/x86: Fix EFI memory map corruption with kexec

[Kalra, Ashish]

On 5/9/2024 4:56 AM, Ruirui Yang wrote:

> On Fri, Apr 26, 2024 at 04:33:48PM +0000, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> With SNP guest kexec observe the following efi memmap corruption :
>>
>> [    0.000000] efi: EFI v2.7 by EDK II
>> [    0.000000] efi: SMBIOS=0x7e33f000 SMBIOS 3.0=0x7e33d000 ACPI=0x7e57e000 ACPI 2.0=0x7e57e014 MEMATTR=0x7cc3c018 Unaccepted=0x7c09e018
>> [    0.000000] efi: [Firmware Bug]: Invalid EFI memory map entries:
>> [    0.000000] efi: mem03: [type=269370880|attr=0x0e42100e42180e41] range=[0x0486200e41038c18-0x200e898a0eee713ac17] (invalid)
>> [    0.000000] efi: mem04: [type=12336|attr=0x0e410686300e4105] range=[0x100e420000000176-0x8c290f26248d200e175] (invalid)
>> [    0.000000] efi: mem06: [type=1124304408|attr=0x000030b400000028] range=[0x0e51300e45280e77-0xb44ed2142f460c1e76] (invalid)
>> [    0.000000] efi: mem08: [type=68|attr=0x300e540583280e41] range=[0x0000011affff3cd8-0x486200e54b38c0bcd7] (invalid)
>> [    0.000000] efi: mem10: [type=1107529240|attr=0x0e42280e41300e41] range=[0x300e41058c280e42-0x38010ae54c5c328ee41] (invalid)
>> [    0.000000] efi: mem11: [type=189335566|attr=0x048d200e42038e18] range=[0x0000318c00000048-0xe42029228ce4200047] (invalid)
>> [    0.000000] efi: mem12: [type=239142534|attr=0x0000002400000b4b] range=[0x0e41380e0a7d700e-0x80f26238f22bfe500d] (invalid)
>> [    0.000000] efi: mem14: [type=239207055|attr=0x0e41300e43380e0a] range=[0x8c280e42048d200e-0xc70b028f2f27cc0a00d] (invalid)
>> [    0.000000] efi: mem15: [type=239210510|attr=0x00080e660b47080e] range=[0x0000324c0000001c-0xa78028634ce490001b] (invalid)
>> [    0.000000] efi: mem16: [type=4294848528|attr=0x0000329400000014] range=[0x0e410286100e4100-0x80f252036a218f20ff] (invalid)
>> [    0.000000] efi: mem19: [type=2250772033|attr=0x42180e42200e4328] range=[0x41280e0ab9020683-0xe0e538c28b39e62682] (invalid)
>> [    0.000000] efi: mem20: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x00000008ffff4438-0xffff44340090333c437] (invalid)
>> [    0.000000] efi: mem22: [Reserved    |attr=0x000000c1ffff4420] range=[0xffff442400003398-0x1033a04240003f397] (invalid)
>> [    0.000000] efi: mem23: [type=1141080856|attr=0x080e41100e43180e] range=[0x280e66300e4b280e-0x440dc5ee7141f4c080d] (invalid)
>> [    0.000000] efi: mem25: [Reserved    |attr=0x0000000affff44a0] range=[0xffff44a400003428-0x1034304a400013427] (invalid)
>> [    0.000000] efi: mem28: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x0000000affff4488-0xffff448400b034bc487] (invalid)
>> [    0.000000] efi: mem30: [Reserved    |attr=0x0000000affff4470] range=[0xffff447400003518-0x10352047400013517] (invalid)
>> [    0.000000] efi: mem33: [type=16|   |  |  |  |  |  |  |  |  |   |WB|  |WC|  ] range=[0x0000000affff4458-0xffff445400b035ac457] (invalid)
>> [    0.000000] efi: mem35: [type=269372416|attr=0x0e42100e42180e41] range=[0x0486200e44038c18-0x200e8b8a0eee823ac17] (invalid)
>> [    0.000000] efi: mem37: [type=2351435330|attr=0x0e42100e42180e42] range=[0x470783380e410686-0x2002b2a041c2141e685] (invalid)
>> [    0.000000] efi: mem38: [type=1093668417|attr=0x100e420000000270] range=[0x42100e42180e4220-0xfff366a4e421b78c21f] (invalid)
>> [    0.000000] efi: mem39: [type=76357646|attr=0x180e42200e42280e] range=[0x0e410686300e4105-0x4130f251a0710ae5104] (invalid)
>> [    0.000000] efi: mem40: [type=940444268|attr=0x0e42200e42280e41] range=[0x180e42200e42280e-0x300fc71c300b4f2480d] (invalid)
>> [    0.000000] efi: mem41: [MMIO        |attr=0x8c280e42048d200e] range=[0xffff479400003728-0x42138e0c87820292727] (invalid)
>> [    0.000000] efi: mem42: [type=1191674680|attr=0x0000004c0000000b] range=[0x300e41380e0a0246-0x470b0f26238f22b8245] (invalid)
>> [    0.000000] efi: mem43: [type=2010|attr=0x0301f00e4d078338] range=[0x45038e180e42028f-0xe4556bf118f282528e] (invalid)
>> [    0.000000] efi: mem44: [type=1109921345|attr=0x300e44000000006c] range=[0x44080e42100e4218-0xfff39254e42138ac217] (invalid)
>> ...
>>
>> This EFI memap corruption is happening with efi_arch_mem_reserve() invocation in case of kexec boot.
>>
>> ( efi_arch_mem_reserve() is invoked with the following call-stack: )
>>
>> [    0.310010]  efi_arch_mem_reserve+0xb1/0x220
>> [    0.311382]  efi_mem_reserve+0x36/0x60
>> [    0.311973]  efi_bgrt_init+0x17d/0x1a0
>> [    0.313265]  acpi_parse_bgrt+0x12/0x20
>> [    0.313858]  acpi_table_parse+0x77/0xd0
>> [    0.314463]  acpi_boot_init+0x362/0x630
>> [    0.315069]  setup_arch+0xa88/0xf80
>> [    0.315629]  start_kernel+0x68/0xa90
>> [    0.316194]  x86_64_start_reservations+0x1c/0x30
>> [    0.316921]  x86_64_start_kernel+0xbf/0x110
>> [    0.317582]  common_startup_64+0x13e/0x141
>>
>> efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory for
>> EFI memory map and due to early allocation it uses memblock allocation.
>>
>> Later during boot, efi_enter_virtual_mode() calls kexec_enter_virtual_mode()
>> in case of a kexec-ed kernel boot.
>>
>> This function kexec_enter_virtual_mode() installs the new EFI memory map by
>> calling efi_memmap_init_late() which remaps the efi_memmap physically allocated
>> in efi_arch_mem_reserve(), but this remapping is still using memblock allocation.
>>
>> Subsequently, when memblock is freed later in boot flow, this remapped
>> efi_memmap will have random corruption (similar to a use-after-free scenario).
>>
>> The corrupted EFI memory map is then passed to the next kexec-ed kernel
>> which causes a panic when trying to use the corrupted EFI memory map.
>>
>> Fix this EFI memory map corruption by skipping efi_arch_mem_reserve() for kexec.
>>
>> Additionally, skipping this function for kexec altogther makes sense
>> as for kexec use case need to use the the EFI memmap passed from first
>> kernel via setup_data and avoid any additional EFI memory map
>> additions/updates.
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>   arch/x86/platform/efi/quirks.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index f0cc00032751..af7126d9c540 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -258,6 +258,26 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>>   	int num_entries;
>>   	void *new;
>>   
>> +	/*
>> +	 * efi_arch_mem_reserve() calls efi_memmap_alloc() to allocate memory for
>> +	 * EFI memory map and due to early allocation it uses memblock allocation.
>> +	 * Later during boot, efi_enter_virtual_mode() calls kexec_enter_virtual_mode()
>> +	 * in case of a kexec-ed kernel boot. This function kexec_enter_virtual_mode()
>> +	 * installs the new EFI memory map by calling efi_memmap_init_late() which
>> +	 * remaps the efi_memmap physically allocated here in efi_arch_mem_reserve(),
>> +	 * but this remapping is still using memblock allocation.
>> +	 * Subsequently, when memblock is freed later in boot flow, this remapped
>> +	 * efi_memmap will have random corruption (similar to a use-after-free scenario).
>> +	 * The corrupted EFI memory map is then passed to the next kexec-ed kernel
>> +	 * which causes a panic when trying to use the corrupted EFI memory map.
>> +	 * Additionally, skipping this function for kexec altogther makes sense
>> +	 * as for kexec use case need to use the the EFI memmap passed from first
>> +	 * kernel via setup_data and avoid any additional EFI memory map
>> +	 * additions/updates.
>> +	 */
>> +	if (efi_setup)
>> +		return;
>> +
> efi_mem_reserve is used to reserve boot service memory eg. bgrt, but
> it is not necessary for kexec boot, as there are no boot services in
> kexec reboot at all after the 1st kernel ExitBootServices().
>
> The UEFI memmap passed to kexec kernel includes not only the runtime
> service memory map but also the boot service memory ranges which were
> reserved by the 1st kernel with efi_mem_reserve, and those boot service
> memory ranges have already been marked "EFI_MEMORY_RUNTIME" attribute.
>
> Take example of bgrt, the saved memory is there only for people to check
> the bgrt image info via /sys/firmware/acpi/bgrt/*, and it is not used in
> early boot phase by boot services.
>
> Above is the reason why the efi_mem_reserve can be skipped for kexec
> booting.  But as I suggested before I personally think that checking
> EFI_MEMORY_RUNTIME attribute set or not looks better than checking
> efi_setup.

Thanks for reviewing the patch.

I will move back to checking the md attribute instead of checking 
efi_setup as i was doing previously and resubmit this patch.

Thanks, Ashish

>
>>   	if (efi_mem_desc_lookup(addr, &md) ||
>>   	    md.type != EFI_BOOT_SERVICES_DATA) {
>>   		pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
>> -- 
>> 2.34.1
>>
>>

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v6 2/3] x86/boot/compressed: Skip Video Memory access in Decompressor for SEV-ES/SNP.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Accessing guest video memory/RAM during kernel decompressor
causes guest termination as boot stage2 #VC handler for
SEV-ES/SNP systems does not support MMIO handling.

This issue is observed with SEV-ES/SNP guest kexec as
kexec -c adds screen_info to the boot parameters
passed to the kexec kernel, which causes console output to
be dumped to both video and serial.

As the decompressor output gets cleared really fast, it is
preferable to get the console output only on serial, hence,
skip accessing video RAM during decompressor stage to
prevent guest termination.

Serial console output during decompressor stage works as
boot stage2 #VC handler already supports handling port I/O.

Suggested-by: Thomas Lendacy <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/boot/compressed/misc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index b70e4a21c15f..3b9f96b3dbcc 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -427,8 +427,10 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
 		vidport = 0x3d4;
 	}
 
-	lines = boot_params_ptr->screen_info.orig_video_lines;
-	cols = boot_params_ptr->screen_info.orig_video_cols;
+	if (!(sev_status & MSR_AMD64_SEV_ES_ENABLED)) {
+		lines = boot_params_ptr->screen_info.orig_video_lines;
+		cols = boot_params_ptr->screen_info.orig_video_cols;
+	}
 
 	init_default_io_ops();
 
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v6 3/3] x86/snp: Convert shared memory back to private on kexec

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

SNP guests allocate shared buffers to perform I/O. It is done by
allocating pages normally from the buddy allocator and converting them
to shared with set_memory_decrypted().

The second kernel has no idea what memory is converted this way. It only
sees E820_TYPE_RAM.

Accessing shared memory via private mapping will cause unrecoverable RMP
page-faults.

On kexec walk direct mapping and convert all shared memory back to
private. It makes all RAM private again and second kernel may use it
normally. Additionally for SNP guests convert all bss decrypted section
pages back to private and switch back ROM regions to shared so that
their revalidation does not fail during kexec kernel boot.

The conversion occurs in two steps: stopping new conversions and
unsharing all memory. In the case of normal kexec, the stopping of
conversions takes place while scheduling is still functioning. This
allows for waiting until any ongoing conversions are finished. The
second step is carried out when all CPUs except one are inactive and
interrupts are disabled. This prevents any conflicts with code that may
access shared memory.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 arch/x86/include/asm/sev.h    |   4 +
 arch/x86/kernel/sev.c         | 161 ++++++++++++++++++++++++++++++++++
 arch/x86/mm/mem_encrypt_amd.c |   3 +
 3 files changed, 168 insertions(+)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 7f57382afee4..78d40d08d201 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -229,6 +229,8 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end);
 u64 snp_get_unsupported_features(u64 status);
 u64 sev_get_status(void);
 void sev_show_status(void);
+void snp_kexec_unshare_mem(void);
+void snp_kexec_stop_conversion(bool crash);
 #else
 static inline void sev_es_ist_enter(struct pt_regs *regs) { }
 static inline void sev_es_ist_exit(void) { }
@@ -258,6 +260,8 @@ static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
 static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
 static inline u64 sev_get_status(void) { return 0; }
 static inline void sev_show_status(void) { }
+static inline void snp_kexec_unshare_mem(void) { }
+static inline void snp_kexec_stop_conversion(bool crash) { }
 #endif
 
 #ifdef CONFIG_KVM_AMD_SEV
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 995f94467101..891257fde810 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -42,6 +42,8 @@
 #include <asm/apic.h>
 #include <asm/cpuid.h>
 #include <asm/cmdline.h>
+#include <asm/pgtable.h>
+#include <asm/set_memory.h>
 
 #define DR7_RESET_VALUE        0x400
 
@@ -92,6 +94,9 @@ static struct ghcb *boot_ghcb __section(".data");
 /* Bitmap of SEV features supported by the hypervisor */
 static u64 sev_hv_features __ro_after_init;
 
+/* Last address to be switched to private during kexec */
+static unsigned long kexec_last_addr_to_make_private;
+
 /* #VC handler runtime per-CPU data */
 struct sev_es_runtime_data {
 	struct ghcb ghcb_page;
@@ -913,6 +918,162 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end)
 	set_pages_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE);
 }
 
+static bool set_pte_enc(pte_t *kpte, int level, void *va)
+{
+	pte_t new_pte;
+
+	if (pte_none(*kpte))
+		return false;
+
+	/*
+	 * Change the physical page attribute from C=0 to C=1. Flush the
+	 * caches to ensure that data gets accessed with the correct C-bit.
+	 */
+	if (pte_present(*kpte))
+		clflush_cache_range(va, page_level_size(level));
+
+	new_pte = __pte(cc_mkenc(pte_val(*kpte)));
+	set_pte_atomic(kpte, new_pte);
+
+	return true;
+}
+
+static bool make_pte_private(pte_t *pte, unsigned long addr, int pages, int level)
+{
+	struct sev_es_runtime_data *data;
+	struct ghcb *ghcb;
+
+	data = this_cpu_read(runtime_data);
+	ghcb = &data->ghcb_page;
+
+	/* Check for GHCB for being part of a PMD range. */
+	if ((unsigned long)ghcb >= addr &&
+	    (unsigned long)ghcb <= (addr + (pages * PAGE_SIZE))) {
+		/*
+		 * Ensure that the current cpu's GHCB is made private
+		 * at the end of unshared loop so that we continue to use the
+		 * optimized GHCB protocol and not force the switch to
+		 * MSR protocol till the very end.
+		 */
+		pr_debug("setting boot_ghcb to NULL for this cpu ghcb\n");
+		kexec_last_addr_to_make_private = addr;
+		return true;
+	}
+
+	if (!set_pte_enc(pte, level, (void *)addr))
+		return false;
+
+	snp_set_memory_private(addr, pages);
+
+	return true;
+}
+
+static void unshare_all_memory(void)
+{
+	unsigned long addr, end;
+
+	/*
+	 * Walk direct mapping and convert all shared memory back to private,
+	 */
+
+	addr = PAGE_OFFSET;
+	end  = PAGE_OFFSET + get_max_mapped();
+
+	while (addr < end) {
+		unsigned long size;
+		unsigned int level;
+		pte_t *pte;
+
+		pte = lookup_address(addr, &level);
+		size = page_level_size(level);
+
+		/*
+		 * pte_none() check is required to skip physical memory holes in direct mapped.
+		 */
+		if (pte && pte_decrypted(*pte) && !pte_none(*pte)) {
+			int pages = size / PAGE_SIZE;
+
+			if (!make_pte_private(pte, addr, pages, level)) {
+				pr_err("Failed to unshare range %#lx-%#lx\n",
+				       addr, addr + size);
+			}
+
+		}
+
+		addr += size;
+	}
+	__flush_tlb_all();
+
+}
+
+static void unshare_all_bss_decrypted_memory(void)
+{
+	unsigned long vaddr, vaddr_end;
+	unsigned int level;
+	unsigned int npages;
+	pte_t *pte;
+
+	vaddr = (unsigned long)__start_bss_decrypted;
+	vaddr_end = (unsigned long)__start_bss_decrypted_unused;
+	npages = (vaddr_end - vaddr) >> PAGE_SHIFT;
+	for (; vaddr < vaddr_end; vaddr += PAGE_SIZE) {
+		pte = lookup_address(vaddr, &level);
+		if (!pte || !pte_decrypted(*pte) || pte_none(*pte))
+			continue;
+
+		set_pte_enc(pte, level, (void *)vaddr);
+	}
+	vaddr = (unsigned long)__start_bss_decrypted;
+	snp_set_memory_private(vaddr, npages);
+}
+
+/* Stop new private<->shared conversions */
+void snp_kexec_stop_conversion(bool crash)
+{
+	/*
+	 * Crash kernel reaches here with interrupts disabled: can't wait for
+	 * conversions to finish.
+	 *
+	 * If race happened, just report and proceed.
+	 */
+	bool wait_for_lock = !crash;
+
+	if (!stop_memory_enc_conversion(wait_for_lock))
+		pr_warn("Failed to finish shared<->private conversions\n");
+}
+
+void snp_kexec_unshare_mem(void)
+{
+	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
+		return;
+
+	unshare_all_memory();
+
+	unshare_all_bss_decrypted_memory();
+
+	if (kexec_last_addr_to_make_private) {
+		unsigned long size;
+		unsigned int level;
+		pte_t *pte;
+
+		/*
+		 * Switch to using the MSR protocol to change this cpu's
+		 * GHCB to private.
+		 * All the per-cpu GHCBs have been switched back to private,
+		 * so can't do any more GHCB calls to the hypervisor beyond
+		 * this point till the kexec kernel starts running.
+		 */
+		boot_ghcb = NULL;
+		sev_cfg.ghcbs_initialized = false;
+
+		pr_debug("boot ghcb 0x%lx\n", kexec_last_addr_to_make_private);
+		pte = lookup_address(kexec_last_addr_to_make_private, &level);
+		size = page_level_size(level);
+		set_pte_enc(pte, level, (void *)kexec_last_addr_to_make_private);
+		snp_set_memory_private(kexec_last_addr_to_make_private, (size / PAGE_SIZE));
+	}
+}
+
 static int snp_set_vmsa(void *va, bool vmsa)
 {
 	u64 attrs;
diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
index e7b67519ddb5..49c40c2ed809 100644
--- a/arch/x86/mm/mem_encrypt_amd.c
+++ b/arch/x86/mm/mem_encrypt_amd.c
@@ -468,6 +468,9 @@ void __init sme_early_init(void)
 	x86_platform.guest.enc_tlb_flush_required    = amd_enc_tlb_flush_required;
 	x86_platform.guest.enc_cache_flush_required  = amd_enc_cache_flush_required;
 
+	x86_platform.guest.enc_kexec_stop_conversion = snp_kexec_stop_conversion;
+	x86_platform.guest.enc_kexec_unshare_mem     = snp_kexec_unshare_mem;
+
 	/*
 	 * AMD-SEV-ES intercepts the RDMSR to read the X2APIC ID in the
 	 * parallel bringup low level code. That raises #VC which cannot be
-- 
2.34.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 0/4] x86/snp: Add kexec support

[Alexander Graf]

Hey Ashish,

On 09.04.24 22:42, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> The patchset adds bits and pieces to get kexec (and crashkernel) work on
> SNP guest.


With this patch set (and similar for the TDX one), you enable the 
typical kdump case, which is great!

However, if a user is running with direct kernel boot - which is very 
typical in SEV-SNP setup, especially for Kata Containers and similar - 
the initial launch measurement is a natural indicator of the target 
environment. Kexec basically allows them to completely bypass that: You 
would be able to run a completely different environment than the one you 
measure through the launch digest. I'm not sure it's a good idea to even 
allow that by default in CoCo environments - at least not if the kernel 
is locked down.

Do you have any plans to build a CoCo native kexec where you allow a VM 
to create a new VM context with a guest provided seed? The new context 
could rerun all of the attestation and so enable users to generate a new 
launch digest. If you then atomically swap into the new context, it 
would in turn enable them to natively "kexec" into a completely new VM 
context including measurements.

I understand that an SVSM + TPM implementation may help to some extent 
here by integrating with IMA and adding the new kernel into the IMA log. 
But that quickly becomes very convoluted (hence difficult to assess 
correctness for) and the same measurement question arises just one level 
up then: How do you update your SVSM while maintaining a full 
measurement and trust chain?


Thanks,

Alex


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 0/4] x86/snp: Add kexec support

[Vitaly Kuznetsov]

Alexander Graf <graf@amazon.com> writes:

> Hey Ashish,
>
> On 09.04.24 22:42, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> The patchset adds bits and pieces to get kexec (and crashkernel) work on
>> SNP guest.
>
>
> With this patch set (and similar for the TDX one), you enable the 
> typical kdump case, which is great!
>
> However, if a user is running with direct kernel boot - which is very 
> typical in SEV-SNP setup, especially for Kata Containers and similar - 
> the initial launch measurement is a natural indicator of the target 
> environment. Kexec basically allows them to completely bypass that: You 
> would be able to run a completely different environment than the one you 
> measure through the launch digest. I'm not sure it's a good idea to even 
> allow that by default in CoCo environments - at least not if the kernel 
> is locked down.

Isn't it the same when we just allow loading kernel modules? I'm sure
you can also achieve a 'completely different environment' with that :-)
With SecureBoot / lockdown we normally require modules to pass signature
check, I guess we can employ the same mechanism for kexec. I.e. in
lockdown, we require signature check on the kexec-ed kernel. Also, it
may make sense to check initramfs too (with direct kernel boot it's also
part of launch measurements, right?) and there's UKI for that already).

Personally, I believe that if we simply forbid kexec for CoCo in
lockdown mode, the feature will become mostly useless in 'full stack'
(which boot through firmware) production envrironments.

-- 
Vitaly


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 0/4] x86/snp: Add kexec support

[Alexander Graf]

On 02.05.24 14:18, Vitaly Kuznetsov wrote:
> Alexander Graf <graf@amazon.com> writes:
>
>> Hey Ashish,
>>
>> On 09.04.24 22:42, Ashish Kalra wrote:
>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>
>>> The patchset adds bits and pieces to get kexec (and crashkernel) work on
>>> SNP guest.
>>
>> With this patch set (and similar for the TDX one), you enable the
>> typical kdump case, which is great!
>>
>> However, if a user is running with direct kernel boot - which is very
>> typical in SEV-SNP setup, especially for Kata Containers and similar -
>> the initial launch measurement is a natural indicator of the target
>> environment. Kexec basically allows them to completely bypass that: You
>> would be able to run a completely different environment than the one you
>> measure through the launch digest. I'm not sure it's a good idea to even
>> allow that by default in CoCo environments - at least not if the kernel
>> is locked down.
> Isn't it the same when we just allow loading kernel modules? I'm sure
> you can also achieve a 'completely different environment' with that :-)
> With SecureBoot / lockdown we normally require modules to pass signature
> check, I guess we can employ the same mechanism for kexec. I.e. in
> lockdown, we require signature check on the kexec-ed kernel. Also, it
> may make sense to check initramfs too (with direct kernel boot it's also
> part of launch measurements, right?) and there's UKI for that already).


Correct. With IMA, you even do exactly that: Enforce a signature check 
of the next binary with kexec.

The problem is that you typically want to update the system because 
something is broken; most likely your original environment had a 
security issue somewhere. From a pure SEV-SNP attestation point of view, 
you can not distinguish between the patched and unpatched environment: 
Both look the same.

So while kexec isn't the problem, it's the fact that you can't tell 
anyone that you're now running a fixed version of the code :).


> Personally, I believe that if we simply forbid kexec for CoCo in
> lockdown mode, the feature will become mostly useless in 'full stack'
> (which boot through firmware) production envrironments.


I'm happy for CoCo to stay smoke and mirrors :). But I believe that if 
you want to genuinely draw a trust chain back to an AMD/Intel 
certificate, we need to come up with a good way of making updates work 
with a working trust chain so that whoever checks whether you're running 
sanctioned code is able to validate the claim.


Alex


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 0/4] x86/snp: Add kexec support

[Vitaly Kuznetsov]

Alexander Graf <graf@amazon.com> writes:

> Correct. With IMA, you even do exactly that: Enforce a signature check 
> of the next binary with kexec.
>
> The problem is that you typically want to update the system because 
> something is broken; most likely your original environment had a 
> security issue somewhere. From a pure SEV-SNP attestation point of view, 
> you can not distinguish between the patched and unpatched environment: 
> Both look the same.
>
> So while kexec isn't the problem, it's the fact that you can't tell 
> anyone that you're now running a fixed version of the code :).

...

>
> I'm happy for CoCo to stay smoke and mirrors :). 

"Only a Sith deals in absolutes" :-)

> But I believe that if 
> you want to genuinely draw a trust chain back to an AMD/Intel 
> certificate, we need to come up with a good way of making updates work 
> with a working trust chain so that whoever checks whether you're running 
> sanctioned code is able to validate the claim.

Launch measurements are what they are, they describe the state of your
guest before it started booting. There are multiple mechanisms in Linux
which change CPL0 code already: self-modifying code like static keys,
loadable modules, runtime patching, kexec,... In case some specific
deployment requires stronger guarantees we can probably introduce
something like 'full lockdown' mode (as a compile time option, I guess)
which would disable all of the aforementioned mechanisms. It will still
not be a hard proof that the running code matches launch measurements
(because vulnerabilities/bugs may still exist) I guess but could be an
improvement.

Basically, what I wanted to argue is that kexec does not need to be
treated 'specially' for CVMs if we keep all other ways to modify kernel
code. Making these methods 'attestable' is currently a challenge indeed.

-- 
Vitaly


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 0/4] x86/snp: Add kexec support

[Kalra, Ashish]

Hello Alexander,

On 5/2/2024 7:01 AM, Alexander Graf wrote:
> Hey Ashish,
>
> On 09.04.24 22:42, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> The patchset adds bits and pieces to get kexec (and crashkernel) work on
>> SNP guest.
>
>
> With this patch set (and similar for the TDX one), you enable the 
> typical kdump case, which is great!
>
> However, if a user is running with direct kernel boot - which is very 
> typical in SEV-SNP setup, especially for Kata Containers and similar - 
> the initial launch measurement is a natural indicator of the target 
> environment. Kexec basically allows them to completely bypass that: 
> You would be able to run a completely different environment than the 
> one you measure through the launch digest. I'm not sure it's a good 
> idea to even allow that by default in CoCo environments - at least not 
> if the kernel is locked down.
>
I thought that kexec is disabled if kernel is in locked-down mode.

Or is it that KEXEC_LOAD syscall is not supported/disabled in kernel 
locked-down mode and KEXEC_FILE_LOAD syscall is supported ?

> Do you have any plans to build a CoCo native kexec where you allow a 
> VM to create a new VM context with a guest provided seed? The new 
> context could rerun all of the attestation and so enable users to 
> generate a new launch digest. If you then atomically swap into the new 
> context, it would in turn enable them to natively "kexec" into a 
> completely new VM context including measurements.

No, currently i don't think there any any such plans.

Thanks, Ashish

>
> I understand that an SVSM + TPM implementation may help to some extent 
> here by integrating with IMA and adding the new kernel into the IMA 
> log. But that quickly becomes very convoluted (hence difficult to 
> assess correctness for) and the same measurement question arises just 
> one level up then: How do you update your SVSM while maintaining a 
> full measurement and trust chain?
>
>
> Thanks,
>
> Alex
>

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 03/18] cpu/hotplug: Add support for declaring CPU offlining not supported

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:29:55PM +0300, Kirill A. Shutemov wrote:
> +/* Declare CPU offlining not supported */
> +void cpu_hotplug_disable_offlining(void)
> +{
> +	cpu_maps_update_begin();

"/*
 * The following two APIs (cpu_maps_update_begin/done) must be used when
 * attempting to serialize the updates to cpu_online_mask & cpu_present_mask.
 */
void cpu_maps_update_begin(void)
..."

> +	cpu_hotplug_offline_disabled = true;

but this doesn't do that here.

Are we doing a one-off here for that variable or what?

> +	cpu_maps_update_done();

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 03/18] cpu/hotplug: Add support for declaring CPU offlining not supported

[Kirill A. Shutemov]

On Thu, Apr 18, 2024 at 04:37:09PM +0200, Borislav Petkov wrote:
> On Tue, Apr 09, 2024 at 02:29:55PM +0300, Kirill A. Shutemov wrote:
> > +/* Declare CPU offlining not supported */
> > +void cpu_hotplug_disable_offlining(void)
> > +{
> > +	cpu_maps_update_begin();
> 
> "/*
>  * The following two APIs (cpu_maps_update_begin/done) must be used when
>  * attempting to serialize the updates to cpu_online_mask & cpu_present_mask.
>  */
> void cpu_maps_update_begin(void)
> ..."
> 
> > +	cpu_hotplug_offline_disabled = true;
> 
> but this doesn't do that here.
> 
> Are we doing a one-off here for that variable or what?

Yes, it is one-off. I guess we could use READ_ONCE()/WRITE_ONCE() to
access the variable with the same result. I am not sure why it would be
better.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 03/18] cpu/hotplug: Add support for declaring CPU offlining not supported

[Borislav Petkov]

On Fri, Apr 19, 2024 at 04:31:39PM +0300, Kirill A. Shutemov wrote:
> Yes, it is one-off. I guess we could use READ_ONCE()/WRITE_ONCE() to
> access the variable with the same result. I am not sure why it would be
> better.

Nah, and it is not even the first one-off:

cpu_hotplug_disable/_enable() uses the same locking to update
cpu_hotplug_disabled.

I guess we need to update the comment over cpu_maps_update_begin().

I guess this is fine wrt big picture of the CPU hotplug universe. Lemme
point tglx to it just in case.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 01/18] x86/acpi: Extract ACPI MADT wakeup code into a separate file

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:29:53PM +0300, Kirill A. Shutemov wrote:
> diff --git a/arch/x86/kernel/acpi/Makefile b/arch/x86/kernel/acpi/Makefile
> index fc17b3f136fe..8c7329c88a75 100644
> --- a/arch/x86/kernel/acpi/Makefile
> +++ b/arch/x86/kernel/acpi/Makefile
> @@ -1,11 +1,12 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -obj-$(CONFIG_ACPI)		+= boot.o
> -obj-$(CONFIG_ACPI_SLEEP)	+= sleep.o wakeup_$(BITS).o
> -obj-$(CONFIG_ACPI_APEI)		+= apei.o
> -obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc.o
> +obj-$(CONFIG_ACPI)			+= boot.o
> +obj-$(CONFIG_ACPI_SLEEP)		+= sleep.o wakeup_$(BITS).o
> +obj-$(CONFIG_ACPI_APEI)			+= apei.o
> +obj-$(CONFIG_ACPI_CPPC_LIB)		+= cppc.o
> +obj-$(CONFIG_X86_ACPI_MADT_WAKEUP)	+= madt_wakeup.o

If you drop the "_X86" from the config symbol, you won't have to
re-align them. And the other config symbols don't have to have "_X86" in
them either because this is all in arch/x86/.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 01/18] x86/acpi: Extract ACPI MADT wakeup code into a separate file

[Kirill A. Shutemov]

On Thu, Apr 18, 2024 at 06:03:24PM +0200, Borislav Petkov wrote:
> On Tue, Apr 09, 2024 at 02:29:53PM +0300, Kirill A. Shutemov wrote:
> > diff --git a/arch/x86/kernel/acpi/Makefile b/arch/x86/kernel/acpi/Makefile
> > index fc17b3f136fe..8c7329c88a75 100644
> > --- a/arch/x86/kernel/acpi/Makefile
> > +++ b/arch/x86/kernel/acpi/Makefile
> > @@ -1,11 +1,12 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> > -obj-$(CONFIG_ACPI)		+= boot.o
> > -obj-$(CONFIG_ACPI_SLEEP)	+= sleep.o wakeup_$(BITS).o
> > -obj-$(CONFIG_ACPI_APEI)		+= apei.o
> > -obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc.o
> > +obj-$(CONFIG_ACPI)			+= boot.o
> > +obj-$(CONFIG_ACPI_SLEEP)		+= sleep.o wakeup_$(BITS).o
> > +obj-$(CONFIG_ACPI_APEI)			+= apei.o
> > +obj-$(CONFIG_ACPI_CPPC_LIB)		+= cppc.o
> > +obj-$(CONFIG_X86_ACPI_MADT_WAKEUP)	+= madt_wakeup.o
> 
> If you drop the "_X86" from the config symbol, you won't have to
> re-align them. And the other config symbols don't have to have "_X86" in
> them either because this is all in arch/x86/.

Okay, fair enough. Updated patch is below.

From b020800f89ea4fce8f3698bd4ef290bba8f40b37 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Fri, 1 Sep 2023 15:42:55 +0300
Subject: [PATCHv10.1 01/18] x86/acpi: Extract ACPI MADT wakeup code into a separate file

In order to prepare for the expansion of support for the ACPI MADT
wakeup method, move the relevant code into a separate file.

Introduce a new configuration option to clearly indicate dependencies
without the use of ifdefs.

There have been no functional changes.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Baoquan He <bhe@redhat.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Tao Liu <ltao@redhat.com>
---
 arch/x86/Kconfig                   |  7 +++
 arch/x86/include/asm/acpi.h        |  5 ++
 arch/x86/kernel/acpi/Makefile      |  1 +
 arch/x86/kernel/acpi/boot.c        | 86 +-----------------------------
 arch/x86/kernel/acpi/madt_wakeup.c | 82 ++++++++++++++++++++++++++++
 5 files changed, 96 insertions(+), 85 deletions(-)
 create mode 100644 arch/x86/kernel/acpi/madt_wakeup.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2dac256b6e8d..723cd5285781 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1116,6 +1116,13 @@ config X86_LOCAL_APIC
 	depends on X86_64 || SMP || X86_32_NON_STANDARD || X86_UP_APIC || PCI_MSI
 	select IRQ_DOMAIN_HIERARCHY
 
+config ACPI_MADT_WAKEUP
+	def_bool y
+	depends on X86_64
+	depends on ACPI
+	depends on SMP
+	depends on X86_LOCAL_APIC
+
 config X86_IO_APIC
 	def_bool y
 	depends on X86_LOCAL_APIC || X86_UP_IOAPIC
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index f896eed4516c..2625b915ae7f 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -76,6 +76,11 @@ static inline bool acpi_skip_set_wakeup_address(void)
 
 #define acpi_skip_set_wakeup_address acpi_skip_set_wakeup_address
 
+union acpi_subtable_headers;
+
+int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
+			      const unsigned long end);
+
 /*
  * Check if the CPU can handle C2 and deeper
  */
diff --git a/arch/x86/kernel/acpi/Makefile b/arch/x86/kernel/acpi/Makefile
index fc17b3f136fe..2feba7257665 100644
--- a/arch/x86/kernel/acpi/Makefile
+++ b/arch/x86/kernel/acpi/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_ACPI)		+= boot.o
 obj-$(CONFIG_ACPI_SLEEP)	+= sleep.o wakeup_$(BITS).o
 obj-$(CONFIG_ACPI_APEI)		+= apei.o
 obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc.o
+obj-$(CONFIG_ACPI_MADT_WAKEUP)	+= madt_wakeup.o
 
 ifneq ($(CONFIG_ACPI_PROCESSOR),)
 obj-y				+= cstate.o
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 4bf82dbd2a6b..9f4618dcd704 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -67,13 +67,6 @@ static bool has_lapic_cpus __initdata;
 static bool acpi_support_online_capable;
 #endif
 
-#ifdef CONFIG_X86_64
-/* Physical address of the Multiprocessor Wakeup Structure mailbox */
-static u64 acpi_mp_wake_mailbox_paddr;
-/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
-static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
-#endif
-
 #ifdef CONFIG_X86_IO_APIC
 /*
  * Locks related to IOAPIC hotplug
@@ -341,60 +334,6 @@ acpi_parse_lapic_nmi(union acpi_subtable_headers * header, const unsigned long e
 
 	return 0;
 }
-
-#ifdef CONFIG_X86_64
-static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
-{
-	/*
-	 * Remap mailbox memory only for the first call to acpi_wakeup_cpu().
-	 *
-	 * Wakeup of secondary CPUs is fully serialized in the core code.
-	 * No need to protect acpi_mp_wake_mailbox from concurrent accesses.
-	 */
-	if (!acpi_mp_wake_mailbox) {
-		acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
-						sizeof(*acpi_mp_wake_mailbox),
-						MEMREMAP_WB);
-	}
-
-	/*
-	 * Mailbox memory is shared between the firmware and OS. Firmware will
-	 * listen on mailbox command address, and once it receives the wakeup
-	 * command, the CPU associated with the given apicid will be booted.
-	 *
-	 * The value of 'apic_id' and 'wakeup_vector' must be visible to the
-	 * firmware before the wakeup command is visible.  smp_store_release()
-	 * ensures ordering and visibility.
-	 */
-	acpi_mp_wake_mailbox->apic_id	    = apicid;
-	acpi_mp_wake_mailbox->wakeup_vector = start_ip;
-	smp_store_release(&acpi_mp_wake_mailbox->command,
-			  ACPI_MP_WAKE_COMMAND_WAKEUP);
-
-	/*
-	 * Wait for the CPU to wake up.
-	 *
-	 * The CPU being woken up is essentially in a spin loop waiting to be
-	 * woken up. It should not take long for it wake up and acknowledge by
-	 * zeroing out ->command.
-	 *
-	 * ACPI specification doesn't provide any guidance on how long kernel
-	 * has to wait for a wake up acknowledgement. It also doesn't provide
-	 * a way to cancel a wake up request if it takes too long.
-	 *
-	 * In TDX environment, the VMM has control over how long it takes to
-	 * wake up secondary. It can postpone scheduling secondary vCPU
-	 * indefinitely. Giving up on wake up request and reporting error opens
-	 * possible attack vector for VMM: it can wake up a secondary CPU when
-	 * kernel doesn't expect it. Wait until positive result of the wake up
-	 * request.
-	 */
-	while (READ_ONCE(acpi_mp_wake_mailbox->command))
-		cpu_relax();
-
-	return 0;
-}
-#endif /* CONFIG_X86_64 */
 #endif /* CONFIG_X86_LOCAL_APIC */
 
 #ifdef CONFIG_X86_IO_APIC
@@ -1124,29 +1063,6 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	}
 	return 0;
 }
-
-#ifdef CONFIG_X86_64
-static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
-				     const unsigned long end)
-{
-	struct acpi_madt_multiproc_wakeup *mp_wake;
-
-	if (!IS_ENABLED(CONFIG_SMP))
-		return -ENODEV;
-
-	mp_wake = (struct acpi_madt_multiproc_wakeup *)header;
-	if (BAD_MADT_ENTRY(mp_wake, end))
-		return -EINVAL;
-
-	acpi_table_print_madt_entry(&header->common);
-
-	acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
-
-	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
-
-	return 0;
-}
-#endif				/* CONFIG_X86_64 */
 #endif				/* CONFIG_X86_LOCAL_APIC */
 
 #ifdef	CONFIG_X86_IO_APIC
@@ -1343,7 +1259,7 @@ static void __init acpi_process_madt(void)
 				smp_found_config = 1;
 			}
 
-#ifdef CONFIG_X86_64
+#ifdef CONFIG_ACPI_MADT_WAKEUP
 			/*
 			 * Parse MADT MP Wake entry.
 			 */
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
new file mode 100644
index 000000000000..7f164d38bd0b
--- /dev/null
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <asm/apic.h>
+#include <asm/barrier.h>
+#include <asm/processor.h>
+
+/* Physical address of the Multiprocessor Wakeup Structure mailbox */
+static u64 acpi_mp_wake_mailbox_paddr;
+
+/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
+static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
+
+static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
+{
+	/*
+	 * Remap mailbox memory only for the first call to acpi_wakeup_cpu().
+	 *
+	 * Wakeup of secondary CPUs is fully serialized in the core code.
+	 * No need to protect acpi_mp_wake_mailbox from concurrent accesses.
+	 */
+	if (!acpi_mp_wake_mailbox) {
+		acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
+						sizeof(*acpi_mp_wake_mailbox),
+						MEMREMAP_WB);
+	}
+
+	/*
+	 * Mailbox memory is shared between the firmware and OS. Firmware will
+	 * listen on mailbox command address, and once it receives the wakeup
+	 * command, the CPU associated with the given apicid will be booted.
+	 *
+	 * The value of 'apic_id' and 'wakeup_vector' must be visible to the
+	 * firmware before the wakeup command is visible.  smp_store_release()
+	 * ensures ordering and visibility.
+	 */
+	acpi_mp_wake_mailbox->apic_id	    = apicid;
+	acpi_mp_wake_mailbox->wakeup_vector = start_ip;
+	smp_store_release(&acpi_mp_wake_mailbox->command,
+			  ACPI_MP_WAKE_COMMAND_WAKEUP);
+
+	/*
+	 * Wait for the CPU to wake up.
+	 *
+	 * The CPU being woken up is essentially in a spin loop waiting to be
+	 * woken up. It should not take long for it wake up and acknowledge by
+	 * zeroing out ->command.
+	 *
+	 * ACPI specification doesn't provide any guidance on how long kernel
+	 * has to wait for a wake up acknowledgment. It also doesn't provide
+	 * a way to cancel a wake up request if it takes too long.
+	 *
+	 * In TDX environment, the VMM has control over how long it takes to
+	 * wake up secondary. It can postpone scheduling secondary vCPU
+	 * indefinitely. Giving up on wake up request and reporting error opens
+	 * possible attack vector for VMM: it can wake up a secondary CPU when
+	 * kernel doesn't expect it. Wait until positive result of the wake up
+	 * request.
+	 */
+	while (READ_ONCE(acpi_mp_wake_mailbox->command))
+		cpu_relax();
+
+	return 0;
+}
+
+int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
+			      const unsigned long end)
+{
+	struct acpi_madt_multiproc_wakeup *mp_wake;
+
+	mp_wake = (struct acpi_madt_multiproc_wakeup *)header;
+	if (BAD_MADT_ENTRY(mp_wake, end))
+		return -EINVAL;
+
+	acpi_table_print_madt_entry(&header->common);
+
+	acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
+
+	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
+
+	return 0;
+}
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:29:56PM +0300, Kirill A. Shutemov wrote:
> ACPI MADT doesn't allow to offline CPU after it got woke up.

In all your text: s/woke/woken/g

> 
> Currently CPU hotplug is prevented based on the confidential computing
> attribute which is set for Intel TDX. But TDX is not the only possible
> user of the wake up method.
> 
> Disable CPU offlining on ACPI MADT wakeup enumeration.

Something's missing in that "justification". It should explain why
CC_ATTR_HOTPLUG_DISABLED is not needed anymore.

And looking at patch 3, I'm still unclear as to why this change is done.
Is it that the "ACPI MADT mailbox wakeup method" is going to be used by
TDX guests now too so that you don't need CC_ATTR_HOTPLUG_DISABLED
anymore?

It seems that if acpi_parse_mp_wake() finds an ok wakeup entry, then
offlining is disabled...

Or is it something else?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup

[Kirill A. Shutemov]

On Tue, Apr 23, 2024 at 06:02:58PM +0200, Borislav Petkov wrote:
> > 
> > Currently CPU hotplug is prevented based on the confidential computing
> > attribute which is set for Intel TDX. But TDX is not the only possible
> > user of the wake up method.
> > 
> > Disable CPU offlining on ACPI MADT wakeup enumeration.
> 
> Something's missing in that "justification". It should explain why
> CC_ATTR_HOTPLUG_DISABLED is not needed anymore.

It was wrong from beginning. If ACPI MADT wake up method is used on the
platform, we cannot handle offline, regardless if it is TDX or not.

> And looking at patch 3, I'm still unclear as to why this change is done.
> Is it that the "ACPI MADT mailbox wakeup method" is going to be used by
> TDX guests now too so that you don't need CC_ATTR_HOTPLUG_DISABLED
> anymore?

ACPI MADT is the only wakeup method supported in TDX guests. But offline
is broken is because of ACPI MADT, not because of TDX.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup

[Borislav Petkov]

On Wed, Apr 24, 2024 at 11:38:42AM +0300, Kirill A. Shutemov wrote:
> It was wrong from beginning. If ACPI MADT wake up method is used on the
> platform, we cannot handle offline, regardless if it is TDX or not.

Sounds to me like this fact should be a prominent part of the commit
message and these 1-4 patches should be carved out as a separate set
fixing that ACPI MADT thing and I should take them separately now...?

Also, does this need to go to stable although it is kinda big for
stable. If stable, do we need a smaller fix first which is backportable?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup

[Kirill A. Shutemov]

On Wed, Apr 24, 2024 at 03:50:52PM +0200, Borislav Petkov wrote:
> On Wed, Apr 24, 2024 at 11:38:42AM +0300, Kirill A. Shutemov wrote:
> > It was wrong from beginning. If ACPI MADT wake up method is used on the
> > platform, we cannot handle offline, regardless if it is TDX or not.
> 
> Sounds to me like this fact should be a prominent part of the commit
> message and these 1-4 patches should be carved out as a separate set
> fixing that ACPI MADT thing and I should take them separately now...?
> 
> Also, does this need to go to stable although it is kinda big for
> stable. If stable, do we need a smaller fix first which is backportable?

Correct me, if I am wrong, but I believe TDX guest is the only user of
ACPI MADT wake up method. At least it was added into kernel for TDX guest.
So it wouldn't fix anything user-visible. It might affect a future
platform that uses this wake up method, but it is a guessing game.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup

[Dave Hansen]

On 4/24/24 07:35, Kirill A. Shutemov wrote:
>> Also, does this need to go to stable although it is kinda big for
>> stable. If stable, do we need a smaller fix first which is backportable?
> Correct me, if I am wrong, but I believe TDX guest is the only user of
> ACPI MADT wake up method. At least it was added into kernel for TDX guest.
> So it wouldn't fix anything user-visible. It might affect a future
> platform that uses this wake up method, but it is a guessing game.

Yeah, the MADT wakeup is pretty funky, highly TDX specific, and not in
use or planned to be _put_ into use anywhere else.  The X86S "fix" for
running in real mode does something else entirely:

	https://cdrdv2.intel.com/v1/dl/getContent/776648

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 04/18] cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup

[Borislav Petkov]

On Wed, Apr 24, 2024 at 07:40:26AM -0700, Dave Hansen wrote:
> On 4/24/24 07:35, Kirill A. Shutemov wrote:
> >> Also, does this need to go to stable although it is kinda big for
> >> stable. If stable, do we need a smaller fix first which is backportable?
> > Correct me, if I am wrong, but I believe TDX guest is the only user of
> > ACPI MADT wake up method. At least it was added into kernel for TDX guest.
> > So it wouldn't fix anything user-visible. It might affect a future
> > platform that uses this wake up method, but it is a guessing game.
> 
> Yeah, the MADT wakeup is pretty funky, highly TDX specific, and not in
> use or planned to be _put_ into use anywhere else.

Then please make sure all that info is in the commit messages in the
next revision. Because as they are now, they're not even beginning to
explain what exactly is this fixing. /me thinking that this is some
generic fix needed in stable is case-in-point.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 09/18] x86/mm: Adding callbacks to prepare encrypted memory for kexec

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:30:01PM +0300, Kirill A. Shutemov wrote:
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index e74d0c4286c1..7a1560d7e62d 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -128,6 +128,12 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
>  #ifdef CONFIG_HPET_TIMER
>  	hpet_disable();
>  #endif
> +
> +	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
> +		x86_platform.guest.enc_kexec_stop_conversion(true);
> +		x86_platform.guest.enc_kexec_unshare_mem();
> +	}

This is not how this is done - the point of those function pointers is
to avoid random checks in the code but simply unconditionally call them.

The platform which needs something special to happen, assigns to them
its own function pointers and the rest assigns dummy stubs.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted memory for kexec

[Borislav Petkov]

On Sat, Apr 27, 2024 at 08:06:34PM +0300, Kirill A. Shutemov wrote:

> Subject: Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted memory for kexec

s/Adding/Add/

> AMD SEV and Intel TDX guests allocate shared buffers for performing I/O.
> This is done by allocating pages normally from the buddy allocator and
> then converting them to shared using set_memory_decrypted().
> 
> On kexec, the second kernel is unaware of which memory has been
> converted in this manner. It only sees E820_TYPE_RAM. Accessing shared
> memory as private is fatal.
> 
> Therefore, the memory state must be reset to its original state before
> starting the new kernel with kexec.
> 
> The process of converting shared memory back to private occurs in two
> steps:
> 
> - enc_kexec_stop_conversion() stops new conversions.
> 
> - enc_kexec_unshare_mem() unshares all existing shared memory, reverting
>   it back to private.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>x
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Tested-by: Tao Liu <ltao@redhat.com>
> ---
>  arch/x86/include/asm/x86_init.h |  2 ++
>  arch/x86/kernel/crash.c         |  4 ++++
>  arch/x86/kernel/reboot.c        | 12 ++++++++++++
>  arch/x86/kernel/x86_init.c      |  4 ++++
>  4 files changed, 22 insertions(+)
> 
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 28ac3cb9b987..c731e6bc4343 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -155,6 +155,8 @@ struct x86_guest {
>  	int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
>  	bool (*enc_tlb_flush_required)(bool enc);
>  	bool (*enc_cache_flush_required)(void);
> +	void (*enc_kexec_stop_conversion)(bool crash);
> +	void (*enc_kexec_unshare_mem)(void);

This is all fine and dandy but those functions need documentation in the
kernel doc above it.

And if there's a "stop_conversion" function, then I'd expect there to be
a "start conversion" one.

I think you need to give those two better names to denote that they're
related, the order in which they should be called and why they're
separate.

>  };
>  
>  /**
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index e74d0c4286c1..f1b261be78b4 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -128,6 +128,10 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
>  #ifdef CONFIG_HPET_TIMER
>  	hpet_disable();
>  #endif
> +
> +	x86_platform.guest.enc_kexec_stop_conversion(true);
> +	x86_platform.guest.enc_kexec_unshare_mem();
> +

You call them here back-to-back...

>  	crash_save_cpu(regs, safe_smp_processor_id());
>  }
>  
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index f3130f762784..c1920ec34f0c 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -12,6 +12,7 @@
>  #include <linux/delay.h>
>  #include <linux/objtool.h>
>  #include <linux/pgtable.h>
> +#include <linux/kexec.h>
>  #include <acpi/reboot.h>
>  #include <asm/io.h>
>  #include <asm/apic.h>
> @@ -716,6 +717,14 @@ static void native_machine_emergency_restart(void)
>  
>  void native_machine_shutdown(void)
>  {
> +	/*
> +	 * Call enc_kexec_stop_conversion() while all CPUs are still active and
> +	 * interrupts are enabled. This will allow all in-flight memory
> +	 * conversions to finish cleanly.
> +	 */
> +	if (kexec_in_progress)
> +		x86_platform.guest.enc_kexec_stop_conversion(false);
> +
>  	/* Stop the cpus and apics */
>  #ifdef CONFIG_X86_IO_APIC
>  	/*
> @@ -752,6 +761,9 @@ void native_machine_shutdown(void)
>  #ifdef CONFIG_X86_64
>  	x86_platform.iommu_shutdown();
>  #endif
> +
> +	if (kexec_in_progress)
> +		x86_platform.guest.enc_kexec_unshare_mem();

... but they're split here.

And I don't know why and nothing tells me...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted memory for kexec

[Kirill A. Shutemov]

On Thu, May 02, 2024 at 03:45:06PM +0200, Borislav Petkov wrote:
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index e74d0c4286c1..f1b261be78b4 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -128,6 +128,10 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
> >  #ifdef CONFIG_HPET_TIMER
> >  	hpet_disable();
> >  #endif
> > +
> > +	x86_platform.guest.enc_kexec_stop_conversion(true);
> > +	x86_platform.guest.enc_kexec_unshare_mem();
> > +
> 
> You call them here back-to-back...
> 
> >  	crash_save_cpu(regs, safe_smp_processor_id());
> >  }
> >  
> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > index f3130f762784..c1920ec34f0c 100644
> > --- a/arch/x86/kernel/reboot.c
> > +++ b/arch/x86/kernel/reboot.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/objtool.h>
> >  #include <linux/pgtable.h>
> > +#include <linux/kexec.h>
> >  #include <acpi/reboot.h>
> >  #include <asm/io.h>
> >  #include <asm/apic.h>
> > @@ -716,6 +717,14 @@ static void native_machine_emergency_restart(void)
> >  
> >  void native_machine_shutdown(void)
> >  {
> > +	/*
> > +	 * Call enc_kexec_stop_conversion() while all CPUs are still active and
> > +	 * interrupts are enabled. This will allow all in-flight memory
> > +	 * conversions to finish cleanly.
> > +	 */
> > +	if (kexec_in_progress)
> > +		x86_platform.guest.enc_kexec_stop_conversion(false);
> > +
> >  	/* Stop the cpus and apics */
> >  #ifdef CONFIG_X86_IO_APIC
> >  	/*
> > @@ -752,6 +761,9 @@ void native_machine_shutdown(void)
> >  #ifdef CONFIG_X86_64
> >  	x86_platform.iommu_shutdown();
> >  #endif
> > +
> > +	if (kexec_in_progress)
> > +		x86_platform.guest.enc_kexec_unshare_mem();
> 
> ... but they're split here.
> 
> And I don't know why and nothing tells me...

I do. See comment just above enc_kexec_stop_conversion() call.

Do you want also comment for enc_kexec_unshare_mem() ?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted memory for kexec

[Borislav Petkov]

On Mon, May 06, 2024 at 04:22:02PM +0300, Kirill A. Shutemov wrote:
> I do. See comment just above enc_kexec_stop_conversion() call.

If you mean this:

        /*
         * Call enc_kexec_stop_conversion() while all CPUs are still active and
         * interrupts are enabled. This will allow all in-flight memory
         * conversions to finish cleanly.
         */
        if (kexec_in_progress)
                x86_platform.guest.enc_kexec_stop_conversion(false);

then no, this is not enough.

I mean this:

/**
 * struct x86_guest - Functions used by misc guest incarnations like SEV, TDX, etc.
 *
 * @enc_status_change_prepare   Notify HV before the encryption status of a range is changed
 * @enc_status_change_finish    Notify HV after the encryption status of a range is changed
 * @enc_tlb_flush_required      Returns true if a TLB flush is needed before changing page encryption status
 * @enc_cache_flush_required    Returns true if a cache flush is needed before changing page encryption status
 * @enc_kexec_begin		Begin the two-step process of stopping
 * 				page conversion... <insert reason why it
 * 				needs to happen this way, blabla>
 * @enc_kexec_finish		...
 */
struct x86_guest {
        int (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc);
        int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
        bool (*enc_tlb_flush_required)(bool enc);
        bool (*enc_cache_flush_required)(void);
        void (*enc_kexec_begin)(bool crash);
        void (*enc_kexec_finish)(void);

And calling them a _begin and _finish makes a lot more sense to me:
_begin starts the kexec process for encrypted guests and _finish
finishes it.

Just from the names you now know what needs to happen and in which
order.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 06/18] x86/mm: Make x86_platform.guest.enc_status_change_*() return errno

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:29:58PM +0300, Kirill A. Shutemov wrote:
> TDX is going to have more than one reason to fail
> enc_status_change_prepare().
> 
> Change the callback to return errno instead of assuming -EIO;
> enc_status_change_finish() changed too to keep the interface symmetric.

"Change enc_status_change_finish() too... "

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."

You should know this by now...

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Tested-by: Tao Liu <ltao@redhat.com>
> ---
>  arch/x86/coco/tdx/tdx.c         | 20 +++++++++++---------
>  arch/x86/hyperv/ivm.c           | 22 ++++++++++------------
>  arch/x86/include/asm/x86_init.h |  4 ++--
>  arch/x86/kernel/x86_init.c      |  4 ++--
>  arch/x86/mm/mem_encrypt_amd.c   |  8 ++++----
>  arch/x86/mm/pat/set_memory.c    |  8 +++++---
>  6 files changed, 34 insertions(+), 32 deletions(-)

Another thing you should long know by now: get_maintainer.pl. You do
know that when you send a patch which touches multiple different
"places", you run it through get_maintainer.pl to get some hints as to
who to CC, right?

Because you're touching HyperV code and yet none of the HyperV folks are
CCed.

Do I need to give you the spiel I give to kernel newbies? :)

Lemme Cc them for you now.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 06/18] x86/mm: Make x86_platform.guest.enc_status_change_*() return errno

[Kirill A. Shutemov]

On Sun, Apr 28, 2024 at 07:25:57PM +0200, Borislav Petkov wrote:
> On Tue, Apr 09, 2024 at 02:29:58PM +0300, Kirill A. Shutemov wrote:
> > TDX is going to have more than one reason to fail
> > enc_status_change_prepare().
> > 
> > Change the callback to return errno instead of assuming -EIO;
> > enc_status_change_finish() changed too to keep the interface symmetric.
> 
> "Change enc_status_change_finish() too... "
> 
> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour."

Hm. I considered the sentence to be in imperative mood already. I guess I
don't fully understand what imperative mood is. Will fix.

> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Reviewed-by: Dave Hansen <dave.hansen@intel.com>
> > Reviewed-by: Kai Huang <kai.huang@intel.com>
> > Tested-by: Tao Liu <ltao@redhat.com>
> > ---
> >  arch/x86/coco/tdx/tdx.c         | 20 +++++++++++---------
> >  arch/x86/hyperv/ivm.c           | 22 ++++++++++------------
> >  arch/x86/include/asm/x86_init.h |  4 ++--
> >  arch/x86/kernel/x86_init.c      |  4 ++--
> >  arch/x86/mm/mem_encrypt_amd.c   |  8 ++++----
> >  arch/x86/mm/pat/set_memory.c    |  8 +++++---
> >  6 files changed, 34 insertions(+), 32 deletions(-)
> 
> Another thing you should long know by now: get_maintainer.pl. You do
> know that when you send a patch which touches multiple different
> "places", you run it through get_maintainer.pl to get some hints as to
> who to CC, right?

You are right, I didn't run get_maintainer.pl this time. I never got it
integrated properly into my workflow. How do you use it? Is it part of
'git send-email' hooks or do you do it manually somehow.

I don't feel I can trust the script to do The Right Thing™ all the time
to put into my hooks. I expect it to blow up on tree-wide patches for
instance.

As result I only run it occasionally, when I remember to which is
suboptimal.

Any tips?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 06/18] x86/mm: Make x86_platform.guest.enc_status_change_*() return errno

[Borislav Petkov]

On Mon, Apr 29, 2024 at 05:29:23PM +0300, Kirill A. Shutemov wrote:
> Hm. I considered the sentence to be in imperative mood already. I guess I
> don't fully understand what imperative mood is. Will fix.

This:

https://en.wikipedia.org/wiki/Imperative_mood

but basically the sentence is a command.

> You are right, I didn't run get_maintainer.pl this time. I never got it
> integrated properly into my workflow. How do you use it? Is it part of
> 'git send-email' hooks or do you do it manually somehow.

So what I do after the whole set is applied, is:

git diff HEAD~<NUM>.. | ./scripts/get_maintainer.pl

where <NUM> is the number of patches which belong to the series.

IOW, you get a full diff of the set and you run that diff through
get_maintainer.pl.

It'll give you a whole lot of people but you can go through that list
and prune it to the people who are really relevant for the set.

And then you do

git send-email --cc-cmd=cccmd.sh ...

and that script simply echoes a "Cc: <name>" one per line. That is, if
there are a lot of people to Cc. If there are only 1-3ish or so, you can
supply each with the "--cc" option to git-send-email.

Anyway, this is what I do. Someone has probably a lot better flow tho.

> I don't feel I can trust the script to do The Right Thing™ all the time
> to put into my hooks. I expect it to blow up on tree-wide patches for
> instance.

Yeah, not even going there. :-)

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCHv10 06/18] x86/mm: Make x86_platform.guest.enc_status_change_*() return errno

[Michael Kelley]

From: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Sent: Tuesday, April 9, 2024 4:30 AM
> 
> TDX is going to have more than one reason to fail
> enc_status_change_prepare().
> 
> Change the callback to return errno instead of assuming -EIO;
> enc_status_change_finish() changed too to keep the interface symmetric.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Tested-by: Tao Liu <ltao@redhat.com>
> ---
>  arch/x86/coco/tdx/tdx.c         | 20 +++++++++++---------
>  arch/x86/hyperv/ivm.c           | 22 ++++++++++------------
>  arch/x86/include/asm/x86_init.h |  4 ++--
>  arch/x86/kernel/x86_init.c      |  4 ++--
>  arch/x86/mm/mem_encrypt_amd.c   |  8 ++++----
>  arch/x86/mm/pat/set_memory.c    |  8 +++++---
>  6 files changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index c1cb90369915..26fa47db5782 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -798,28 +798,30 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>  	return true;
>  }
> 
> -static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> -					  bool enc)
> +static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> +					 bool enc)
>  {
>  	/*
>  	 * Only handle shared->private conversion here.
>  	 * See the comment in tdx_early_init().
>  	 */
> -	if (enc)
> -		return tdx_enc_status_changed(vaddr, numpages, enc);
> -	return true;
> +	if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> +		return -EIO;
> +
> +	return 0;
>  }
> 
> -static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> +static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
>  					 bool enc)
>  {
>  	/*
>  	 * Only handle private->shared conversion here.
>  	 * See the comment in tdx_early_init().
>  	 */
> -	if (!enc)
> -		return tdx_enc_status_changed(vaddr, numpages, enc);
> -	return true;
> +	if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> +		return -EIO;
> +
> +	return 0;
>  }
> 
>  void __init tdx_early_init(void)
> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> index 768d73de0d09..b4a851d27c7c 100644
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -523,9 +523,9 @@ static int hv_mark_gpa_visibility(u16 count, const u64 pfn[],
>   * transition is complete, hv_vtom_set_host_visibility() marks the pages
>   * as "present" again.
>   */
> -static bool hv_vtom_clear_present(unsigned long kbuffer, int pagecount, bool enc)
> +static int hv_vtom_clear_present(unsigned long kbuffer, int pagecount, bool enc)
>  {
> -	return !set_memory_np(kbuffer, pagecount);
> +	return set_memory_np(kbuffer, pagecount);
>  }
> 
>  /*
> @@ -536,20 +536,19 @@ static bool hv_vtom_clear_present(unsigned long kbuffer, int pagecount, bool enc
>   * with host. This function works as wrap of hv_mark_gpa_visibility()
>   * with memory base and size.
>   */
> -static bool hv_vtom_set_host_visibility(unsigned long kbuffer, int pagecount, bool enc)
> +static int hv_vtom_set_host_visibility(unsigned long kbuffer, int pagecount, bool enc)
>  {
>  	enum hv_mem_host_visibility visibility = enc ?
>  			VMBUS_PAGE_NOT_VISIBLE : VMBUS_PAGE_VISIBLE_READ_WRITE;
>  	u64 *pfn_array;
>  	phys_addr_t paddr;
> +	int i, pfn, err;
>  	void *vaddr;
>  	int ret = 0;
> -	bool result = true;
> -	int i, pfn;
> 
>  	pfn_array = kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
>  	if (!pfn_array) {
> -		result = false;
> +		ret = -ENOMEM;
>  		goto err_set_memory_p;
>  	}
> 
> @@ -568,10 +567,8 @@ static bool hv_vtom_set_host_visibility(unsigned long kbuffer, int pagecount, bo
>  		if (pfn == HV_MAX_MODIFY_GPA_REP_COUNT || i == pagecount - 1) {
>  			ret = hv_mark_gpa_visibility(pfn, pfn_array,
>  						     visibility);
> -			if (ret) {
> -				result = false;
> +			if (ret)
>  				goto err_free_pfn_array;
> -			}
>  			pfn = 0;
>  		}
>  	}
> @@ -586,10 +583,11 @@ static bool hv_vtom_set_host_visibility(unsigned long kbuffer, int pagecount, bo
>  	 * order to avoid leaving the memory range in a "broken" state. Setting
>  	 * the PRESENT bits shouldn't fail, but return an error if it does.
>  	 */
> -	if (set_memory_p(kbuffer, pagecount))
> -		result = false;
> +	err = set_memory_p(kbuffer, pagecount);
> +	if (err && !ret)
> +		ret = err;
> 
> -	return result;
> +	return ret;
>  }
> 
>  static bool hv_vtom_tlb_flush_required(bool private)
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 6149eabe200f..28ac3cb9b987 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -151,8 +151,8 @@ struct x86_init_acpi {
>   * @enc_cache_flush_required	Returns true if a cache flush is needed before changing page encryption status
>   */
>  struct x86_guest {
> -	bool (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc);
> -	bool (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
> +	int (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc);
> +	int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
>  	bool (*enc_tlb_flush_required)(bool enc);
>  	bool (*enc_cache_flush_required)(void);
>  };
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index d5dc5a92635a..a7143bb7dd93 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -134,8 +134,8 @@ struct x86_cpuinit_ops x86_cpuinit = {
> 
>  static void default_nmi_init(void) { };
> 
> -static bool enc_status_change_prepare_noop(unsigned long vaddr, int npages, bool enc) { return true; }
> -static bool enc_status_change_finish_noop(unsigned long vaddr, int npages, bool enc) { return true; }
> +static int enc_status_change_prepare_noop(unsigned long vaddr, int npages, bool enc) { return 0; }
> +static int enc_status_change_finish_noop(unsigned long vaddr, int npages, bool enc) { return 0; }
>  static bool enc_tlb_flush_required_noop(bool enc) { return false; }
>  static bool enc_cache_flush_required_noop(void) { return false; }
>  static bool is_private_mmio_noop(u64 addr) {return false; }
> diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
> index 422602f6039b..e7b67519ddb5 100644
> --- a/arch/x86/mm/mem_encrypt_amd.c
> +++ b/arch/x86/mm/mem_encrypt_amd.c
> @@ -283,7 +283,7 @@ static void enc_dec_hypercall(unsigned long vaddr, unsigned long size, bool enc)
>  #endif
>  }
> 
> -static bool amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc)
> +static int amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc)
>  {
>  	/*
>  	 * To maintain the security guarantees of SEV-SNP guests, make sure
> @@ -292,11 +292,11 @@ static bool amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool
>  	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && !enc)
>  		snp_set_memory_shared(vaddr, npages);
> 
> -	return true;
> +	return 0;
>  }
> 
>  /* Return true unconditionally: return value doesn't matter for the SEV side */
> -static bool amd_enc_status_change_finish(unsigned long vaddr, int npages, bool enc)
> +static int amd_enc_status_change_finish(unsigned long vaddr, int npages, bool enc)
>  {
>  	/*
>  	 * After memory is mapped encrypted in the page table, validate it
> @@ -308,7 +308,7 @@ static bool amd_enc_status_change_finish(unsigned long vaddr, int npages, bool e
>  	if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
>  		enc_dec_hypercall(vaddr, npages << PAGE_SHIFT, enc);
> 
> -	return true;
> +	return 0;
>  }
> 
>  static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 80c9037ffadf..e5b454036bf3 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -2156,7 +2156,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
>  		cpa_flush(&cpa, x86_platform.guest.enc_cache_flush_required());
> 
>  	/* Notify hypervisor that we are about to set/clr encryption attribute. */
> -	if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc))
> +	ret = x86_platform.guest.enc_status_change_prepare(addr, numpages, enc);
> +	if (ret)
>  		goto vmm_fail;
> 
>  	ret = __change_page_attr_set_clr(&cpa, 1);
> @@ -2174,7 +2175,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
>  		return ret;
> 
>  	/* Notify hypervisor that we have successfully set/clr encryption attribute. */
> -	if (!x86_platform.guest.enc_status_change_finish(addr, numpages, enc))
> +	ret = x86_platform.guest.enc_status_change_finish(addr, numpages, enc);
> +	if (ret)
>  		goto vmm_fail;
> 
>  	return 0;
> @@ -2183,7 +2185,7 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
>  	WARN_ONCE(1, "CPA VMM failure to convert memory (addr=%p, numpages=%d) to %s.\n",
>  		  (void *)addr, numpages, enc ? "private" : "shared");

Nit: Now that there's an error code instead of just a boolean, it would be nice
to have this warning message include the error code.  Some of the callers of
set_memory_decrypted()/encrypted() also output a message on failure that
includes the error code, in which case this message will be redundant.  But
many callers do not.

> 
> -	return -EIO;
> +	return ret;
>  }
> 
>  static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> --
> 2.43.0
> 

My nit notwithstanding,

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 10/18] x86/tdx: Convert shared memory back to private on kexec

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:30:02PM +0300, Kirill A. Shutemov wrote:
> TDX guests allocate shared buffers to perform I/O. It is done by
> allocating pages normally from the buddy allocator and converting them
> to shared with set_memory_decrypted().
> 
> The second kernel has no idea what memory is converted this way. It only

"The kexec-ed kernel..."

is more precise.

> sees E820_TYPE_RAM.
> 
> Accessing shared memory via private mapping is fatal. It leads to
> unrecoverable TD exit.
> 
> On kexec walk direct mapping and convert all shared memory back to
> private. It makes all RAM private again and second kernel may use it
> normally.
> 
> The conversion occurs in two steps: stopping new conversions and
> unsharing all memory. In the case of normal kexec, the stopping of
> conversions takes place while scheduling is still functioning. This
> allows for waiting until any ongoing conversions are finished. The
> second step is carried out when all CPUs except one are inactive and
> interrupts are disabled. This prevents any conflicts with code that may
> access shared memory.

This is the missing bit of information I was looking for in the previous
patch. This needs to be documented in the code.

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Tested-by: Tao Liu <ltao@redhat.com>
> ---
>  arch/x86/coco/tdx/tdx.c           | 72 +++++++++++++++++++++++++++++++
>  arch/x86/include/asm/pgtable.h    |  5 +++
>  arch/x86/include/asm/set_memory.h |  3 ++
>  arch/x86/mm/pat/set_memory.c      | 35 +++++++++++++--
>  4 files changed, 112 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 979891e97d83..59776ce1c1d7 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -7,6 +7,7 @@
>  #include <linux/cpufeature.h>
>  #include <linux/export.h>
>  #include <linux/io.h>
> +#include <linux/kexec.h>
>  #include <asm/coco.h>
>  #include <asm/tdx.h>
>  #include <asm/vmx.h>
> @@ -14,6 +15,7 @@
>  #include <asm/insn.h>
>  #include <asm/insn-eval.h>
>  #include <asm/pgtable.h>
> +#include <asm/set_memory.h>
>  
>  /* MMIO direction */
>  #define EPT_READ	0
> @@ -831,6 +833,73 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
>  	return 0;
>  }
>  
> +/* Stop new private<->shared conversions */
> +static void tdx_kexec_stop_conversion(bool crash)
> +{
> +	/*
> +	 * Crash kernel reaches here with interrupts disabled: can't wait for
> +	 * conversions to finish.
> +	 *
> +	 * If race happened, just report and proceed.
> +	 */
> +	bool wait_for_lock = !crash;

You don't need that bool - use crash.

> +
> +	if (!stop_memory_enc_conversion(wait_for_lock))
> +		pr_warn("Failed to stop shared<->private conversions\n");
> +}
> +
> +static void tdx_kexec_unshare_mem(void)
> +{
> +	unsigned long addr, end;
> +	long found = 0, shared;
> +
> +	/*
> +	 * Walk direct mapping and convert all shared memory back to private,
> +	 */

Over the function name and end with a fullstop.

> +
> +	addr = PAGE_OFFSET;
> +	end  = PAGE_OFFSET + get_max_mapped();
> +
> +	while (addr < end) {
> +		unsigned long size;
> +		unsigned int level;
> +		pte_t *pte;
> +
> +		pte = lookup_address(addr, &level);
> +		size = page_level_size(level);
> +
> +		if (pte && pte_decrypted(*pte)) {
> +			int pages = size / PAGE_SIZE;
> +
> +			/*
> +			 * Touching memory with shared bit set triggers implicit
> +			 * conversion to shared.
> +			 *
> +			 * Make sure nobody touches the shared range from
> +			 * now on.
> +			 */

lockdep_assert_irqs_disabled() ?

> +			set_pte(pte, __pte(0));
> +
> +			if (!tdx_enc_status_changed(addr, pages, true)) {
> +				pr_err("Failed to unshare range %#lx-%#lx\n",
> +				       addr, addr + size);

Why are we printing something here if we're not really acting up on it?

Who should care here?

> +			}
> +
> +			found += pages;
> +		}
> +
> +		addr += size;
> +	}
> +
> +	__flush_tlb_all();
> +
> +	shared = atomic_long_read(&nr_shared);
> +	if (shared != found) {
> +		pr_err("shared page accounting is off\n");
> +		pr_err("nr_shared = %ld, nr_found = %ld\n", shared, found);
> +	}

Ok, we failed unsharing. And yet we don't do anything.

But if we fail unsharing, we will die on a unrecoverable TD exit or
whatever.

Why aren't we failing kexec here?

> +}
> +
>  void __init tdx_early_init(void)
>  {
>  	struct tdx_module_args args = {
> @@ -890,6 +959,9 @@ void __init tdx_early_init(void)
>  	x86_platform.guest.enc_cache_flush_required  = tdx_cache_flush_required;
>  	x86_platform.guest.enc_tlb_flush_required    = tdx_tlb_flush_required;
>  
> +	x86_platform.guest.enc_kexec_stop_conversion = tdx_kexec_stop_conversion;
> +	x86_platform.guest.enc_kexec_unshare_mem     = tdx_kexec_unshare_mem;
> +
>  	/*
>  	 * TDX intercepts the RDMSR to read the X2APIC ID in the parallel
>  	 * bringup low level code. That raises #VE which cannot be handled
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 315535ffb258..17f4d97fae06 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -140,6 +140,11 @@ static inline int pte_young(pte_t pte)
>  	return pte_flags(pte) & _PAGE_ACCESSED;
>  }
>  
> +static inline bool pte_decrypted(pte_t pte)
> +{
> +	return cc_mkdec(pte_val(pte)) == pte_val(pte);
> +}
> +
>  #define pmd_dirty pmd_dirty
>  static inline bool pmd_dirty(pmd_t pmd)
>  {
> diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h
> index 9aee31862b4a..44b6d711296c 100644
> --- a/arch/x86/include/asm/set_memory.h
> +++ b/arch/x86/include/asm/set_memory.h
> @@ -49,8 +49,11 @@ int set_memory_wb(unsigned long addr, int numpages);
>  int set_memory_np(unsigned long addr, int numpages);
>  int set_memory_p(unsigned long addr, int numpages);
>  int set_memory_4k(unsigned long addr, int numpages);
> +
> +bool stop_memory_enc_conversion(bool wait);
>  int set_memory_encrypted(unsigned long addr, int numpages);
>  int set_memory_decrypted(unsigned long addr, int numpages);
> +
>  int set_memory_np_noalias(unsigned long addr, int numpages);
>  int set_memory_nonglobal(unsigned long addr, int numpages);
>  int set_memory_global(unsigned long addr, int numpages);
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 6c49f69c0368..21835339c0e6 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -2188,12 +2188,41 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
>  	return ret;
>  }
>  

<--- insert comment here what this thing is guarding.

> +static DECLARE_RWSEM(mem_enc_lock);
> +
> +/*
> + * Stop new private<->shared conversions.
> + *
> + * Taking the exclusive mem_enc_lock waits for in-flight conversions to complete.
> + * The lock is not released to prevent new conversions from being started.
> + *
> + * If sleep is not allowed, as in a crash scenario, try to take the lock.
> + * Failure indicates that there is a race with the conversion.
> + */
> +bool stop_memory_enc_conversion(bool wait)

This is a global function which means, it should be called:

set_memory_enc_stop_conversion()

or so. With the proper prefix and so on.

> +{
> +	if (!wait)
> +		return down_write_trylock(&mem_enc_lock);
> +
> +	down_write(&mem_enc_lock);
> +
> +	return true;
> +}
> +
>  static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
>  {
> -	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
> -		return __set_memory_enc_pgtable(addr, numpages, enc);
> +	int ret = 0;
>  
> -	return 0;
> +	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
> +		if (!down_read_trylock(&mem_enc_lock))
> +			return -EBUSY;

This function is called on SEV* and HyperV and the respective folks need
to at least ack this new approach.

I see Ashish's patch adds the respective stuff:

https://lore.kernel.org/r/c24516a4636a36d57186ea90ae26495b3c1cfb8b.1714148366.git.ashish.kalra@amd.com

which leaves HyperV. You'd need to Cc them on the next submission.

Thx.


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 10/18] x86/tdx: Convert shared memory back to private on kexec

[Kirill A. Shutemov]

On Sun, May 05, 2024 at 02:13:19PM +0200, Borislav Petkov wrote:
> On Tue, Apr 09, 2024 at 02:30:02PM +0300, Kirill A. Shutemov wrote:
> > TDX guests allocate shared buffers to perform I/O. It is done by
> > allocating pages normally from the buddy allocator and converting them
> > to shared with set_memory_decrypted().
> > 
> > The second kernel has no idea what memory is converted this way. It only
> 
> "The kexec-ed kernel..."
> 
> is more precise.

"second kernel" is nomenclature kexec folks are using, but okay.


> > @@ -831,6 +833,73 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> >  	return 0;
> >  }
> >  
> > +/* Stop new private<->shared conversions */
> > +static void tdx_kexec_stop_conversion(bool crash)
> > +{
> > +	/*
> > +	 * Crash kernel reaches here with interrupts disabled: can't wait for
> > +	 * conversions to finish.
> > +	 *
> > +	 * If race happened, just report and proceed.
> > +	 */
> > +	bool wait_for_lock = !crash;
> 
> You don't need that bool - use crash.

Dave suggested the variable for documentation purposes.

https://lore.kernel.org/all/0b70ee1e-4bb5-4867-9378-f5723ca091d5@intel.com

I'm fine either way.

> > +
> > +	addr = PAGE_OFFSET;
> > +	end  = PAGE_OFFSET + get_max_mapped();
> > +
> > +	while (addr < end) {
> > +		unsigned long size;
> > +		unsigned int level;
> > +		pte_t *pte;
> > +
> > +		pte = lookup_address(addr, &level);
> > +		size = page_level_size(level);
> > +
> > +		if (pte && pte_decrypted(*pte)) {
> > +			int pages = size / PAGE_SIZE;
> > +
> > +			/*
> > +			 * Touching memory with shared bit set triggers implicit
> > +			 * conversion to shared.
> > +			 *
> > +			 * Make sure nobody touches the shared range from
> > +			 * now on.
> > +			 */
> 
> lockdep_assert_irqs_disabled() ?

Yep.

> > +			set_pte(pte, __pte(0));
> > +
> > +			if (!tdx_enc_status_changed(addr, pages, true)) {
> > +				pr_err("Failed to unshare range %#lx-%#lx\n",
> > +				       addr, addr + size);
> 
> Why are we printing something here if we're not really acting up on it?
> 
> Who should care here?

The only thing we can do at this point on failure is panic. It think
it is reasonable to proceed, especially for crash case.

The print leaves a trace in the log to give a clue for debug.

One possible reason for the failure is if kdump raced with memory
conversion. In this case shared bit in page table got set (or not cleared
form shared->private conversion), but the page is actually private. So this
failure is not going to affect the kexec'ed kernel.

> > +static DECLARE_RWSEM(mem_enc_lock);
> > +
> > +/*
> > + * Stop new private<->shared conversions.
> > + *
> > + * Taking the exclusive mem_enc_lock waits for in-flight conversions to complete.
> > + * The lock is not released to prevent new conversions from being started.
> > + *
> > + * If sleep is not allowed, as in a crash scenario, try to take the lock.
> > + * Failure indicates that there is a race with the conversion.
> > + */
> > +bool stop_memory_enc_conversion(bool wait)
> 
> This is a global function which means, it should be called:
> 
> set_memory_enc_stop_conversion()
> 
> or so. With the proper prefix and so on.

Sure.

> > +{
> > +	if (!wait)
> > +		return down_write_trylock(&mem_enc_lock);
> > +
> > +	down_write(&mem_enc_lock);
> > +
> > +	return true;
> > +}
> > +
> >  static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> >  {
> > -	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
> > -		return __set_memory_enc_pgtable(addr, numpages, enc);
> > +	int ret = 0;
> >  
> > -	return 0;
> > +	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
> > +		if (!down_read_trylock(&mem_enc_lock))
> > +			return -EBUSY;
> 
> This function is called on SEV* and HyperV and the respective folks need
> to at least ack this new approach.
> 
> I see Ashish's patch adds the respective stuff:
> 
> https://lore.kernel.org/r/c24516a4636a36d57186ea90ae26495b3c1cfb8b.1714148366.git.ashish.kalra@amd.com
> 
> which leaves HyperV. You'd need to Cc them on the next submission.

Okay.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 10/18] x86/tdx: Convert shared memory back to private on kexec

[Borislav Petkov]

On Mon, May 06, 2024 at 06:37:19PM +0300, Kirill A. Shutemov wrote:
> "second kernel" is nomenclature kexec folks are using, but okay.

And the "third kernel" is the one which got kexec-ed the second time?

You can make it: "The second, kexec-ed kernel" and then it is perfectly
clear.

> > > +	/*
> > > +	 * Crash kernel reaches here with interrupts disabled: can't wait for
> > > +	 * conversions to finish.
> > > +	 *
> > > +	 * If race happened, just report and proceed.
> > > +	 */
> > > +	bool wait_for_lock = !crash;
> > 
> > You don't need that bool - use crash.
> 
> Dave suggested the variable for documentation purposes.
> 
> https://lore.kernel.org/all/0b70ee1e-4bb5-4867-9378-f5723ca091d5@intel.com
> 
> I'm fine either way.

But you have the comment above it which already explains what's going
on...

> > Why are we printing something here if we're not really acting up on it?
> > 
> > Who should care here?
> 
> The only thing we can do at this point on failure is panic. It think
> it is reasonable to proceed, especially for crash case.
> 
> The print leaves a trace in the log to give a clue for debug.

Sure but you'll leave a trace if you panic right then and there, on the
first failure. Why noodle through the pages if the first failure is
already fatal?

> One possible reason for the failure is if kdump raced with memory
> conversion. In this case shared bit in page table got set (or not cleared
> form shared->private conversion), but the page is actually private. So
> this failure is not going to affect the kexec'ed kernel.

Lemme make sure I understand what you're saying here:

1. This is a fatal failure and we should panic

However,

2. the kexec-ed kernel is using a different page table so there won't be
   a mismatch between shared/private marking of the page so it doesn't
   matter

Close?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 10/18] x86/tdx: Convert shared memory back to private on kexec

[Kirill A. Shutemov]

On Wed, May 08, 2024 at 02:04:22PM +0200, Borislav Petkov wrote:
> On Mon, May 06, 2024 at 06:37:19PM +0300, Kirill A. Shutemov wrote:
> > "second kernel" is nomenclature kexec folks are using, but okay.
> 
> And the "third kernel" is the one which got kexec-ed the second time?
> 
> You can make it: "The second, kexec-ed kernel" and then it is perfectly
> clear.

Okay.

> > One possible reason for the failure is if kdump raced with memory
> > conversion. In this case shared bit in page table got set (or not cleared
> > form shared->private conversion), but the page is actually private. So
> > this failure is not going to affect the kexec'ed kernel.
> 
> Lemme make sure I understand what you're saying here:
> 
> 1. This is a fatal failure and we should panic
> 
> However,
> 
> 2. the kexec-ed kernel is using a different page table so there won't be
>    a mismatch between shared/private marking of the page so it doesn't
>    matter
> 
> Close?

Yes.

One other point is even if the failure is real and we cannot touch the
page as private, kdump kernel will boot fine as it uses pre-reserved
memory. What happens next depends on what dumping process does. We have
reasonable chance to produce useful dump on crash.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 11/18] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:30:03PM +0300, Kirill A. Shutemov wrote:
> Subject: Re: [PATCHv10 11/18] x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges
						^^^^^^^

e820__end_of_ram_pfn()

> e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
> things, guides where direct mapping ends. Any memory above max_pfn is
> not going to be present in the direct mapping.
> 
> e820__end_of_ram_pfn() finds the end of the ram based on the highest

RAM

...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv10 13/18] x86/acpi: Rename fields in acpi_madt_multiproc_wakeup structure

[Borislav Petkov]

On Tue, Apr 09, 2024 at 02:30:05PM +0300, Kirill A. Shutemov wrote:
> To prepare for the addition of support for MADT wakeup structure version

"In order to support... "

> 1, it is necessary to provide more appropriate names for the fields in
> the structure.
> 
> The field 'mailbox_version' renamed as 'version'. This field signifies

From Documentation/process/submitting-patches.rst:

 "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour."

So:

"Rename 'mailbox_version' to 'version' because... "

> the version of the structure and the related protocols, rather than the
> version of the mailbox. This field has not been utilized in the code
> thus far.
> 
> The field 'base_address' renamed as 'mailbox_address' to clarify the

Ditto.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec