Re: [PATCH v11 15/20] x86/virt/tdx: Configure global KeyID on all packages

[Nikolay Borisov]

On 4.06.23 г. 17:27 ч., Kai Huang wrote:
> After the list of TDMRs and the global KeyID are configured to the TDX
> module, the kernel needs to configure the key of the global KeyID on all
> packages using TDH.SYS.KEY.CONFIG.
>
> This SEAMCALL cannot run parallel on different cpus.  Loop all online
> cpus and use smp_call_on_cpu() to call this SEAMCALL on the first cpu of
> each package.
>
> To keep things simple, this implementation takes no affirmative steps to
> online cpus to make sure there's at least one cpu for each package.  The
> callers (aka. KVM) can ensure success by ensuring that.

The last sentence is a bit hard to read due to the multiple use of 
ensure/ensuring. OTOH I find the comment in the code somewhat more 
coherent:

> + * This code takes no affirmative steps to online CPUs.  Callers (aka.
> + * KVM) can ensure success by ensuring sufficient CPUs are online for
> + * this to succeed.
> + */

I'd suggest you just use those words. Or just saying "Callers (such as 
KVM) can ensure success by onlining at least 1 CPU per package."

<snip>


>   static int init_tdx_module(void)
>   {
>   	static DECLARE_PADDED_STRUCT(tdsysinfo_struct, tdsysinfo,
> @@ -980,15 +1073,47 @@ static int init_tdx_module(void)
>   	if (ret)
>   		goto out_free_pamts;
>   
> +	/*
> +	 * Hardware doesn't guarantee cache coherency across different
> +	 * KeyIDs.  The kernel needs to flush PAMT's dirty cachelines
> +	 * (associated with KeyID 0) before the TDX module can use the
> +	 * global KeyID to access the PAMT.  Given PAMTs are potentially
> +	 * large (~1/256th of system RAM), just use WBINVD on all cpus
> +	 * to flush the cache.
> +	 */
> +	wbinvd_on_all_cpus();
> +
> +	/* Config the key of global KeyID on all packages */
> +	ret = config_global_keyid();
> +	if (ret)
> +		goto out_reset_pamts;
> +
>   	/*
>   	 * TODO:
>   	 *
> -	 *  - Configure the global KeyID on all packages.
>   	 *  - Initialize all TDMRs.
>   	 *
>   	 *  Return error before all steps are done.
>   	 */
>   	ret = -EINVAL;
> +out_reset_pamts:
> +	if (ret) {

Again with those conditionals in the error paths. Just copy the 
put_mem_online(); ret 0. and this will decrease the indentation level 
and make the code linear. Here;s what the diff looks like:


diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 4aa41352edfc..49fda2a28f24 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -1131,6 +1131,8 @@ static int init_tdx_module(void)
        if (ret)
                goto out_free_pamts;

+       pr_info("%lu KBs allocated for PAMT.\n",
+               tdmrs_count_pamt_pages(&tdx_tdmr_list) * 4);
        /*
         * Hardware doesn't guarantee cache coherency across different
         * KeyIDs.  The kernel needs to flush PAMT's dirty cachelines
@@ -1148,36 +1150,32 @@ static int init_tdx_module(void)

        /* Initialize TDMRs to complete the TDX module initialization */
        ret = init_tdmrs(&tdx_tdmr_list);
+
+       put_online_mems();
+
+       return 0;
 out_reset_pamts:
-       if (ret) {
-               /*
-                * Part of PAMTs may already have been initialized by the
-                * TDX module.  Flush cache before returning PAMTs back
-                * to the kernel.
-                */
-               wbinvd_on_all_cpus();
-               /*
-                * According to the TDX hardware spec, if the platform
-                * doesn't have the "partial write machine check"
-                * erratum, any kernel read/write will never cause #MC
-                * in kernel space, thus it's OK to not convert PAMTs
-                * back to normal.  But do the conversion anyway here
-                * as suggested by the TDX spec.
-                */
-               tdmrs_reset_pamt_all(&tdx_tdmr_list);
-       }
+       /*
+        * Part of PAMTs may already have been initialized by the
+        * TDX module.  Flush cache before returning PAMTs back
+        * to the kernel.
+        */
+       wbinvd_on_all_cpus();
+       /*
+        * According to the TDX hardware spec, if the platform
+        * doesn't have the "partial write machine check"
+        * erratum, any kernel read/write will never cause #MC
+        * in kernel space, thus it's OK to not convert PAMTs
+        * back to normal.  But do the conversion anyway here
+        * as suggested by the TDX spec.
+        */
+       tdmrs_reset_pamt_all(&tdx_tdmr_list);
 out_free_pamts:
-       if (ret)
-               tdmrs_free_pamt_all(&tdx_tdmr_list);
-       else
-               pr_info("%lu KBs allocated for PAMT.\n",
-                               tdmrs_count_pamt_pages(&tdx_tdmr_list) * 4);
+       tdmrs_free_pamt_all(&tdx_tdmr_list);
 out_free_tdmrs:
-       if (ret)
-               free_tdmr_list(&tdx_tdmr_list);
+       free_tdmr_list(&tdx_tdmr_list);
 out_free_tdx_mem:
-       if (ret)
-               free_tdx_memlist(&tdx_memlist);
+       free_tdx_memlist(&tdx_memlist);
 out:
        /*
         * @tdx_memlist is written here and read at memory hotplug time.


<snip>