Re: [PATCHv3.1 2/32] x86/coco: Explicitly declare type of confidential computing platform

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

On Mon, Feb 21, 2022 at 12:07:15PM +0100, Borislav Petkov wrote:
> On Sat, Feb 19, 2022 at 03:13:04AM +0300, Kirill A. Shutemov wrote:
> > diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
> > index 6a6ffcd978f6..891d3074a16e 100644
> > --- a/arch/x86/kernel/cc_platform.c
> > +++ b/arch/x86/kernel/cc_platform.c
> > @@ -9,18 +9,15 @@
> >  
> >  #include <linux/export.h>
> >  #include <linux/cc_platform.h>
> > -#include <linux/mem_encrypt.h>
> >  
> > -#include <asm/mshyperv.h>
> > +#include <asm/coco.h>
> >  #include <asm/processor.h>
> >  
> > -static bool __maybe_unused intel_cc_platform_has(enum cc_attr attr)
> > +static enum cc_vendor cc_vendor;
> 
> static enum cc_vendor vendor __ro_after_init;

Hm. Isn't 'vendor' too generic? It may lead to name conflict in the
future.

What is wrong with cc_vendor here? I noticed that you don't like name of
a variable to match type name. Why?

> > @@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
> >  		 */
> >  		swiotlb_force = SWIOTLB_FORCE;
> >  #endif
> > +		if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> > +			cc_init(CC_VENDOR_HYPERV);
> 
> Isn't that supposed to test HV_ISOLATION_TYPE_SNP instead?

Currently cc_platform_has() relies on hv_is_isolation_supported() which
checks for !HV_ISOLATION_TYPE_NONE. This is direct transfer to the new
scheme. It might be wrong, but it is not regression.

> I mean, I have no clue what HV_ISOLATION_TYPE_VBS is. It is not used
> anywhere in the tree either.
> 
> a6c76bb08dc7 ("x86/hyperv: Load/save the Isolation Configuration leaf")
> calls it "'VBS' (software-based isolation)" - whatever that means - so
> I'm not sure that is going to need the cc-facilities.
> 
> For stuff like that you need to use get_maintainers.pl and Cc them
> folks:
> 
> $ git log -p -1 | ./scripts/get_maintainer.pl | grep -i hyper
> "K. Y. Srinivasan" <kys@microsoft.com> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Haiyang Zhang <haiyangz@microsoft.com> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Stephen Hemminger <sthemmin@microsoft.com> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Wei Liu <wei.liu@kernel.org> (supporter:Hyper-V/Azure CORE AND DRIVERS,commit_signer:1/4=25%)
> Dexuan Cui <decui@microsoft.com> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> linux-hyperv@vger.kernel.org (open list:Hyper-V/Azure CORE AND DRIVERS)
> 
> /me adds the ML to Cc.

+Tianyu, who brought HyperV cc_platform_has().

Speaking about HyperV, moving to scheme with cc_init() revealed that
HyperV never selected ARCH_HAS_CC_PLATFORM. Now it leads to build failure
if AMD memory encryption is not enabled:

	ld: arch/x86/kernel/cpu/mshyperv.o: in function `ms_hyperv_init_platform':
	mshyperv.c:(.init.text+0x297): undefined reference to `cc_init'

Maybe something like this:

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 0747a8f1fcee..574ea80601e9 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -8,6 +8,7 @@ config HYPERV
 		|| (ARM64 && !CPU_BIG_ENDIAN))
 	select PARAVIRT
 	select X86_HV_CALLBACK_VECTOR if X86
+	select ARCH_HAS_CC_PLATFORM if X86
 	select VMAP_PFN
 	help
 	  Select this option to run Linux as a Hyper-V client operating

Again, it is pre-existing issue. It only escalated to build failure.

> >  	if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) {
> > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > index 3f0abb403340..eb7fbd85b77e 100644
> > --- a/arch/x86/mm/mem_encrypt_identity.c
> > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > @@ -44,6 +44,7 @@
> >  #include <asm/setup.h>
> >  #include <asm/sections.h>
> >  #include <asm/cmdline.h>
> > +#include <asm/coco.h>
> >  
> >  #include "mm_internal.h"
> >  
> > @@ -565,8 +566,7 @@ void __init sme_enable(struct boot_params *bp)
> >  	} else {
> >  		/* SEV state cannot be controlled by a command line option */
> >  		sme_me_mask = me_mask;
> > -		physical_mask &= ~sme_me_mask;
> > -		return;
> > +		goto out;
> >  	}
> >  
> >  	/*
> > @@ -600,6 +600,8 @@ void __init sme_enable(struct boot_params *bp)
> >  		sme_me_mask = 0;
> >  	else
> >  		sme_me_mask = active_by_default ? me_mask : 0;
> > -
> > +out:
> >  	physical_mask &= ~sme_me_mask;
> > +	if (sme_me_mask)
> > +		cc_init(CC_VENDOR_AMD);
> >  }
> 
> I guess.
> 
> Adding SEV folks to Cc too.
> 
> Please use get_maintainer.pl - you should know that - you're not some
> newbie who started doing kernel work two weeks ago.

Sorry, will do.

-- 
 Kirill A. Shutemov