Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1695581 > unrolled thread

Re: [RFC Part1 PATCH v3 02/17] x86/CPU/AMD: Add the Secure Encrypted Virtualization CPU feature

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC Part1 PATCH v3 02/17] x86/CPU/AMD: Add the Secure Encrypted  Virtualization CPU feature Borislav Petkov <bp@suse.de> - 2017-07-25 12:30 +0200
    Re: [RFC Part1 PATCH v3 02/17] x86/CPU/AMD: Add the Secure Encrypted  Virtualization CPU feature Borislav Petkov <bp@suse.de> - 2017-07-25 16:40 +0200
      Re: [RFC Part1 PATCH v3 02/17] x86/CPU/AMD: Add the Secure Encrypted  Virtualization CPU feature Borislav Petkov <bp@suse.de> - 2017-07-25 17:20 +0200
        Re: [RFC Part1 PATCH v3 02/17] x86/CPU/AMD: Add the Secure Encrypted  Virtualization CPU feature Borislav Petkov <bp@suse.de> - 2017-07-25 17:40 +0200

#1695581 — Re: [RFC Part1 PATCH v3 02/17] x86/CPU/AMD: Add the Secure Encrypted Virtualization CPU feature

On Mon, Jul 24, 2017 at 02:07:42PM -0500, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> Update the CPU features to include identifying and reporting on the
> Secure Encrypted Virtualization (SEV) feature.  SME is identified by
> CPUID 0x8000001f, but requires BIOS support to enable it (set bit 23 of
> MSR_K8_SYSCFG and set bit 0 of MSR_K7_HWCR).  Only show the SEV feature
> as available if reported by CPUID and enabled by BIOS.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/include/asm/cpufeatures.h |  1 +
>  arch/x86/include/asm/msr-index.h   |  2 ++
>  arch/x86/kernel/cpu/amd.c          | 30 +++++++++++++++++++++++++-----
>  arch/x86/kernel/cpu/scattered.c    |  1 +
>  4 files changed, 29 insertions(+), 5 deletions(-)

...

> @@ -637,6 +642,21 @@ static void early_init_amd(struct cpuinfo_x86 *c)
>  			clear_cpu_cap(c, X86_FEATURE_SME);
>  		}
>  	}
> +
> +	if (cpu_has(c, X86_FEATURE_SEV)) {
> +		if (IS_ENABLED(CONFIG_X86_32)) {
> +			clear_cpu_cap(c, X86_FEATURE_SEV);
> +		} else {
> +			u64 syscfg, hwcr;
> +
> +			/* Check if SEV is enabled */
> +			rdmsrl(MSR_K8_SYSCFG, syscfg);
> +			rdmsrl(MSR_K7_HWCR, hwcr);
> +			if (!(syscfg & MSR_K8_SYSCFG_MEM_ENCRYPT) ||
> +			    !(hwcr & MSR_K7_HWCR_SMMLOCK))
> +				clear_cpu_cap(c, X86_FEATURE_SEV);
> +		}
> +	}

Let's simplify this and read the MSRs only once. Diff ontop. Please
check if I'm missing a case:

---
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index c413f04bdd41..79af07731ab1 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -546,6 +546,48 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
 	}
 }
 
