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:0.100 / U:0.480 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site