Re: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted

[Borislav Petkov]

On Thu, Mar 09, 2023 at 03:45:35PM +0000, David Woodhouse wrote:
> cc_vendor isn't yet exposed. As we discussed this in IRC, I've been

Right, and we might just as well expose it because having
a setter/getter is kinda silly for a __ro_after_init variable, see
below.

So here's what I was able to scratch up quickly to remove the static
key.

The asm looks like this:

# ./arch/x86/include/asm/sev.h:156: 	if (cc_vendor == CC_VENDOR_AMD &&
	cmpl	$1, cc_vendor(%rip)	#, cc_vendor
	je	.L204	#,

...

.L204:
# ./arch/x86/include/asm/sev.h:157:         cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
        movl    $3, %edi        #,
        call    cc_platform_has #
# ./arch/x86/include/asm/sev.h:156:     if (cc_vendor == CC_VENDOR_AMD &&
        testb   %al, %al        # tmp134
        je      .L158   #,

and so I doubt that this is at all measureable comparing that to the
rest of the code that gets executed in the NMI handler. And it lets us
get rid of yet another static key and use only the CC APIs.

Oh, and it bitches because cc_platform_has() is being called in noinstr
region but we can mark it noinstr or add the drop-noinstr markers around
it, if needed. Not that important as that function and what it calls
don't do anything magical.

Thx.

---
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index 49b44f881484..34446383e68b 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -13,7 +13,7 @@
 #include <asm/coco.h>
 #include <asm/processor.h>
 
-static enum cc_vendor vendor __ro_after_init;
+enum cc_vendor cc_vendor __ro_after_init;
 static u64 cc_mask __ro_after_init;
 
 static bool intel_cc_platform_has(enum cc_attr attr)
@@ -83,7 +83,7 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)
 
 bool cc_platform_has(enum cc_attr attr)
 {
-	switch (vendor) {
+	switch (cc_vendor) {
 	case CC_VENDOR_AMD:
 		return amd_cc_platform_has(attr);
 	case CC_VENDOR_INTEL:
@@ -105,7 +105,7 @@ u64 cc_mkenc(u64 val)
 	 * - for AMD, bit *set* means the page is encrypted
 	 * - for Intel *clear* means encrypted.
 	 */
-	switch (vendor) {
+	switch (cc_vendor) {
 	case CC_VENDOR_AMD:
 		return val | cc_mask;
 	case CC_VENDOR_INTEL:
@@ -118,7 +118,7 @@ u64 cc_mkenc(u64 val)
 u64 cc_mkdec(u64 val)
 {
 	/* See comment in cc_mkenc() */
-	switch (vendor) {
+	switch (cc_vendor) {
 	case CC_VENDOR_AMD:
 		return val & ~cc_mask;
 	case CC_VENDOR_INTEL:
@@ -131,7 +131,7 @@ EXPORT_SYMBOL_GPL(cc_mkdec);
 
 __init void cc_set_vendor(enum cc_vendor v)
 {
-	vendor = v;
+	cc_vendor = v;
 }
 
 __init void cc_set_mask(u64 mask)
diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index 3d98c3a60d34..0563e07a1002 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -11,6 +11,8 @@ enum cc_vendor {
 	CC_VENDOR_INTEL,
 };
 
+extern enum cc_vendor cc_vendor;
+
 void cc_set_vendor(enum cc_vendor v);
 void cc_set_mask(u64 mask);
 
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index ebc271bb6d8e..1335781e4976 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -12,6 +12,7 @@
 #include <asm/insn.h>
 #include <asm/sev-common.h>
 #include <asm/bootparam.h>
+#include <asm/coco.h>
 
 #define GHCB_PROTOCOL_MIN	1ULL
 #define GHCB_PROTOCOL_MAX	2ULL
@@ -134,24 +135,26 @@ struct snp_secrets_page_layout {
 } __packed;
 
 #ifdef CONFIG_AMD_MEM_ENCRYPT
-extern struct static_key_false sev_es_enable_key;
 extern void __sev_es_ist_enter(struct pt_regs *regs);
 extern void __sev_es_ist_exit(void);
 static __always_inline void sev_es_ist_enter(struct pt_regs *regs)
 {
-	if (static_branch_unlikely(&sev_es_enable_key))
+	if (cc_vendor == CC_VENDOR_AMD &&
+	    cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
 		__sev_es_ist_enter(regs);
 }
 static __always_inline void sev_es_ist_exit(void)
 {
-	if (static_branch_unlikely(&sev_es_enable_key))
+	if (cc_vendor == CC_VENDOR_AMD &&
+	    cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
 		__sev_es_ist_exit();
 }
 extern int sev_es_setup_ap_jump_table(struct real_mode_header *rmh);
 extern void __sev_es_nmi_complete(void);
 static __always_inline void sev_es_nmi_complete(void)
 {
-	if (static_branch_unlikely(&sev_es_enable_key))
+	if (cc_vendor == CC_VENDOR_AMD &&
+	    cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
 		__sev_es_nmi_complete();
 }
 extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 679026a640ef..7d873bffbd8e 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -111,7 +111,6 @@ struct ghcb_state {
 };
 
 static DEFINE_PER_CPU(struct sev_es_runtime_data*, runtime_data);
-DEFINE_STATIC_KEY_FALSE(sev_es_enable_key);
 
 static DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa);
 
@@ -1393,9 +1392,6 @@ void __init sev_es_init_vc_handling(void)
 			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
 	}
 
-	/* Enable SEV-ES special handling */
-	static_branch_enable(&sev_es_enable_key);
-
 	/* Initialize per-cpu GHCB pages */
 	for_each_possible_cpu(cpu) {
 		alloc_runtime_data(cpu);

-- 
Regards/Gruss,
    Boris.

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