+static void early_detect_mem_enc(struct cpuinfo_x86 *c)
+{
+	u64 syscfg, hwcr;
+
+	/*
+	 * BIOS support is required for SME and SEV.
+	 *   For SME: If BIOS has enabled SME then adjust x86_phys_bits by
+	 *	      the SME physical address space reduction value.
+	 *	      If BIOS has not enabled SME then don't advertise the
+	 *	      SME feature (set in scattered.c).
+	 *   For SEV: If BIOS has not enabled SEV then don't advertise the
+	 *            SEV feature (set in scattered.c).
+	 *
+	 *   In all cases, since support for SME and SEV requires long mode,
+	 *   don't advertise the feature under CONFIG_X86_32.
+	 */
+	if (cpu_has(c, X86_FEATURE_SME) ||
+	    cpu_has(c, X86_FEATURE_SEV)) {
+
+		if (IS_ENABLED(CONFIG_X86_32))
+			goto clear;
+
+		/* Check if SME is enabled */
+		rdmsrl(MSR_K8_SYSCFG, syscfg);
+		if (!(syscfg & MSR_K8_SYSCFG_MEM_ENCRYPT))
+			goto clear;
+
+		c->x86_phys_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f;
+
+		/* Check if SEV is enabled */
+		rdmsrl(MSR_K7_HWCR, hwcr);
+		if (!(hwcr & MSR_K7_HWCR_SMMLOCK))
+			goto clear_sev;
+
+		return;
+clear:
+		clear_cpu_cap(c, X86_FEATURE_SME);
+clear_sev:
+		clear_cpu_cap(c, X86_FEATURE_SEV);
+	}
+}
+
 static void early_init_amd(struct cpuinfo_x86 *c)
 {
 	u32 dummy;
@@ -617,46 +659,8 @@ static void early_init_amd(struct cpuinfo_x86 *c)
 	if (cpu_has_amd_erratum(c, amd_erratum_400))
 		set_cpu_bug(c, X86_BUG_AMD_E400);
 
-	/*
-	 * BIOS support is required for SME and SEV.
-	 *   For SME: If BIOS has enabled SME then adjust x86_phys_bits by
-	 *	      the SME physical address space reduction value.
-	 *	      If BIOS has not enabled SME then don't advertise the
-	 *	      SME feature (set in scattered.c).
-	 *   For SEV: If BIOS has not enabled SEV then don't advertise the
-	 *            SEV feature (set in scattered.c).
-	 *
-	 *   In all cases, since support for SME and SEV requires long mode,
-	 *   don't advertise the feature under CONFIG_X86_32.
-	 */
-	if (cpu_has(c, X86_FEATURE_SME)) {
-		u64 msr;
-
-		/* Check if SME is enabled */
-		rdmsrl(MSR_K8_SYSCFG, msr);
-		if (msr & MSR_K8_SYSCFG_MEM_ENCRYPT) {
-			c->x86_phys_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f;
-			if (IS_ENABLED(CONFIG_X86_32))
-				clear_cpu_cap(c, X86_FEATURE_SME);
-		} else {
-			clear_cpu_cap(c, X86_FEATURE_SME);
-		}
-	}
+	early_detect_mem_enc(c);
 
-	if (cpu_has(c, X86_FEATURE_SEV)) {
-		if (IS_ENABLED(CONFIG_X86_32)) {
-			clear_cpu_cap(c, X86_FEATURE_SEV);
-		} else {
-			u64 syscfg, hwcr;
-
-			/* Check if SEV is enabled */
-			rdmsrl(MSR_K8_SYSCFG, syscfg);
-			rdmsrl(MSR_K7_HWCR, hwcr);
-			if (!(syscfg & MSR_K8_SYSCFG_MEM_ENCRYPT) ||
-			    !(hwcr & MSR_K7_HWCR_SMMLOCK))
-				clear_cpu_cap(c, X86_FEATURE_SEV);
-		}
-	}
 }
 
 static void init_amd_k8(struct cpuinfo_x86 *c)

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [next] | [standalone]

#1695790

On Tue, Jul 25, 2017 at 09:29:40AM -0500, Tom Lendacky wrote:
> Yup, we can do something like that.  I believe the only change that
> would be needed to your patch would be to move the IS_ENABLED() check
> to after the physical address space reduction check.

Yeah, I wasn't sure about that. The logic is that if BIOS has enabled
SME and thus reduction is in place, we need to update x86_phys_bits on
32-bit regardless, right?

But, come to think of it, that reduction won't have any effect since we
have 32-bit addresses and the reduction is above 32-bits, right? And
thus it is moot.

Or?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]

#1695832

On Tue, Jul 25, 2017 at 09:58:54AM -0500, Tom Lendacky wrote:
> True, but it is more about being accurate and making sure the value is
> correct where ever it may be used.

So early_identify_cpu() initializes phys_bits to 32 on 32-bit.
Subtracting it there would actually make actively it wrong, AFAICT.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]

#1695857

On Tue, Jul 25, 2017 at 10:29:40AM -0500, Tom Lendacky wrote:
> But early_identify_cpu() calls get_cpu_cap() which will check for cpuid
> leaf 0x80000008 support and set x86_phys_bits.

Right, but it can't be less than 32, can it? And if it is more than 32
bits, then it probably doesn't really matter on 32-bit. Unless it is
less than 36 bits and you do PAE...

> I'll try to build and run a 32-bit kernel and see how this all flows.

Yeah, that would be good.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web