[2/4] x86/virt/tdx: Advertise the CC_ATTR_HOST_MEM_INCOHERENT for TDX host

Message ID ebbc67eb2c6052dd56fda31cd22bb830d3d290ef.1706698706.git.kai.huang@intel.com
State New
Headers
Series TDX host: kexec() support |

Commit Message

Kai Huang Jan. 31, 2024, 11:31 a.m. UTC
  From: Kai Huang <kai.huang@intel.com>

On the TDX capable platform, during kexec() the old kernel needs to
flush dirty cachelines of all TDX private memory otherwise they may
silently corrupt the new kernel's memory.

Advertise the new introduced CC_ATTR_HOST_MEM_INCOHERENT attribute for
TDX host platform so the cache will be flushed during kexec().

Note theoretically cache flush is only needed when TDX module is
initialized, but the module initialization is done at runtime so just
advertise the CC attribute when the platform has TDX enabled.

Signed-off-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reviewed-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/Kconfig            |  1 +
 arch/x86/coco/core.c        | 21 ++++++++++++++++++++-
 arch/x86/virt/vmx/tdx/tdx.c |  3 +++
 include/linux/cc_platform.h |  3 ++-
 4 files changed, 26 insertions(+), 2 deletions(-)
  

Comments

Dave Hansen Jan. 31, 2024, 5:11 p.m. UTC | #1
On 1/31/24 03:31, Huang, Kai wrote:
> From: Kai Huang <kai.huang@intel.com>
> 
> On the TDX capable platform, during kexec() the old kernel needs to
> flush dirty cachelines of all TDX private memory otherwise they may
> silently corrupt the new kernel's memory.
> 
> Advertise the new introduced CC_ATTR_HOST_MEM_INCOHERENT attribute for
> TDX host platform so the cache will be flushed during kexec().

So, you're setting a new bit, CC_ATTR_HOST_MEM_INCOHERENT.  The way I
like to deal with these is to go back and look at the definition of
CC_ATTR_HOST_MEM_INCOHERENT and see whether the changelog here convinces
me that CC_ATTR_HOST_MEM_INCOHERENT is being set appropriately.  Bonus
points if this changelog uses the same nomenclature as the comment
describing CC_ATTR_HOST_MEM_INCOHERENT.

How well does this match the comment above CC_ATTR_HOST_MEM_INCOHERENT?

> Note theoretically cache flush is only needed when TDX module is
> initialized, but the module initialization is done at runtime so just
> advertise the CC attribute when the platform has TDX enabled.

I find this really hard to parse.  Here's a rewrite, as usual:

	A TDX-host-capable system might not actually have any incoherent
	memory.  This can occur if a TDX module was never initialized or
	if the caches have been flushed since the last time TDX was
	used.  Ignore that case.  Eliminate the need for any locking and
	assume that any TDX-host-capable system might have incoherent
	memory by always setting CC_ATTR_HOST_MEM_INCOHERENT.
  
Kai Huang Feb. 1, 2024, 2:42 p.m. UTC | #2
On 1/02/2024 1:11 am, Dave Hansen wrote:
> On 1/31/24 03:31, Huang, Kai wrote:
>> From: Kai Huang <kai.huang@intel.com>
>>
>> On the TDX capable platform, during kexec() the old kernel needs to
>> flush dirty cachelines of all TDX private memory otherwise they may
>> silently corrupt the new kernel's memory.
>>
>> Advertise the new introduced CC_ATTR_HOST_MEM_INCOHERENT attribute for
>> TDX host platform so the cache will be flushed during kexec().
> 
> So, you're setting a new bit, CC_ATTR_HOST_MEM_INCOHERENT.  The way I
> like to deal with these is to go back and look at the definition of
> CC_ATTR_HOST_MEM_INCOHERENT and see whether the changelog here convinces
> me that CC_ATTR_HOST_MEM_INCOHERENT is being set appropriately.  Bonus
> points if this changelog uses the same nomenclature as the comment
> describing CC_ATTR_HOST_MEM_INCOHERENT.
> 
> How well does this match the comment above CC_ATTR_HOST_MEM_INCOHERENT?

I will try to improve the changelog: explain what does 
CC_ATTR_HOST_MEM_INCOHERENT mean, and why it is suitable for TDX host.

Please let me know if you have more comments.  Thanks.

> 
>> Note theoretically cache flush is only needed when TDX module is
>> initialized, but the module initialization is done at runtime so just
>> advertise the CC attribute when the platform has TDX enabled.
> 
> I find this really hard to parse.  Here's a rewrite, as usual:
> 
> 	A TDX-host-capable system might not actually have any incoherent
> 	memory.  This can occur if a TDX module was never initialized or
> 	if the caches have been flushed since the last time TDX was
> 	used.  Ignore that case.  Eliminate the need for any locking and
> 	assume that any TDX-host-capable system might have incoherent
> 	memory by always setting CC_ATTR_HOST_MEM_INCOHERENT.

I appreciate, as usual.  :-)
  

Patch

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 502986237cb6..ac3b32149a77 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1975,6 +1975,7 @@  config INTEL_TDX_HOST
 	depends on CONTIG_ALLOC
 	depends on !KEXEC_CORE
 	depends on X86_MCE
+	select ARCH_HAS_CC_PLATFORM
 	help
 	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
 	  host and certain physical attacks.  This option enables necessary TDX
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index 8d6d727e6e18..ecb15852b69d 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -12,11 +12,12 @@ 
 
 #include <asm/coco.h>
 #include <asm/processor.h>
+#include <asm/cpufeature.h>
 
 enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
 static u64 cc_mask __ro_after_init;
 
-static bool noinstr intel_cc_platform_has(enum cc_attr attr)
+static bool noinstr intel_cc_platform_guest_has(enum cc_attr attr)
 {
 	switch (attr) {
 	case CC_ATTR_GUEST_UNROLL_STRING_IO:
@@ -29,6 +30,24 @@  static bool noinstr intel_cc_platform_has(enum cc_attr attr)
 	}
 }
 
+static bool noinstr intel_cc_platform_host_has(enum cc_attr attr)
+{
+	switch (attr) {
+	case CC_ATTR_HOST_MEM_INCOHERENT:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool noinstr intel_cc_platform_has(enum cc_attr attr)
+{
+	if (boot_cpu_has(X86_FEATURE_TDX_HOST_PLATFORM))
+		return intel_cc_platform_host_has(attr);
+
+	return intel_cc_platform_guest_has(attr);
+}
+
 /*
  * Handle the SEV-SNP vTOM case where sme_me_mask is zero, and
  * the other levels of SME/SEV functionality, including C-bit
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 4d6826a76f78..9f1fed458a32 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -37,6 +37,7 @@ 
 #include <asm/intel-family.h>
 #include <asm/processor.h>
 #include <asm/mce.h>
+#include <asm/coco.h>
 #include "tdx.h"
 
 static u32 tdx_global_keyid __ro_after_init;
@@ -1488,5 +1489,7 @@  void __init tdx_init(void)
 
 	setup_force_cpu_cap(X86_FEATURE_TDX_HOST_PLATFORM);
 
+	cc_vendor = CC_VENDOR_INTEL;
+
 	check_tdx_erratum();
 }
diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
index 2f7273596102..654777d64dc0 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -53,7 +53,8 @@  enum cc_attr {
 	 * Use this in places where the cache coherency of the memory matters
 	 * but the encryption status does not.
 	 *
-	 * Includes all systems that set CC_ATTR_HOST_MEM_ENCRYPT.
+	 * Includes all systems that set CC_ATTR_HOST_MEM_ENCRYPT, but
+	 * additionally adds TDX hosts.
 	 */
 	CC_ATTR_HOST_MEM_INCOHERENT,