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

[Borislav Petkov]

On Wed, Aug 09, 2017 at 01:17:54PM -0500, Tom Lendacky wrote:
> Ok, finally got around to running a 32-bit kernel and it reports
> x86_phys_bits as 48.

So it doesn't really matter on 32-bit. I guess you could add a comment
saying why we don't care.

Thanks.

-- 
Regards/Gruss,
Boris.

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

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

[Borislav Petkov]

On Wed, Aug 09, 2017 at 01:17:54PM -0500, Tom Lendacky wrote:
> Ok, finally got around to running a 32-bit kernel and it reports
> x86_phys_bits as 48.

So it doesn't really matter on 32-bit. I guess you could add a comment
saying why we don't care.

Thanks.

-- 
Regards/Gruss,
Boris.

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

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

[Tom Lendacky]

On 7/25/2017 10:33 AM, Borislav Petkov wrote:

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 0x8008 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.


Ok, finally got around to running a 32-bit kernel and it reports
x86_phys_bits as 48.

Thanks,
Tom



Thanks.

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

[Tom Lendacky]

On 7/25/2017 10:33 AM, Borislav Petkov wrote:

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 0x8008 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.


Ok, finally got around to running a 32-bit kernel and it reports
x86_phys_bits as 48.

Thanks,
Tom



Thanks.

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

[Borislav Petkov]

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 0x8008 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)
-- 

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

[Borislav Petkov]

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 0x8008 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)
-- 

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

[Tom Lendacky]

On 7/25/2017 10:13 AM, Borislav Petkov wrote:

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.


But early_identify_cpu() calls get_cpu_cap() which will check for cpuid
leaf 0x8008 support and set x86_phys_bits.  I'll try to build and
run a 32-bit kernel and see how this all flows.

Thanks,
Tom

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

[Tom Lendacky]

On 7/25/2017 10:13 AM, Borislav Petkov wrote:

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.


But early_identify_cpu() calls get_cpu_cap() which will check for cpuid
leaf 0x8008 support and set x86_phys_bits.  I'll try to build and
run a 32-bit kernel and see how this all flows.

Thanks,
Tom

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

[Borislav Petkov]

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)
-- 

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

[Borislav Petkov]

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)
-- 

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

[Tom Lendacky]

On 7/25/2017 9:36 AM, Borislav Petkov wrote:

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.



True, but it is more about being accurate and making sure the value is
correct where ever it may be used.

Thanks,
Tom



Or?

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

[Tom Lendacky]

On 7/25/2017 9:36 AM, Borislav Petkov wrote:

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.



True, but it is more about being accurate and making sure the value is
correct where ever it may be used.

Thanks,
Tom



Or?

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

[Borislav Petkov]

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)
-- 

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

[Borislav Petkov]

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)
-- 

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

[Tom Lendacky]

On 7/25/2017 5:26 AM, Borislav Petkov wrote:

On Mon, Jul 24, 2017 at 02:07:42PM -0500, Brijesh Singh wrote:

From: Tom Lendacky 

Update the CPU features to include identifying and reporting on the
Secure Encrypted Virtualization (SEV) feature.  SME is identified by
CPUID 0x801f, 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 
Signed-off-by: Brijesh Singh 
---
  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:


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.

Thanks,
Tom



---
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(0x801f) >> 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(0x801f) >> 6) & 0x3f;
-   if (IS_ENABLED(CONFIG_X86_32))
-   clear_cpu_cap(c, 

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

[Tom Lendacky]

On 7/25/2017 5:26 AM, Borislav Petkov wrote:

On Mon, Jul 24, 2017 at 02:07:42PM -0500, Brijesh Singh wrote:

From: Tom Lendacky 

Update the CPU features to include identifying and reporting on the
Secure Encrypted Virtualization (SEV) feature.  SME is identified by
CPUID 0x801f, 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 
Signed-off-by: Brijesh Singh 
---
  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:


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.

Thanks,
Tom



---
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(0x801f) >> 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(0x801f) >> 6) & 0x3f;
-   if (IS_ENABLED(CONFIG_X86_32))
-   clear_cpu_cap(c, X86_FEATURE_SME);
-   } else {
-   clear_cpu_cap(c, 

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

[Borislav Petkov]

On Mon, Jul 24, 2017 at 02:07:42PM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> Update the CPU features to include identifying and reporting on the
> Secure Encrypted Virtualization (SEV) feature.  SME is identified by
> CPUID 0x801f, 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 
> Signed-off-by: Brijesh Singh 
> ---
>  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(0x801f) >> 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(0x801f) >> 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 

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

[Borislav Petkov]

On Mon, Jul 24, 2017 at 02:07:42PM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> Update the CPU features to include identifying and reporting on the
> Secure Encrypted Virtualization (SEV) feature.  SME is identified by
> CPUID 0x801f, 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 
> Signed-off-by: Brijesh Singh 
> ---
>  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(0x801f) >> 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(0x801f) >> 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,