Re: [PATCHv5 04/30] x86/tdx: Extend the confidential computing API to support TDX guests

[Dave Hansen <dave.hansen@intel.com>]

On 3/2/22 06:27, Kirill A. Shutemov wrote:
...
> Like AMD SME/SEV, TDX uses a bit in the page table entry to indicate
> encryption status of the page, but the polarity of the mask is
> opposite to AMD: if the bit is set the page is accessible to VMM.

I'd much rather this be in a code comment next to the weird-looking code
than in the changelog.

> Details about which bit in the page table entry to be used to indicate
> shared/private state can be determined by using the TDINFO TDCALL.

s/can be/are/

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/Kconfig     |  1 +
>  arch/x86/coco/core.c |  4 ++++
>  arch/x86/coco/tdx.c  | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index c346d66b51fc..93e67842e369 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -884,6 +884,7 @@ config INTEL_TDX_GUEST
>  	bool "Intel TDX (Trust Domain Extensions) - Guest Support"
>  	depends on X86_64 && CPU_SUP_INTEL
>  	depends on X86_X2APIC
> +	select ARCH_HAS_CC_PLATFORM
>  	help
>  	  Support running as a guest under Intel TDX.  Without this support,
>  	  the guest kernel can not boot or run under TDX.
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index fc1365dd927e..9113baebbfd2 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -90,6 +90,8 @@ u64 cc_mkenc(u64 val)
>  	switch (vendor) {
>  	case CC_VENDOR_AMD:
>  		return val | cc_mask;
> +	case CC_VENDOR_INTEL:
> +		return val & ~cc_mask;
>  	default:
>  		return val;
>  	}
> @@ -100,6 +102,8 @@ u64 cc_mkdec(u64 val)
>  	switch (vendor) {
>  	case CC_VENDOR_AMD:
>  		return val & ~cc_mask;
> +	case CC_VENDOR_INTEL:
> +		return val | cc_mask;
>  	default:
>  		return val;
>  	}
> diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
> index 17365fd40ba2..912ef12e434e 100644
> --- a/arch/x86/coco/tdx.c
> +++ b/arch/x86/coco/tdx.c
> @@ -5,8 +5,12 @@
>  #define pr_fmt(fmt)     "tdx: " fmt
>  
>  #include <linux/cpufeature.h>
> +#include <asm/coco.h>
>  #include <asm/tdx.h>
>  
> +/* TDX module Call Leaf IDs */
> +#define TDX_GET_INFO			1
> +
>  /*
>   * Wrapper for standard use of __tdx_hypercall with no output aside from
>   * return code.
> @@ -25,8 +29,32 @@ static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15)
>  	return __tdx_hypercall(&args, 0);
>  }
>  
> +static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> +				   struct tdx_module_output *out)
> +{
> +	if (__tdx_module_call(fn, rcx, rdx, r8, r9, out))
> +		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
> +}

I really think we need to document the panic()s that we add.  It might
mean duplicating a wee bit of the text from the SEAMCALL/TDCALL
assembly, but I think it's worth it so that folks don't think this is an
over-eager panic().