| Message ID | 20170725102657.GD21822@nazgul.tnic (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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: 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(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) >
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?
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? >
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.
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 0x80000008 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 >
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.
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 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. Ok, finally got around to running a 32-bit kernel and it reports x86_phys_bits as 48. Thanks, Tom > > Thanks. >
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.
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)