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

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

[Borislav Petkov]

On Sat, Feb 19, 2022 at 03:13:04AM +0300, Kirill A. Shutemov wrote:
> Kernel derives type of confidential computing platform from sme_me_mask
> value and hv_is_isolation_supported(). This detection process will be
> more complicated as more platforms get added.
> 
> Declare confidential computing vendor explicitly via cc_init().
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/include/asm/coco.h        | 16 ++++++++++++++++
>  arch/x86/kernel/cc_platform.c      | 29 +++++++++++++++++------------
>  arch/x86/kernel/cpu/mshyperv.c     |  3 +++
>  arch/x86/mm/mem_encrypt_identity.c |  8 +++++---
>  4 files changed, 41 insertions(+), 15 deletions(-)
>  create mode 100644 arch/x86/include/asm/coco.h
> 
> diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
> new file mode 100644
> index 000000000000..6e770e0dd683
> --- /dev/null
> +++ b/arch/x86/include/asm/coco.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_COCO_H
> +#define _ASM_X86_COCO_H
> +
> +#include <asm/pgtable_types.h>
> +
> +enum cc_vendor {
> +	CC_VENDOR_NONE,
> +	CC_VENDOR_AMD,
> +	CC_VENDOR_HYPERV,
> +	CC_VENDOR_INTEL,
> +};
> +
> +void cc_init(enum cc_vendor);
> +
> +#endif /* _ASM_X86_COCO_H */
> 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;

> +
> +static bool intel_cc_platform_has(enum cc_attr attr)
>  {
> -#ifdef CONFIG_INTEL_TDX_GUEST
> -	return false;
> -#else
>  	return false;
> -#endif
>  }
>  
>  /*
> @@ -74,12 +71,20 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)
>  
>  bool cc_platform_has(enum cc_attr attr)
>  {
> -	if (sme_me_mask)
> +	switch (cc_vendor) {
> +	case CC_VENDOR_AMD:
>  		return amd_cc_platform_has(attr);
> -
> -	if (hv_is_isolation_supported())
> +	case CC_VENDOR_INTEL:
> +		return intel_cc_platform_has(attr);
> +	case CC_VENDOR_HYPERV:
>  		return hyperv_cc_platform_has(attr);
> -
> -	return false;
> +	default:
> +		return false;
> +	}
>  }
>  EXPORT_SYMBOL_GPL(cc_platform_has);
> +
> +__init void cc_init(enum cc_vendor vendor)
> +{
> +	cc_vendor = vendor;
> +}
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 5a99f993e639..d77cf3a31f07 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -33,6 +33,7 @@
>  #include <asm/nmi.h>
>  #include <clocksource/hyperv_timer.h>
>  #include <asm/numa.h>
> +#include <asm/coco.h>
>  
>  /* Is Linux running as the root partition? */
>  bool hv_root_partition;
> @@ -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?

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.

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

-- 
Regards/Gruss,
    Boris.

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

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

[Kirill A. Shutemov]

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

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

[Borislav Petkov]

On Mon, Feb 21, 2022 at 02:44:51PM +0300, Kirill A. Shutemov wrote:
> Hm. Isn't 'vendor' too generic? It may lead to name conflict in the
> future.

It's a static variable visible only in this unit.

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

Because when I look at the name I don't know whether it is the type or a
variable of that type. Sure, sure, it depends on the context but let's
make it as non-ambiguous as possible.

> 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 didn't say it is a regression - I'm just wondering why.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Wei Liu]

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/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > index 5a99f993e639..d77cf3a31f07 100644
> > --- a/arch/x86/kernel/cpu/mshyperv.c
> > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > @@ -33,6 +33,7 @@
> >  #include <asm/nmi.h>
> >  #include <clocksource/hyperv_timer.h>
> >  #include <asm/numa.h>
> > +#include <asm/coco.h>
> >  
> >  /* Is Linux running as the root partition? */
> >  bool hv_root_partition;
> > @@ -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?
> 
> 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.
> 

Hi Boris and Kirill, I only see VBS mentioned here so I don't have much
context, but VBS likely means virtualization-based security. There is a
public document for it.

https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/oem-vbs

Whether it needs a new isolation type or not, I am not sure. Perhaps
Tianyu can provide more context.

Thanks,
Wei.

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

[Borislav Petkov]

On Mon, Feb 21, 2022 at 01:52:58PM +0000, Wei Liu wrote:
> Hi Boris and Kirill, I only see VBS mentioned here so I don't have much
> context, but VBS likely means virtualization-based security. There is a
> public document for it.
> 
> https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/oem-vbs
> 
> Whether it needs a new isolation type or not, I am not sure. Perhaps
> Tianyu can provide more context.

Right, this came in with

  c789b90a6904 ("x86/hyper-v: Add hyperv Isolation VM check in the cc_platform_has()")

which says

    Hyper-V provides Isolation VM for confidential computing support and
    guest memory is encrypted in it. Places checking cc_platform_has()
    with GUEST_MEM_ENCRYPT attr should return "True" in Isolation VM.

I'm guessing this was done because you "need to adjust the SWIOTLB size
just like SEV guests."

So my question is, does this VBS thing do guest memory encryption or
does it only use hw virt features?

Because you guys have HV_ISOLATION_TYPE_SNP already. And so, the check

	 hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE;

includes VBS because VBS is only interested in the SWIOTLB buffer size
adjustment and not the rest of the cc_* stuff. Or?

But let's see what Tianyu says.

Thx.

-- 
Regards/Gruss,
    Boris.

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