Message-ID: <9db9a874e5b7ff62e9cb419f51279047438c30ec.camel@intel.com>
Date:   Fri, 6 Oct 2023 09:48:55 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "Sun, Yi" <yi.sun@...el.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "x86@...nel.org" <x86@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
CC:     "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Su, Heng" <heng.su@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Yan, Dongcheng" <dongcheng.yan@...el.com>,
        "yi.sun@...ux.intel.com" <yi.sun@...ux.intel.com>
Subject: Re: [PATCH v4] x86/tdx: Dump TDX version During the TD Bootup

On Fri, 2023-10-06 at 14:28 +0800, Yi Sun wrote:
> It is essential for TD users to be aware of the vendor and version of
> the current TDX. Additionally, they can reference the TDX version when
> reporting bugs or issues.
> 
> Furthermore, the applications or device drivers running in TD can achieve
> enhanced reliability and flexibility by following the TDX Module ABI
> specification, because there are significant differences between different
> versions of TDX, as mentioned in the "Intel® TDX Module Incompatibilities
> between v1.0 and v1.5" reference. Here are a few examples:
> 
> MSR Name		Index		Reason
> ----------------------------------------------
> IA32_UARCH_MISC_CTL	0x1B01		From v1.5
> IA32_ARCH_CAPABILITIES	0x010A		Changed in v1.5
> IA32_TSX_CTRL		0x0122		Changed in v1.5
> 
> CPUID Leaf	Sub-leaf	Reason
> ---------------------------------------
> 0x7		2		From v1.5
> 0x22		0		From v1.5
> 0x23		0~3		From v1.5
> 0x80000007	0		From v1.5

I am not sure whether you want to list these details.  People cannot understand
what do they mean immediately anyway (well at least me).

The justification w/o them seems already good enough.

> 
> During TD initialization, the TDX version info can be obtained by calling
> TDG.SYS.RD. This will fetch the current version of TDX, including the major
> and minor version numbers and vendor ID.

Although already mentioned in the patch title, perhaps still better to add one
sentence to say what this patch does.

> 
> The TDCALL TDG.SYS.RD originates from TDX version 1.5. If the error
> TDCALL_INVALID_OPERAND occurs, it should be treated as TDX version 1.0.
> 
> If the __tdcall_ret fails, expect a zero value for all tdx sys info.

__tdcall_ret -> TDCALL

And I don't quite understand "expect a zero value for all TDX sys info" mean.

Are they used somewhere else besides being printed during early boot?

> No additional error code is necessary to avoid introducing noise during
> the bootup.

Perhaps better to print some error message?

Given this patch only prints module version, I think you can just remove this
paragraph (as it is a detail can be seen in the patch).

> 
> Co-developed-by: Dongcheng Yan <dongcheng.yan@...el.com>
> Signed-off-by: Dongcheng Yan <dongcheng.yan@...el.com>
> Signed-off-by: Yi Sun <yi.sun@...el.com>

It's better if you can maintain the code change history among different versions
so that people can see what changes have been done:

	<your SoB>
	---
	code change history 
	---
	...

The "code change history" will be thrown away when the patch is applied.

> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 3e6dbd2199cf..991f7dc695bd 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -37,6 +37,24 @@
>  
>  #define TDREPORT_SUBTYPE_0	0
>  
> +/*
> + * TDX metadata base field id, used by TDCALL TDG.SYS.RD
> + * See TDX ABI Spec section 3.3.2.3 Global Metadata Fields

I believe I was told we don't need to mention the specific section number (well
at least in the changelog, can't recall whether applies to comment, though),
because they can be changed (especially for TDX).

> + */
> +#define TDX_SYS_VENDOR_ID_FID		0x0800000200000000ULL
> +#define TDX_SYS_MINOR_FID		0x0800000100000003ULL
> +#define TDX_SYS_MAJOR_FID		0x0800000100000004ULL
> +#define TDX_VENDOR_INTEL		0x8086
> +
> +/*
> + * The global-scope metadata field via TDG.SYS.RD TDCALL
> + */
> +struct tdg_sys_info {
> +	u32 vendor_id;
> +	u16 major_version;
> +	u16 minor_version;
> +};
> +
>  /* Called from __tdx_hypercall() for unrecoverable failure */
>  noinstr void __noreturn __tdx_hypercall_failed(void)
>  {
> @@ -800,6 +818,60 @@ static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
>  	return true;
>  }
>  
> +/*
> + * Parse the tdx module version info from the global-scope metadata fields.
> + *
> + * Refer to Intel TDX Application Binary Interface (ABI) section
> + * "TDG.SYS.RD".

I think you don't need to mention TDG.SYS.RD here, as you will mention it couple
of times later in the function anyway.

> + */
> +static void tdg_get_sysinfo(struct tdg_sys_info *td_sys)
> +{
> +	u64 ret;
> +	struct tdx_module_args args = {
> +		.rdx = TDX_SYS_VENDOR_ID_FID,
> +	};

Please keep these variables in reverse christmas tree style.

> +
> +	if (!td_sys)
> +		return;

If you print the module info in this function rather than in tdx_early_init(),
you can declare @td_sys as local variable in this function and remove this
check.

You don't even need 'struct tdg_sys_info', but have 3 local variables instead:
vendor_id, major_version, minor_version.

In fact, perhaps you don't even need vendor_id, because it must be Intel, but
printing it won't hurt I suppose.

> +
> +	memset(td_sys, 0, sizeof(struct tdg_sys_info));

This can be removed too.  If needed, major_version and minor_version can be
initialized when they are declared.

> +
> +	/*
> +	 * TDCALL leaf TDX_SYS_RD
> +	 * Input Field Identifier via RDX and get the output via R8.
> +	 */

If you need to say "Input Field Identifier via RDX", then I guess it's better to
explicitly initialize args.rdx here.  It is explicitly initialized below for
major_version and minor_version anyway.

> +	ret = __tdcall_ret(TDX_SYS_RD, &args);
> +	/*
> +	 * The TDCALL TDG.SYS.RD originates from TDX version 1.5.
> +	 * Treat TDCALL_INVALID_OPERAND error as TDX version 1.0.
> +	 * If other errors occur, return with zero td_sys.
> +	 */
> +	if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> +		goto version_1_0;
> +	else if (ret)
> +		return;

Looks you don't need the 'else'.

And as Nikolay mentioned, it's better to print some error message here to say
unable to get module version.  Maybe even WARN() because AFAICT it can only be
kernel bug or TDX firmware bug.

> +
> +	td_sys->vendor_id = (u32)args.r8;
> +
> +	args.rdx = TDX_SYS_MAJOR_FID;
> +	__tdcall_ret(TDX_SYS_RD, &args);
> +
> +	td_sys->major_version = (u16)args.r8;
> +
> +	args.rdx = TDX_SYS_MINOR_FID;
> +	__tdcall_ret(TDX_SYS_RD, &args);
> +
> +	td_sys->minor_version = (u16)args.r8;
> +
> +	return;
> +
> +	/* TDX 1.0 does not have the TDCALL TDG.SYS.RD */
> +version_1_0:
> +	td_sys->vendor_id = TDX_VENDOR_INTEL;
> +	td_sys->major_version = 1;
> +	td_sys->minor_version = 0;

So the main reason that I think you can just print module version in this
function instead of tdx_early_init() is because IIUC you are setting up some
random value for TDX module 1.0.  Probably it's better to just print it's module
1.0 w/o mentioning major/minor.

> +}
> +
>  void __init tdx_early_init(void)
>  {
>  	struct tdx_module_args args = {
> @@ -808,6 +880,7 @@ void __init tdx_early_init(void)
>  	};
>  	u64 cc_mask;
>  	u32 eax, sig[3];
> +	struct tdg_sys_info td_sys_info;
>  
>  	cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2],  &sig[1]);
>  
> @@ -867,5 +940,9 @@ void __init tdx_early_init(void)
>  	 */
>  	x86_cpuinit.parallel_bringup = false;
>  
> -	pr_info("Guest detected\n");
> +	tdg_get_sysinfo(&td_sys_info);
> +
> +	pr_info("Guest detected. TDX version:%u.%u VendorID: %x\n",
> +		td_sys_info.major_version, td_sys_info.minor_version,
> +		td_sys_info.vendor_id);
>  }

As replied above, I think you can print module info in tdg_get_sysinfo().

If you do so, perhaps to rename the function to something like
detect_tdx_version().

You can either still keep

	pr_info("Guest detected\n");

here, or remove it but embed it to the message when you print module version.


> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index f74695dea217..d326509832e6 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -17,6 +17,7 @@
>  #define TDG_MR_REPORT			4
>  #define TDG_MEM_PAGE_ACCEPT		6
>  #define TDG_VM_WR			8
> +#define TDX_SYS_RD			11

TDG_SYS_RD, please.

Powered by blists - more mailing lists