Received-SPF: pass (google.com: domain of linux-kernel+bounces-64207-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 13 Feb 2024 21:05:53 +0100
From: Borislav Petkov <bp@alien8.de>
To: Ard Biesheuvel <ardb+git@google.com>
Cc: linux-kernel@vger.kernel.org, Ard Biesheuvel <ardb@kernel.org>,
	Kevin Loughlin <kevinloughlin@google.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Dionna Glaze <dionnaglaze@google.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Andy Lutomirski <luto@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Justin Stitt <justinstitt@google.com>,
	Kees Cook <keescook@chromium.org>, Brian Gerst <brgerst@gmail.com>,
	linux-arch@vger.kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH v4 01/11] x86/startup_64: Simplify global variable
 accesses in GDT/IDT programming
Message-ID: <20240213200553.GYZcvLoYUNJOPGxoid@fat_crate.local>
References: <20240213124143.1484862-13-ardb+git@google.com>
 <20240213124143.1484862-14-ardb+git@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240213124143.1484862-14-ardb+git@google.com>

On Tue, Feb 13, 2024 at 01:41:45PM +0100, Ard Biesheuvel wrote:
> @@ -632,5 +616,5 @@ void __head startup_64_setup_env(unsigned long physbase)
>  		     "movl %%eax, %%ss\n"
>  		     "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory");
>  
> -	startup_64_load_idt(physbase);
> +	startup_64_load_idt(&RIP_REL_REF(vc_no_ghcb));

It took me a while to figure out that even if we pass in one of the two
GHCB handler pointers, we only set it if CONFIG_AMD_MEM_ENCRYPT.

I think this ontop of yours is a bit more readable as it makes it
perfectly clear *when* the pointer is valid.

Yeah, if handler is set, we set it for the X86_TRAP_VC vector
unconditionally but that can be changed later, if really needed.

But this way it is clear by having the callers select the pointer. I.e.,
it is a more common coding pattern this way, I'd say.

Thx.

---

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 9d7f12829f2d..e39114c348e2 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -582,8 +582,8 @@ static void __head startup_64_load_idt(void *handler)
 	};
 	gate_desc *idt = (gate_desc *)desc.address;
 
-	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
-		/* VMM Communication Exception */
+	/* @handler is set only for a VMM Communication Exception */
+	if (handler)
 		set_bringup_idt_handler(idt, X86_TRAP_VC, handler);
 
 	native_load_idt(&desc);
@@ -592,10 +592,14 @@ static void __head startup_64_load_idt(void *handler)
 /* This is used when running on kernel addresses */
 void early_setup_idt(void)
 {
-	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
+	void *handler = NULL;
+
+	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
 		setup_ghcb();
+		handler = vc_boot_ghcb;
+	}
 
-	startup_64_load_idt(vc_boot_ghcb);
+	startup_64_load_idt(handler);
 }
 
 /*
@@ -603,6 +607,8 @@ void early_setup_idt(void)
  */
 void __head startup_64_setup_gdt_idt(void)
 {
+	void *handler = NULL;
+
 	struct desc_ptr startup_gdt_descr = {
 		.address	= (unsigned long)&RIP_REL_REF(startup_gdt),
 		.size		= sizeof(startup_gdt) - 1,
@@ -616,5 +622,8 @@ void __head startup_64_setup_gdt_idt(void)
 		     "movl %%eax, %%ss\n"
 		     "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory");
 
-	startup_64_load_idt(&RIP_REL_REF(vc_no_ghcb));
+	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
+		handler = &RIP_REL_REF(vc_no_ghcb);
+
+	startup_64_load_idt(handler);
 }

-- 
Regards/Gruss,
    Boris.

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