[PATCH v4] x86/boot: Don't return encryption mask from __startup_64()

[PATCH v4] x86/boot: Don't return encryption mask from __startup_64()

[Khalid Ali]

Avoid returning the SME encryption mask from __startup_64(), and instead
let the function handle encryption directly as needed. The encryption
mask is already available to callers and can be accessed via
sme_get_me_mask() in C code, or directly through the sme_me_mask symbol
in assembly, if CONFIG_AMD_MEM_ENCRYPT is enabled.

This change aligns with how secondary_startup_64_no_verify handles SME
and keeps the behavior consistent. For Intel CPUs, SME is not relevant,
so there is no need to retrieve the mask unless CONFIG_AMD_MEM_ENCRYPT
is enabled.

Signed-off-by: Khalid Ali <khaliidcaliy@gmail.com>
---
 arch/x86/boot/startup/map_kernel.c | 11 +++--------
 arch/x86/include/asm/setup.h       |  2 +-
 arch/x86/kernel/head_64.S          | 12 +++++-------
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/arch/x86/boot/startup/map_kernel.c b/arch/x86/boot/startup/map_kernel.c
index 332dbe6688c4..6fdb340e9147 100644
--- a/arch/x86/boot/startup/map_kernel.c
+++ b/arch/x86/boot/startup/map_kernel.c
@@ -30,7 +30,7 @@ static inline bool check_la57_support(void)
 	return true;
 }
 
-static unsigned long __head sme_postprocess_startup(struct boot_params *bp,
+static void __head sme_postprocess_startup(struct boot_params *bp,
 						    pmdval_t *pmd,
 						    unsigned long p2v_offset)
 {
@@ -68,11 +68,6 @@ static unsigned long __head sme_postprocess_startup(struct boot_params *bp,
 		}
 	}
 
-	/*
-	 * Return the SME encryption mask (if SME is active) to be used as a
-	 * modifier for the initial pgdir entry programmed into CR3.
-	 */
-	return sme_get_me_mask();
 }
 
 /*
@@ -84,7 +79,7 @@ static unsigned long __head sme_postprocess_startup(struct boot_params *bp,
  * the 1:1 mapping of memory. Kernel virtual addresses can be determined by
  * subtracting p2v_offset from the RIP-relative address.
  */
-unsigned long __head __startup_64(unsigned long p2v_offset,
+void __head __startup_64(unsigned long p2v_offset,
 				  struct boot_params *bp)
 {
 	pmd_t (*early_pgts)[PTRS_PER_PMD] = rip_rel_ptr(early_dynamic_pgts);
@@ -213,5 +208,5 @@ unsigned long __head __startup_64(unsigned long p2v_offset,
 	for (; i < PTRS_PER_PMD; i++)
 		pmd[i] &= ~_PAGE_PRESENT;
 
-	return sme_postprocess_startup(bp, pmd, p2v_offset);
+	sme_postprocess_startup(bp, pmd, p2v_offset);
 }
diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
index 692af46603a1..29ea24bb85ff 100644
--- a/arch/x86/include/asm/setup.h
+++ b/arch/x86/include/asm/setup.h
@@ -50,7 +50,7 @@ extern unsigned long acpi_realmode_flags;
 
 extern void reserve_standard_io_resources(void);
 extern void i386_reserve_resources(void);
-extern unsigned long __startup_64(unsigned long p2v_offset, struct boot_params *bp);
+extern void __startup_64(unsigned long p2v_offset, struct boot_params *bp);
 extern void startup_64_setup_gdt_idt(void);
 extern void startup_64_load_idt(void *vc_handler);
 extern void early_setup_idt(void);
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 3e9b3a3bd039..8b50bdd90927 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -106,18 +106,16 @@ SYM_CODE_START_NOALIGN(startup_64)
 
 	/*
 	 * Perform pagetable fixups. Additionally, if SME is active, encrypt
-	 * the kernel and retrieve the modifier (SME encryption mask if SME
-	 * is active) to be added to the initial pgdir entry that will be
-	 * programmed into CR3.
-	 */
+	 * the kernel.
+	 /
 	movq	%r15, %rsi
 	call	__startup_64
 
 	/* Form the CR3 value being sure to include the CR3 modifier */
-	leaq	early_top_pgt(%rip), %rcx
-	addq	%rcx, %rax
-
+	leaq	early_top_pgt(%rip), %rax
+	
 #ifdef CONFIG_AMD_MEM_ENCRYPT
+	addq	sme_me_mask(%rip), %rax
 	mov	%rax, %rdi
 
 	/*
-- 
2.49.0

Re: [PATCH v4] x86/boot: Don't return encryption mask from __startup_64()

[Huang, Kai]

On Thu, 2025-06-19 at 07:36 +0000, Khalid Ali wrote:
> Avoid returning the SME encryption mask from __startup_64(), and instead
> let the function handle encryption directly as needed. 
> 

Some nits below:

"the function" here is confusing, since it sounds like you are referring to
__startup_64(), but I think you actually meant its caller.

So, "the function" -> "the caller".


> The encryption
> mask is already available to callers and can be accessed via 

"callers" -> "users", since only functions can have callers?

Or just:

  The encryption mask is already available after sme_enable() and ...

? 

> sme_get_me_mask() in C code, or directly through the sme_me_mask symbol
> in assembly, if CONFIG_AMD_MEM_ENCRYPT is enabled.
> 
> This change aligns with how secondary_startup_64_no_verify handles SME
> and keeps the behavior consistent. For Intel CPUs, SME is not relevant,
> so there is no need to retrieve the mask unless CONFIG_AMD_MEM_ENCRYPT
> is enabled.
> 
> Signed-off-by: Khalid Ali <khaliidcaliy@gmail.com>

Overall I think this patch makes code logic clearer, so:

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

Re: [PATCH v4] x86/boot: Don't return encryption mask from __startup_64()

[kernel test robot]

Hello,

kernel test robot noticed "BUG:kernel_failed_in_early-boot_stage,last_printk:Booting_the_kernel(entry_offset:#)" on:

commit: ceefc6a6846712aaeee61b8dfa97d5c33313d063 ("[PATCH v4] x86/boot: Don't return encryption mask from __startup_64()")
url: https://github.com/intel-lab-lkp/linux/commits/Khalid-Ali/x86-boot-Don-t-return-encryption-mask-from-__startup_64/20250619-153838
base: https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git 6a7c3c2606105a41dde81002c0037420bc1ddf00
patch link: https://lore.kernel.org/all/20250619073652.719-1-khaliidcaliy@gmail.com/
patch subject: [PATCH v4] x86/boot: Don't return encryption mask from __startup_64()

in testcase: boot

config: x86_64-kexec
compiler: clang-20
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+--------------------------------------------------------------------------------------+------------+------------+
|                                                                                      | 6a7c3c2606 | ceefc6a684 |
+--------------------------------------------------------------------------------------+------------+------------+
| boot_successes                                                                       | 6          | 0          |
| boot_failures                                                                        | 0          | 6          |
| BUG:kernel_failed_in_early-boot_stage,last_printk:Booting_the_kernel(entry_offset:#) | 0          | 6          |
+--------------------------------------------------------------------------------------+------------+------------+


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 <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202506251439.913b4c32-lkp@intel.com


trampoline_32bit: 0x0000000000000000

Decompressing Linux... Parsing ELF... done.
Booting the kernel (entry_offset: 0x0000000001161e30).
convert early boot stage from reboot-without-warning to failed
BUG: kernel failed in early-boot stage, last printk: Booting the kernel (entry_offset: 0x0000000001161e30).
Linux version 6.15.0-rc7-00359-gceefc6a68467 #1
Command line: ip=::::vm-meta-270::dhcp root=/dev/ram0 RESULT_ROOT=/result/boot/1/vm-snb/debian-11.1-i386-20220923.cgz/x86_64-kexec/clang-20/ceefc6a6846712aaeee61b8dfa97d5c33313d063/0 BOOT_IMAGE=/pkg/linux/x86_64-kexec/clang-20/ceefc6a6846712aaeee61b8dfa97d5c33313d063/vmlinuz-6.15.0-rc7-00359-gceefc6a68467 branch=linux-devel/devel-hourly-20250619-233935 job=/lkp/jobs/scheduled/vm-meta-270/boot-1-debian-11.1-i386-20220923.cgz-ceefc6a68467-20250621-121357-1bgjd25-0.yaml user=lkp ARCH=x86_64 kconfig=x86_64-kexec commit=ceefc6a6846712aaeee61b8dfa97d5c33313d063 intremap=posted_msi vmalloc=256M initramfs_async=0 page_owner=on carrier_timeout=60 rcupdate.rcu_self_test=0 max_uptime=600 LKP_SERVER=internal-lkp-server selinux=0 debug apic=debug sysrq_always_enabled rcupdate.rcu_cpu_stall_timeout=100 net.ifnames=0 printk.devkmsg=on panic=-1 softlockup_panic=1 nmi_watchdog=panic oops=panic load_ramdisk=2 prompt_ramdisk=0 drbd.minor_count=8 systemd.log_level=err ignore_loglevel console=tty0 earlyprintk=ttyS0,115200 console=ttyS0,115200 vga=normal rw rcuperf.shutdown=0 rcuscale.shutdown=0 refscale.shutdown=0 watchdog_thresh=240 audit=0 kunit.enable=0 ia32_emulation=on riscv_isa_fallback=1

Kboot worker: lkp-worker51
Elapsed time: 60


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250625/202506251439.913b4c32-lkp@intel.com



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

Re: [PATCH v4] x86/boot: Don't return encryption mask from __startup_64()

[Huang, Kai]

On Thu, 2025-06-19 at 07:36 +0000, Khalid Ali wrote:
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -106,18 +106,16 @@ SYM_CODE_START_NOALIGN(startup_64)
>  
>  	/*
>  	 * Perform pagetable fixups. Additionally, if SME is active, encrypt
> -	 * the kernel and retrieve the modifier (SME encryption mask if SME
> -	 * is active) to be added to the initial pgdir entry that will be
> -	 * programmed into CR3.
> -	 */
> +	 * the kernel.
> +	 /

Sigh... this comment is broken, since there's no '*' before the last '/'.

>  	movq	%r15, %rsi
>  	call	__startup_64

Here's how I find it:

So I went to see why this patch caused early boot failure, since the code
change doesn't seem wrong to me.

After staring at the code for half hour and yet unable to see any issue, I
went to disassemble the kernel image, then I found the above two lines of
code wasn't there at all.

Then looking at this again, it's obvious that the reason is the change to
the comment is broken, leading the above two lines of code being commented
out.

With this fixed, I can boot the kernel in a normal VM (both w/ and w/o
CONFIG_AMD_MEM_ENCRYPT).

And this patch has other style issues too like the broken indent of function
parameters after changing returning value from 'unsigned int' to 'void', and
...
	
>  
>  	/* Form the CR3 value being sure to include the CR3 modifier */
> -	leaq	early_top_pgt(%rip), %rcx
> -	addq	%rcx, %rax
> -
> +	leaq	early_top_pgt(%rip), %rax
> +	
... a tailing whitespace here.

So, please, before posting patches, test them, and run
./scripts/checkpatch.pl against them.

And I would suggest AMD guys to avoid looking into this until those issues
are fixed.

Re: [PATCH v4] x86/boot: Don't return encryption mask from __startup_64()

[Khalid Ali]

> Sigh... this comment is broken, since there's no '*' before the last '/'.

>Here's how I find it:
>
>So I went to see why this patch caused early boot failure, since the code
>change doesn't seem wrong to me.
>
>After staring at the code for half hour and yet unable to see any issue, I
>went to disassemble the kernel image, then I found the above two lines of
>code wasn't there at all.
>
>Then looking at this again, it's obvious that the reason is the change to
>the comment is broken, leading the above two lines of code being commented
>out.

So what i was wringling all along was the comment. I don't know why i couldn't spot it
if those two lines where commented out then even boot was supposed to fail on my side too.
I guess toolchain difference. Probably this is the reason since i compiled with gcc not clang.

Thanks though. :)

>And this patch has other style issues too like the broken indent of function
>parameters after changing returning value from 'unsigned int' to 'void', and
Could please clarify more as checkpatch.pl didn't notice it.

>So, please, before posting patches, test them, and run
>./scripts/checkpatch.pl against them.

I used it, well the tool seems broken and giving me false positive. I fixed all your suggestions and sent 
v5, as my best i could, i fixed all issues and that patch seems good. 

Could you please check it?