Re: [PATCH v12 18/22] x86/virt/tdx: Keep TDMRs when module initialization is successful

[Nikolay Borisov <nik.borisov@suse.com>]

On 26.06.23 г. 17:12 ч., Kai Huang wrote:
> On the platforms with the "partial write machine check" erratum, the
> kexec() needs to convert all TDX private pages back to normal before
> booting to the new kernel.  Otherwise, the new kernel may get unexpected
> machine check.
> 
> There's no existing infrastructure to track TDX private pages.  Change
> to keep TDMRs when module initialization is successful so that they can
> be used to find PAMTs.
> 
> With this change, only put_online_mems() and freeing the buffer of the
> TDSYSINFO_STRUCT and CMR array still need to be done even when module
> initialization is successful.  Adjust the error handling to explicitly
> do them when module initialization is successful and unconditionally
> clean up the rest when initialization fails.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
> 
> v11 -> v12 (new patch):
>    - Defer keeping TDMRs logic to this patch for better review
>    - Improved error handling logic (Nikolay/Kirill in patch 15)
> 
> ---
>   arch/x86/virt/vmx/tdx/tdx.c | 84 ++++++++++++++++++-------------------
>   1 file changed, 42 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 52b7267ea226..85b24b2e9417 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -49,6 +49,8 @@ static DEFINE_MUTEX(tdx_module_lock);
>   /* All TDX-usable memory regions.  Protected by mem_hotplug_lock. */
>   static LIST_HEAD(tdx_memlist);
>   
> +static struct tdmr_info_list tdx_tdmr_list;
> +
>   /*
>    * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
>    * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
> @@ -1047,7 +1049,6 @@ static int init_tdmrs(struct tdmr_info_list *tdmr_list)
>   static int init_tdx_module(void)
>   {
>   	struct tdsysinfo_struct *sysinfo;
> -	struct tdmr_info_list tdmr_list;
>   	struct cmr_info *cmr_array;
>   	int ret;
>   
> @@ -1088,17 +1089,17 @@ static int init_tdx_module(void)
>   		goto out_put_tdxmem;
>   
>   	/* Allocate enough space for constructing TDMRs */
> -	ret = alloc_tdmr_list(&tdmr_list, sysinfo);
> +	ret = alloc_tdmr_list(&tdx_tdmr_list, sysinfo);
>   	if (ret)
>   		goto out_free_tdxmem;
>   
>   	/* Cover all TDX-usable memory regions in TDMRs */
> -	ret = construct_tdmrs(&tdx_memlist, &tdmr_list, sysinfo);
> +	ret = construct_tdmrs(&tdx_memlist, &tdx_tdmr_list, sysinfo);

nit: Does it make sense to keep passing those global variables are 
function parameters? Since those functions are static it's unlikely that 
they are going to be used with any other parameter so might as well use 
the parameter directly. It makes the code somewhat easier to follow.

<snip>