Re: [PATCH] x86/virt/tdx: accurately distinguishes TDX module loading situations

["Huang, Kai" <kai.huang@intel.com>]

On Sun, 2024-06-23 at 01:50 +0800, Jun Miao wrote:
> The first SEAMCALL is important to response the state of TDX Module/BIOS.
> 
> In actual incorrect BIOS setup or deployment, the sysinit_ret will be
> -EOPNOTSUPP. But the message "module not loaded" isn`t enough to describe
> the accurate loading situation when module loaded but SEAMCALL failed
> for some BIOS wrong setting. So add the error return operation code number.

(firstly, please CC me in the future, and please also CC x86 maintainers,
x86@kernel.org, and linux-kernel@vger.kernel.org). 

So this is the case where TDX is detected as enabled by the kernel (TDX KeyID
reports valid info), but TDX actually isn't enabled by the BIOS.

This seems BIOS bug to me.  The BIOS should never report valid TDX KeyID if TDX
isn't enabled.  I guess it happens on development machines, but I am not sure
whether the production machine would fix this.

Anyway ...

> 
> Signed-off-by: Jun Miao <jun.miao@intel.com>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 4e2b2e2ac9f9..787dfaf44036 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -122,10 +122,14 @@ static int try_init_module_global(void)
>  	/*
>  	 * The first SEAMCALL also detects the TDX module, thus
>  	 * it can fail due to the TDX module is not loaded.
> -	 * Dump message to let the user know.
> +	 * Dump more detailed message to let the user know.
>  	 */
>  	if (sysinit_ret == -ENODEV)
>  		pr_err("module not loaded\n");
> +	else if (sysinit_ret)
> +		pr_warn("module loaded error ret=%d\n",sysinit_ret);
> +	else
> +		pr_info("module loaded\n");
>  
>  	sysinit_done = true;
>  out:

... printing the error code won't help user a lot since it is not obvious to the
user.  I think we can do something below.

Hi Dave/Kirill,

Any comments?

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 49a1c6890b55..a8c273cfe17e 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -119,13 +119,35 @@ static int try_init_module_global(void)
        args.rcx = 0;
        sysinit_ret = seamcall_prerr(TDH_SYS_INIT, &args);
 
-       /*
-        * The first SEAMCALL also detects the TDX module, thus
-        * it can fail due to the TDX module is not loaded.
-        * Dump message to let the user know.
-        */
-       if (sysinit_ret == -ENODEV)
+       switch (sysinit_ret) {
+       case -ENODEV:
+               /*
+                * The first SEAMCALL also detects the TDX module,
+                * thus it can fail due to the TDX module is not
+                * loaded.  Dump message to let the user know.
+                */
                pr_err("module not loaded\n");
+               break;
+       case -EOPNOTSUPP:
+               /*
+                * Some TDX-capable development machines may report
+                * valid TDX KeyID when TDX is actually not enabled
+                * by the BIOS (e.g., due to BIOS misconfiguration).
+                * Let the user know.
+                */
+               pr_err("[BIOS bug]: TDX isn't enabled.\n");
+               break;
+       case -EACCES:
+               /*
+                * -EACCES happens when this is called when CPU is
+                * not in post-VMXON state.  It's kernel issue, so
+                * don't dump error message.
+                */
+               break;
+       default:
+               /* Actual SEAMCALL failure, message already printed */
+               break;
+       }
 
        sysinit_done = true;
 out: