Re: [PATCH v10 02/16] x86/virt/tdx: Detect TDX during kernel boot

[David Hildenbrand <david@redhat.com>]

On 16.03.23 23:37, Huang, Kai wrote:
> On Thu, 2023-03-16 at 13:48 +0100, David Hildenbrand wrote:
>> On 06.03.23 15:13, Kai Huang wrote:
>>> Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
>>> host and certain physical attacks.  A CPU-attested software module
>>> called 'the TDX module' runs inside a new isolated memory range as a
>>> trusted hypervisor to manage and run protected VMs.
>>>
>>> Pre-TDX Intel hardware has support for a memory encryption architecture
>>> called MKTME.  The memory encryption hardware underpinning MKTME is also
>>> used for Intel TDX.  TDX ends up "stealing" some of the physical address
>>> space from the MKTME architecture for crypto-protection to VMs.  The
>>> BIOS is responsible for partitioning the "KeyID" space between legacy
>>> MKTME and TDX.  The KeyIDs reserved for TDX are called 'TDX private
>>> KeyIDs' or 'TDX KeyIDs' for short.
>>>
>>> TDX doesn't trust the BIOS.  During machine boot, TDX verifies the TDX
>>> private KeyIDs are consistently and correctly programmed by the BIOS
>>> across all CPU packages before it enables TDX on any CPU core.  A valid
>>> TDX private KeyID range on BSP indicates TDX has been enabled by the
>>> BIOS, otherwise the BIOS is buggy.
>>

Sorry for the late reply!

>> So we don't trust the BIOS, but trust the BIOS that it won't hot-remove
>> physical memory or hotplug physical CPUS (if I understood the cover
>> letter correctly)? :)
> 
> The "trust" in this context means security, but not functionality.  BIOS needs
> to do the right thing in order to make things work correctly in terms of
> functionality.
> 
> For physical memory hotplug or CPU hotplug, we don't have patch to _explicitly_
> distinguish them (from logical memory hotplug and logical cpu online/offline),
> but actually they are kinda also handled:  For memory hotplug, and hot-added
> memory is rejected to go online (because they cannot be in TDX's convertible
> memory ranges).  For CPU hotplug, we have a function to do per-cpu
> initialization (tdx_cpu_enable() in patch 5), and it will return error for hot-
> added physical cpu.

Make sense, thanks!

> 
>>
>>>
>>> The TDX module is expected to be loaded by the BIOS when it enables TDX,
>>> but the kernel needs to properly initialize it before it can be used to
>>> create and run any TDX guests.  The TDX module will be initialized by
>>> the KVM subsystem when KVM wants to use TDX.
>>>
>>> Add a new early_initcall(tdx_init) to detect the TDX by detecting TDX
>>> private KeyIDs.  Also add a function to report whether TDX is enabled by
>>> the BIOS.  Similar to AMD SME, kexec() will use it to determine whether
>>> cache flush is needed.
>>>
>>> The TDX module itself requires one TDX KeyID as the 'TDX global KeyID'
>>> to protect its metadata.  Each TDX guest also needs a TDX KeyID for its
>>> own protection.  Just use the first TDX KeyID as the global KeyID and
>>> leave the rest for TDX guests.  If no TDX KeyID is left for TDX guests,
>>> disable TDX as initializing the TDX module alone is useless.
>>
>> Does that really happen in practice that we care about that at all?
>> Seems weird and rather like a broken firmware or sth like that ...
> 
> No it doesn't happen in practice, because the BIOS is sane enough.
> 
> But since the public spec doesn't explicitly say it is guaranteed this doesn't
> happen when TDX is enabled, I just added this sanity check.

Okay!

> 
>>
>>>
>>> To start to support TDX, create a new arch/x86/virt/vmx/tdx/tdx.c for
>>> TDX host kernel support.  Add a new Kconfig option CONFIG_INTEL_TDX_HOST
>>> to opt-in TDX host kernel support (to distinguish with TDX guest kernel
>>> support).  So far only KVM uses TDX.  Make the new config option depend
>>> on KVM_INTEL.
>>>
>>> Signed-off-by: Kai Huang <kai.huang@intel.com>
>>> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>
>>
>> [...]
>>
>>> ---
>>>    arch/x86/Kconfig                 |  12 ++++
>>>    arch/x86/Makefile                |   2 +
>>>    arch/x86/include/asm/msr-index.h |   3 +
>>>    arch/x86/include/asm/tdx.h       |   7 +++
>>>    arch/x86/virt/Makefile           |   2 +
>>>    arch/x86/virt/vmx/Makefile       |   2 +
>>>    arch/x86/virt/vmx/tdx/Makefile   |   2 +
>>>    arch/x86/virt/vmx/tdx/tdx.c      | 105 +++++++++++++++++++++++++++++++
>>>    8 files changed, 135 insertions(+)
>>>    create mode 100644 arch/x86/virt/Makefile
>>>    create mode 100644 arch/x86/virt/vmx/Makefile
>>>    create mode 100644 arch/x86/virt/vmx/tdx/Makefile
>>>    create mode 100644 arch/x86/virt/vmx/tdx/tdx.c
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index 3604074a878b..fc010973a6ff 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -1952,6 +1952,18 @@ config X86_SGX
>>>    
>>>    	  If unsure, say N.
>>>    
>>> +config INTEL_TDX_HOST
>>> +	bool "Intel Trust Domain Extensions (TDX) host support"
>>> +	depends on CPU_SUP_INTEL
>>> +	depends on X86_64
>>> +	depends on KVM_INTEL
>>> +	help
>>> +	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
>>> +	  host and certain physical attacks.  This option enables necessary TDX
>>> +	  support in host kernel to run protected VMs.
>>
>> s/in host/in the host/ ?
> 
> Sure.
> 
>>
>> Also, is "protected VMs" the right term to use here? "Encrypted VMs",
>> "Confidential VMs" ... ?
> 
> "Encrypted VM" perhaps is not a good choice, because there are more things than
> encryption.  I am also OK with "Confidential VMs", but "protected VMs" is also
> used in the KVM series (not upstreamed yet), and also used by s390 by looking at
> the git log.
> 
> So both "protected VM" and "confidential VM" work for me.
> 
> Not sure anyone else wants to comment?

I'm fine as long as it's used consistently. "Protected VM" would have 
been the one out of the 3 alternatives that I have heard least frequently.

> 
>>
> [...]
> 
>>> +static u32 tdx_global_keyid __ro_after_init;
>>> +static u32 tdx_guest_keyid_start __ro_after_init;
>>> +static u32 tdx_nr_guest_keyids __ro_after_init;
>>> +
>>> +/*
>>> + * Use tdx_global_keyid to indicate that TDX is uninitialized.
>>> + * This is used in TDX initialization error paths to take it from
>>> + * initialized -> uninitialized.
>>> + */
>>> +static void __init clear_tdx(void)
>>> +{
>>> +	tdx_global_keyid = 0;
>>> +}
>>
>> Why not set "tdx_global_keyid" last, such that you don't have to clear
>> when anything goes wrong before that? Seems more straight forward.
> 
> My thinking was by reserving the global keyid and taking it out first, I can
> check the remaining keyids for TDX guests easily:
> 
> 
> +	if (!nr_tdx_keyids) {
> +		pr_info("initialization failed: too few private KeyIDs
> available.\n");
> +		goto no_tdx;
> +	}
> 
> Otherwise need to do:
> 
> 	if (nr_tdx_keyids < 2) {
> 		...
> 	}
> 
> Also, in the later patch to handle memory hotplug we will add an additional step
> to register_memory_notifier() which can also fail, so I just introduced
> clear_tdx() here.
> 
> But nothing is big deal, and yes we can set the global keyid at last and remove
> clear_tdx().

Good, that simplifies things, thanks!

-- 
Thanks,

David / dhildenb