lkml.org 
[lkml]   [2022]   [Feb]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.

    \
     
     \ /
      Last update: 2022-02-18 21:39    [W:3.691 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site