Messages in this thread | | | Date | Fri, 18 Feb 2022 12:36:02 -0800 | From | Dave Hansen <> | Subject | Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask |
| |
On 2/18/22 08:16, Kirill A. Shutemov wrote: > +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM > +u64 cc_get_mask(bool enc); > +u64 cc_mkenc(u64 val); > +u64 cc_mkdec(u64 val); > +#else > +#define cc_get_mask(enc) 0 > +#define cc_mkenc(val) (val) > +#define cc_mkdec(val) (val) > +#endif
Is there a reason the stubs as #defines? Static inlines are preferred for consistent type safety among other things.
It would also be nice to talk about the u64 type in the changelog. If I remember right, there was a reason you didn't want to have a pgprot_t here.
... > @@ -74,12 +72,52 @@ 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);
This patch is bordering on doing too many different things. Adding the CC_VENDOR_*'s in a separate patch probably would have been nice, for instance.
This wasn't really broached at all in the changelog.
> +u64 cc_get_mask(bool enc) > +{ > + switch (cc_vendor) { > + case CC_VENDOR_AMD: > + return enc ? cc_mask : 0; > + default: > + return 0; > + } > +}
I had to ask myself why you need all three of these functions. It's obvious _after_ reading the whole patch, but it left me wanting more in the changelog about it.
> +u64 cc_mkenc(u64 val) > +{ > + switch (cc_vendor) { > + case CC_VENDOR_AMD: > + return val | cc_mask; > + default: > + return val; > + } > +} > + > +u64 cc_mkdec(u64 val) > +{ > + switch (cc_vendor) { > + case CC_VENDOR_AMD: > + return val & ~cc_mask; > + default: > + return val; > + } > +} > +EXPORT_SYMBOL_GPL(cc_mkdec); > + > +__init void cc_init(enum cc_vendor vendor, u64 mask) > +{ > + cc_vendor = vendor; > + cc_mask = mask; > +} > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c > index 5a99f993e639..9af6be143998 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, 0); > } > > 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..fa758247ab57 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, sme_me_mask); > }
I don't think you need to mop it up here, but where does this leave sme_me_mask?
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c > index b4072115c8ef..e79366b8a9da 100644 > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -1999,8 +1999,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc) > memset(&cpa, 0, sizeof(cpa)); > cpa.vaddr = &addr; > cpa.numpages = numpages; > - cpa.mask_set = enc ? __pgprot(_PAGE_ENC) : __pgprot(0); > - cpa.mask_clr = enc ? __pgprot(0) : __pgprot(_PAGE_ENC); > + cpa.mask_set = __pgprot(cc_get_mask(enc)); > + cpa.mask_clr = __pgprot(cc_get_mask(!enc)); > cpa.pgd = init_mm.pgd; > > /* Must avoid aliasing mappings in the highmem code */
This actually ended up looking pretty nice. It was a real mess along the way, but this makes me think this is a _good_ solution.
| |