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@xxxxxxxxxxxxx> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Stephen Hemminger <sthemmin@xxxxxxxxxxxxx> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Wei Liu <wei.liu@xxxxxxxxxx> (supporter:Hyper-V/Azure CORE AND DRIVERS,commit_signer:1/4=25%)
> Dexuan Cui <decui@xxxxxxxxxxxxx> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> linux-hyperv@xxxxxxxxxxxxxxx (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
