[PATCH] x86/coco: Define cc_vendor without CONFIG_ARCH_HAS_CC_PLATFORM

[PATCH] x86/coco: Define cc_vendor without CONFIG_ARCH_HAS_CC_PLATFORM

[Nathan Chancellor]

After commit a9ef277488cf ("x86/kvm: Fix SEV check in
sev_map_percpu_data()"), there is a build error when building
x86_64_defconfig with GCOV using LLVM:

  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

which corresponds to

  if (cc_vendor != CC_VENDOR_AMD ||
      !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
            return;

Without GCOV, clang is able to eliminate the use of cc_vendor because
cc_platform_has() evaluates to false when CONFIG_ARCH_HAS_CC_PLATFORM is
not set, meaning that if statement will be true no matter what value
cc_vendor has.

With GCOV, the instrumentation keeps the use of cc_vendor around for
code coverage purposes but cc_vendor is only declared, not defined,
without CONFIG_ARCH_HAS_CC_PLATFORM, leading to the build error above.

Provide a macro definition of cc_vendor when CONFIG_ARCH_HAS_CC_PLATFORM
is not set with a value of CC_VENDOR_NONE, so that the first condition
can always be evaluated/eliminated at compile time, avoiding the build
error altogether. This is very similar to the situation prior to
commit da86eb961184 ("x86/coco: Get rid of accessor functions").

Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
Commit a9ef277488cf ("x86/kvm: Fix SEV check in sev_map_percpu_data()")
exposes this build error but I think it is really a problem with commit
da86eb961184 ("x86/coco: Get rid of accessor functions"), although I am
not positive so I left out the fixes tag. It would be nice if this could
go along with KVM tree that has that change but it is obviously well
within -tip's territory so I will just aim it at both parties and let
them figure it out :)
---
 arch/x86/include/asm/coco.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index 6ae2d16a7613..76c310b19b11 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -10,13 +10,14 @@ 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;

---
base-commit: a9ef277488cfc1b7da88235dc11c338a14f34835
change-id: 20240202-provide-cc_vendor-without-arch_has_cc_platform-325a3fae8550

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] x86/coco: Define cc_vendor without CONFIG_ARCH_HAS_CC_PLATFORM

[Borislav Petkov]

On Fri, Feb 02, 2024 at 04:53:21PM -0700, Nathan Chancellor wrote:
> Commit a9ef277488cf ("x86/kvm: Fix SEV check in sev_map_percpu_data()")
> exposes this build error but I think it is really a problem with commit
> da86eb961184 ("x86/coco: Get rid of accessor functions"), although I am
> not positive so I left out the fixes tag.

Well, which is it?

If you're running those GCOV LLVM tests regularly and you haven't seen
it after da86eb961184, then it cannot be that one, can it?

In any case, you can simply revert a9ef277488cf and see if it fires.

> It would be nice if this could go along with KVM tree that has that
> change but it is obviously well within -tip's territory so I will just
> aim it at both parties and let them figure it out :)

The answer to the above question would determine the proper Fixes tag
and who picks it up.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/coco: Define cc_vendor without CONFIG_ARCH_HAS_CC_PLATFORM

[Nathan Chancellor]

On Sat, Feb 03, 2024 at 11:29:25AM +0100, Borislav Petkov wrote:
> On Fri, Feb 02, 2024 at 04:53:21PM -0700, Nathan Chancellor wrote:
> > Commit a9ef277488cf ("x86/kvm: Fix SEV check in sev_map_percpu_data()")
> > exposes this build error but I think it is really a problem with commit
> > da86eb961184 ("x86/coco: Get rid of accessor functions"), although I am
> > not positive so I left out the fixes tag.
> 
> Well, which is it?

Perhaps I should have expanded more on this in the commit message or
trailer.

> If you're running those GCOV LLVM tests regularly and you haven't seen
> it after da86eb961184, then it cannot be that one, can it?

Well the issue is that at da86eb961184, all uses of cc_vendor is in code
that is guarded by either CONFIG_AMD_MEM_ENCRYPT or
CONFIG_INTEL_TDX_GUEST, which both select CONFIG_ARCH_HAS_CC_PLATFORM,
so this build error cannot happen at that revision.

