| Message ID | 20240124130317.495519-1-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] x86/sev: Fix SEV check in sev_map_percpu_data() | expand |
On Wed, Jan 24, 2024 at 2:09 PM Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote: > > The function sev_map_percpu_data() checks if it is running on an SEV > platform by checking the CC_ATTR_GUEST_MEM_ENCRYPT attribute. However, > this attribute is also defined for TDX. > > To avoid false positives, add a cc_vendor check. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Fixes: 4d96f9109109 ("x86/sev: Replace occurrences of sev_active() with cc_platform_has()") > Suggested-by: Borislav Petkov (AMD) <bp@alien8.de> > Acked-by: David Rientjes <rientjes@google.com> Queued, with "x86/kvm in the subject". Paolo > --- > arch/x86/kernel/kvm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index dfe9945b9bec..428ee74002e1 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -434,7 +434,8 @@ static void __init sev_map_percpu_data(void) > { > int cpu; > > - if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) > + if (cc_vendor != CC_VENDOR_AMD || > + !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) > return; > > for_each_possible_cpu(cpu) { > -- > 2.43.0 >
On Wed, Jan 24, 2024 at 03:03:17PM +0200, Kirill A. Shutemov wrote: > The function sev_map_percpu_data() checks if it is running on an SEV > platform by checking the CC_ATTR_GUEST_MEM_ENCRYPT attribute. However, > this attribute is also defined for TDX. > > To avoid false positives, add a cc_vendor check. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Fixes: 4d96f9109109 ("x86/sev: Replace occurrences of sev_active() with cc_platform_has()") > Suggested-by: Borislav Petkov (AMD) <bp@alien8.de> > Acked-by: David Rientjes <rientjes@google.com> > --- > arch/x86/kernel/kvm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index dfe9945b9bec..428ee74002e1 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -434,7 +434,8 @@ static void __init sev_map_percpu_data(void) > { > int cpu; > > - if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) > + if (cc_vendor != CC_VENDOR_AMD || > + !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) > return; > > for_each_possible_cpu(cpu) { > -- > 2.43.0 > Our CI has started seeing a build failure as a result of this patch when using LLVM to build x86_64_defconfig + CONFIG_GCOV_KERNEL=y + CONFIG_GCOV_PROFILE_ALL=y: $ echo 'CONFIG_GCOV_KERNEL=y CONFIG_GCOV_PROFILE_ALL=y' >kernel/configs/gcov.config $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig gcov.config vmlinux ... ld.lld: error: undefined symbol: cc_vendor >>> referenced by kvm.c >>> arch/x86/kernel/kvm.o:(kvm_smp_prepare_boot_cpu) in archive vmlinux.a ... I was somewhat confused at first why this build error only shows up with GCOV until I looked at the optimized IR. This configuration has CONFIG_ARCH_HAS_CC_PLATFORM=n, which means that cc_vendor is declared but not defined anywhere, so I was expecting an unconditional error. Looking closer, I realized that cc_platform_has() evaluates to false in that configuration, so the compiler can always turn if (cond || !false) action(); into action(); but it seems like with the GCOV instrumentation, it keeps both branches (since GCOV is about code coverage, it makes sense that you would want to see if each branch is ever taken). I can eliminate the error with the following diff, I am not sure if that is too much though. diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 428ee74002e1..4432ee09cbcb 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -434,7 +434,7 @@ static void __init sev_map_percpu_data(void) { int cpu; - if (cc_vendor != CC_VENDOR_AMD || + if ((IS_ENABLED(CONFIG_ARCH_HAS_CC_PLATFORM) && cc_vendor != CC_VENDOR_AMD) || !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) return; Perhaps another solution would be to just #define cc_vendor (CC_VENDOR_NONE) if CONFIG_ARCH_HAS_CC_PLATFORM is not set, since it can never be changed from the default in arch/x86/coco/core.c. diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h index 6ae2d16a7613..f3909894f82f 100644 --- a/arch/x86/include/asm/coco.h +++ b/arch/x86/include/asm/coco.h @@ -10,13 +10,13 @@ enum cc_vendor { CC_VENDOR_INTEL, }; -extern enum cc_vendor cc_vendor; - #ifdef CONFIG_ARCH_HAS_CC_PLATFORM +extern enum cc_vendor cc_vendor; void cc_set_mask(u64 mask); u64 cc_mkenc(u64 val); u64 cc_mkdec(u64 val); #else +#define cc_vendor (CC_VENDOR_NONE) static inline u64 cc_mkenc(u64 val) { return val; Cheers, Nathan
On Thu, Feb 01, 2024 at 12:38:09PM -0700, Nathan Chancellor wrote: > Perhaps another solution would be to just > > #define cc_vendor (CC_VENDOR_NONE) > > if CONFIG_ARCH_HAS_CC_PLATFORM is not set, since it can never be changed > from the default in arch/x86/coco/core.c. I think this approach is cleaner. Could you post a proper patch? > > diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h > index 6ae2d16a7613..f3909894f82f 100644 > --- a/arch/x86/include/asm/coco.h > +++ b/arch/x86/include/asm/coco.h > @@ -10,13 +10,13 @@ enum cc_vendor { > CC_VENDOR_INTEL, > }; > > -extern enum cc_vendor cc_vendor; > - > #ifdef CONFIG_ARCH_HAS_CC_PLATFORM > +extern enum cc_vendor cc_vendor; > void cc_set_mask(u64 mask); > u64 cc_mkenc(u64 val); > u64 cc_mkdec(u64 val); > #else > +#define cc_vendor (CC_VENDOR_NONE) > static inline u64 cc_mkenc(u64 val) > { > return val; > > Cheers, > Nathan >
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index dfe9945b9bec..428ee74002e1 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -434,7 +434,8 @@ static void __init sev_map_percpu_data(void) { int cpu; - if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) + if (cc_vendor != CC_VENDOR_AMD || + !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) return; for_each_possible_cpu(cpu) {