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

[Huang, Kai]

On Thu, 2023-06-15 at 11:12 +0300, Nikolay Borisov wrote:
> 
> 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."
> 

OK will do.  Thanks.

> 
> 
> >   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:

(to your another reply too)

I noticed this too when preparing this series.  In old versions the TDMRs were
always freed no matter module initialization result, thus it's not good to do
what you suggested.  Now we can do what you suggested (assuming we don't change
back to always freeing TDMRs), I just wasn't so sure when I was preparing this
series.

I'll take a look at the yielding patches with your suggestion.  Thanks.

Hi Kirill/Dave,

Since I have received couple of tags from you, may I know which way do you
prefer?

> 
> 
> 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>