$ git grep cc_vendor da86eb961184
da86eb961184:arch/x86/coco/core.c:enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
da86eb961184:arch/x86/coco/core.c:        switch (cc_vendor) {
da86eb961184:arch/x86/coco/core.c:        switch (cc_vendor) {
da86eb961184:arch/x86/coco/core.c:        switch (cc_vendor) {
da86eb961184:arch/x86/coco/tdx/tdx.c:        cc_vendor = CC_VENDOR_INTEL;
da86eb961184:arch/x86/hyperv/ivm.c:        cc_vendor = CC_VENDOR_AMD;
da86eb961184:arch/x86/include/asm/coco.h:enum cc_vendor {
da86eb961184:arch/x86/include/asm/coco.h:extern enum cc_vendor cc_vendor;
da86eb961184:arch/x86/include/asm/sev.h:        if (cc_vendor == CC_VENDOR_AMD &&
da86eb961184:arch/x86/include/asm/sev.h:        if (cc_vendor == CC_VENDOR_AMD &&
da86eb961184:arch/x86/include/asm/sev.h:        if (cc_vendor == CC_VENDOR_AMD &&
da86eb961184:arch/x86/mm/mem_encrypt_identity.c:                cc_vendor = CC_VENDOR_AMD;

However, is it really a9ef277488cf's fault that it happened to use
cc_vendor in generic code where those same conditions may or may not
satisfied? If it had used cc_get_vendor() instead if da86eb961184 had
not existed, this issue would not have happened.

I have no issues with blaming a9ef277488cf but I think da86eb961184 is
equally blamable for removing the option to use cc_vendor in generic x86
code where CONFIG_ARCH_HAS_CC_PLATFORM may not be set. Hopefully that at
least carifies the "which is it?" question, I'll do whatever you think
is best.

Cheers,
Nathan

Re: [PATCH] x86/coco: Define cc_vendor without CONFIG_ARCH_HAS_CC_PLATFORM

[Borislav Petkov]

On Sat, Feb 03, 2024 at 09:08:06AM -0700, Nathan Chancellor wrote:
> I have no issues with blaming a9ef277488cf but I think da86eb961184 is
> equally blamable for removing the option to use cc_vendor in generic x86
> code where CONFIG_ARCH_HAS_CC_PLATFORM may not be set. Hopefully that at
> least carifies the "which is it?" question, I'll do whatever you think
> is best.

I guess I wasn't clear enough, sorry about that. Of the two, that one
should be in Fixes which is the first one which causes the build issue
so that the fix can be backported to the respective kernels.

IOW, if you can't trigger with da86eb961184, then a9ef277488cf should be
in Fixes and your fix should go through the KVM tree, along with
a9ef277488cf.

How does that sound?

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/coco: Define cc_vendor without CONFIG_ARCH_HAS_CC_PLATFORM

[Nathan Chancellor]

On Sat, Feb 03, 2024 at 08:07:29PM +0100, Borislav Petkov wrote:
> On Sat, Feb 03, 2024 at 09:08:06AM -0700, Nathan Chancellor wrote:
> > I have no issues with blaming a9ef277488cf but I think da86eb961184 is
> > equally blamable for removing the option to use cc_vendor in generic x86
> > code where CONFIG_ARCH_HAS_CC_PLATFORM may not be set. Hopefully that at
> > least carifies the "which is it?" question, I'll do whatever you think
> > is best.
> 
> I guess I wasn't clear enough, sorry about that. Of the two, that one

Guess that makes both of us :)

> should be in Fixes which is the first one which causes the build issue
> so that the fix can be backported to the respective kernels.
> 
> IOW, if you can't trigger with da86eb961184, then a9ef277488cf should be
> in Fixes and your fix should go through the KVM tree, along with
> a9ef277488cf.
> 
> How does that sound?

Yeah, that seems like a fair plan to me. I was a little concerned about
a future change that would require backporting to kernels that have
da86eb961184 (i.e., 6.6) that do not have a9ef277488cf and miss this fix
but that is a bridge that can be crossed if it ever appears, no point in
thinking too hard about it at this point.

I can send a v2 on Monday, unless Paolo wants to just add

Fixes: a9ef277488cf ("x86/kvm: Fix SEV check in sev_map_percpu_data()")

directly during application. I think the rest of the patch is fine but
if there are any other changes that should be made, I am more than happy
do to so.

Cheers,
Nathan

Re: [PATCH] x86/coco: Define cc_vendor without CONFIG_ARCH_HAS_CC_PLATFORM

[Borislav Petkov]

On Sat, Feb 03, 2024 at 12:35:52PM -0700, Nathan Chancellor wrote:
> Yeah, that seems like a fair plan to me. I was a little concerned about
> a future change that would require backporting to kernels that have
> da86eb961184 (i.e., 6.6) that do not have a9ef277488cf and miss this fix
> but that is a bridge that can be crossed if it ever appears, no point in
> thinking too hard about it at this point.

Yep.

> I can send a v2 on Monday, unless Paolo wants to just add
> 
> Fixes: a9ef277488cf ("x86/kvm: Fix SEV check in sev_map_percpu_data()")
> 
> directly during application. I think the rest of the patch is fine but
> if there are any other changes that should be made, I am more than happy
> do to so.

Nah, LGTM.

Acked-by: Borislav Petkov (AMD) <bp@alien8.de>

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/coco: Define cc_vendor without CONFIG_ARCH_HAS_CC_PLATFORM

[Paolo Bonzini]

Queued, thanks.

Paolo