lkml.org 
[lkml]   [2021]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 04/11] x86: Introduce generic protected guest abstraction
    From
    Date


    On 6/28/21 10:52 AM, Tom Lendacky wrote:
    > On 6/18/21 5:57 PM, Kuppuswamy Sathyanarayanan wrote:
    >> Add a generic way to check if we run with an encrypted guest,
    >> without requiring x86 specific ifdefs. This can then be used in
    >> non architecture specific code.
    >>
    >> prot_guest_has() is used to check for protected guest feature
    >> flags.
    >>
    >> Originally-by: Andi Kleen <ak@linux.intel.com>
    >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
    >> ---
    >>
    >> Change since v1:
    >> * Introduced PR_GUEST_TDX and PR_GUEST_SEV vendor flags as per
    >> Boris suggestion.
    >> * Replaced is_tdx_guest() with if (boot_cpu_data.x86_vendor ==
    >> X86_VENDOR_INTEL) in prot_guest_has().
    >> * Modified tdx_protected_guest_has() and sev_protected_guest_has()
    >> to support vendor flags.
    >>
    >> arch/Kconfig | 3 +++
    >> arch/x86/Kconfig | 2 ++
    >> arch/x86/include/asm/protected_guest.h | 20 +++++++++++++++++
    >> arch/x86/include/asm/sev.h | 3 +++
    >> arch/x86/include/asm/tdx.h | 4 ++++
    >> arch/x86/kernel/sev.c | 17 +++++++++++++++
    >> arch/x86/kernel/tdx.c | 17 +++++++++++++++
    >> include/linux/protected_guest.h | 30 ++++++++++++++++++++++++++
    >> 8 files changed, 96 insertions(+)
    >> create mode 100644 arch/x86/include/asm/protected_guest.h
    >> create mode 100644 include/linux/protected_guest.h
    >>

    >> +static inline bool prot_guest_has(unsigned long flag)
    >> +{
    >> + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
    >> + return tdx_protected_guest_has(flag);
    >> + else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
    >> + return sev_protected_guest_has(flag);
    >
    > So as I think about this, I don't think this will work if the hypervisor
    > decides to change the vendor name, right?

    For TDX guest, vendor name cannot be changed. It is set by TDX module and
    it is fixed as per TDX module spec.

    >
    > And doesn't TDX supply "IntelTDX " as a signature. I don't see where
    > the signature is used to set the CPU vendor to X86_VENDOR_INTEL.

    We don't need to specially handle it for TDX. Generic early_identify_cpu() will
    set boot_cpu_data.x86_vendor as X86_VENDOR_INTEL for TDX guest. I think it is
    based on Intel in vendor string.

    >
    > The current SEV checks to set sev_status, which is used by sme_active(),
    > sev_active, etc.) are based on the max leaf and CPUID bits, but not a
    > CPUID vendor check.
    >

    You also set x86_vendor id as AMD based on SEV checks?

    > So maybe we can keep the prot_guest_has() but I think it will have to be a
    > common routine, with a "switch" statement that has supporting case element
    > that check for "sev_active() || static_cpu_has(X86_FEATURE_TDX_GUEST)", etc.

    >> }
    >> +
    >> +bool sev_protected_guest_has(unsigned long flag)
    >> +{
    >> + switch (flag) {
    >> + case PR_GUEST_MEM_ENCRYPT:
    >> + case PR_GUEST_MEM_ENCRYPT_ACTIVE:
    >> + case PR_GUEST_UNROLL_STRING_IO:
    >> + case PR_GUEST_HOST_MEM_ENCRYPT:
    >> + return true;
    >
    > This will need to be fixed up because this function will be called for
    > baremetal and legacy guests and those properties aren't true for those
    > situations. Something like (although I'm unsure of the difference between
    > PR_GUEST_MEM_ENCRYPT and PR_GUEST_MEM_ENCRYPT_ACTIVE):

    MEM_ENCRYPT_ACTIVE is suggested for mem_encrypt_active() case (I think it
    means some sort of encryption is active).

    PR_GUEST_MEM_ENCRYPT means guest supports memory encryption (sev_active()
    case).

    Yes, I can include following changes in next version.

    >
    > case PR_GUEST_MEM_ENCRYPT:
    > case PR_GUEST_MEM_ENCRYPT_ACTIVE:
    > return sev_active();
    > case PR_GUEST_UNROLL_STRING_IO:
    > return sev_active() && !sev_es_active();
    > case PR_GUEST_HOST_MEM_ENCRYPT:
    > return sme_active();
    >
    > But you (or I) would have to audit all of the locations where
    > mem_encrypt_active(), sme_active(), sev_active() and sev_es_active() are
    > used, to be sure the right thing is being done. And for bisectability,
    > that should probably be the first patch if you will be invoking
    > prot_guest_has() in the same location as any of the identified functions.
    >
    > Create the new helper and fixup the locations should be one (or more)
    > patches. Then add the TDX support to the helper function as a follow-on patch.

    Can you submit a patch to replace all existing uses cases of mem_encrypt_active()
    ,sme_active(), sev_active() and sev_es_active() with prot_guest_has() calls? Since
    I cannot test any of these changes for AMD, it would be better if you could do it.

    Once you submit a tested version, I can enable these features for TDX and test
    and submit it separately.

    This patch can be split as below:

    1. x86: Introduce generic protected guest abstraction patch (with below changes).
    - Remove all PR_GUEST flags in sev_protected_guest_has() and
    tdx_protected_guest_has().
    2. Patch from you to use prot_guest_has() for AMD code and enable relevant
    PR_GUEST flags in sev_protected_guest_has().
    3. Patch from me to us prot_guest_has() for TDX cases and enable relevant
    PR_GUEST flags in tdx_protected_guest_has().

    Agree?


    >> diff --git a/include/linux/protected_guest.h b/include/linux/protected_guest.h
    >> new file mode 100644
    >> index 000000000000..c5b7547e5a68
    >> --- /dev/null
    >> +++ b/include/linux/protected_guest.h
    >> @@ -0,0 +1,30 @@
    >> +/* SPDX-License-Identifier: GPL-2.0-only */
    >> +#ifndef _LINUX_PROTECTED_GUEST_H
    >> +#define _LINUX_PROTECTED_GUEST_H 1
    >> +
    >> +/* Protected Guest Feature Flags (leave 0-0xfff for vendor specific flags) */
    >> +
    >> +/* 0-ff is reserved for Intel specific flags */
    >> +#define PR_GUEST_TDX 0x0000
    >> +
    >> +/* 100-1ff is reserved for AMD specific flags */
    >> +#define PR_GUEST_SEV 0x0100
    >> +
    >> +/* Support for guest encryption */
    >> +#define PR_GUEST_MEM_ENCRYPT 0x1000
    >
    > I'm not sure I follow the difference between this and
    > PR_GUEST_MEM_ENCRYPT_ACTIVE. Is this saying that the host has support for
    > starting guests that support memory encryption or the guest has support
    > for memory encryption but it hasn't been activated yet (which doesn't seem
    > possible)?

    Explained it above.

    >
    > Thanks,
    > Tom
    >

    --
    Sathyanarayanan Kuppuswamy
    Linux Kernel Developer

    \
     
     \ /
      Last update: 2021-06-28 21:16    [W:4.781 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site