Re: [PATCH v2 11/31] x86/virt/tdx: Make TDX Module initialize Extensions

[Xu Yilun]

> >  static int tdx_ext_mem_add(struct tdx_page_array *ext_mem)
> >  {
> >  	struct tdx_module_args args = {
> > @@ -1572,6 +1589,17 @@ static int __maybe_unused init_tdx_ext(void)
> >  	if (!(tdx_sysinfo.features.tdx_features0 & TDX_FEATURES0_EXT))
> >  		return 0;
> >  
> > +	/*
> > +	 * With this errata, TDX should use movdir64b to clear private pages
> > +	 * when reclaiming them. See tdx_quirk_reset_paddr().
> > +	 *
> > +	 * Don't expect this errata on any TDX Extensions supported platform.
> > +	 * All features require TDX Extensions (including TDX Extensions
> > +	 * itself) will never call tdx_quirk_reset_paddr().
> > +	 */
> > +	if (boot_cpu_has_bug(X86_BUG_TDX_PW_MCE))
> > +		return -ENXIO;
> 
> I don't know if we are going to want to sprinkle these over every new feature
> until the end of time. If this feature will only show up on the platforms with
> this erratum, then I say we just drop the check.

Make sense.

> 
> > +
> >  	nr_pages = tdx_sysinfo.ext.memory_pool_required_pages;
> >  	/*
> >  	 * memory_pool_required_pages == 0 means no need to add more pages,
> > @@ -1587,6 +1615,20 @@ static int __maybe_unused init_tdx_ext(void)
> >  			goto out_ext_mem;
> >  	}
> >  
> > +	/*
> > +	 * ext_required == 0 means no need to call TDH.EXT.INIT, the Extensions
> > +	 * are already working.
> 
> How does this scenario happen exactly? And why not check it above at the
> beginning? Before the allocation, so it doesn't need to free.
> 
> Is there a scenario where the memory needs to be given, but the extension is
> already inited?

mm.. you are right. It leads to something absurd.

I checked with TDX Module team again. The correct understanding is:

 - TDX_FEATURES0_EXT bit shows Extensions is supported.
 - optional feature bits are selected on TDH_SYS_CONFIG
 - If one of the optional feature (e.g. TDX CONNECT) requires Extention,
   memory_pool_required_pages > 0 && ext_required == 1. Otherwise no
   need to initialize Extension.

So yes, I should check memory_pool_required_pages && ext_required at the
beginning.

> 
> > +	 */
> > +	if (tdx_sysinfo.ext.ext_required) {
> > +		ret = tdx_ext_init();
> > +		/*
> > +		 * Some pages may have been touched by the TDX module.
> > +		 * Flush cache before returning these pages to kernel.
> > +		 */
> > +		if (ret)
> > +			goto out_flush;
> > +	}
> > +
> >  	/* Extension memory is never reclaimed once assigned */
> >  	tdx_page_array_ctrl_leak(ext_mem);
> >  
> > @@ -1595,6 +1637,9 @@ static int __maybe_unused init_tdx_ext(void)
> >  
> >  	return 0;
> >  
> > +out_flush:
> > +	if (ext_mem)
> 
> For the error path we don't need to be efficient. But also why does it assume
> tdx_ext_init() can touch the pages, but tdx_ext_mem_add() can't?

The tdx_ext_mem_add() only collects memory, tdx_ext_init() does the
actual initialization for Extensions and touches the memory. But the
detail of when touching the pages is not specified in SPEC, do you think
host doesn't have to tell the difference, just flush when any one of
ext-SEAMCALLs is called?

> 
> 
> > +		wbinvd_on_all_cpus();
> >  out_ext_mem:
> >  	tdx_page_array_free(ext_mem);
> >  
>