Re: [PATCH v2 05/31] x86/virt/tdx: Extend tdx_page_array to support IOMMU_MT

[Dan Williams]

Xu Yilun wrote:
[..]
> > 
> > I'm drafting some changes and make the tdx_page_array look like:
> > 
> >   struct tdx_page_array {
> > 	/* public: */
> > 	unsigned int nr_pages;
> > 	struct page **pages;
> > 
> > 	/* private: */
> > 	u64 *root;
> > 	bool flush_on_free;

How about "need_phymem_page_wbinvd"?

That makes it a bit more greppable and not to be confused with other
flushing.

[..]
> Hi, I end up made the following changes on top of this series:
> 
> -------8<--------
> 
>  arch/x86/include/asm/tdx.h            |  32 +-
>  arch/x86/virt/vmx/tdx/tdx.c           | 561 ++++++++------------------
>  drivers/virt/coco/tdx-host/tdx-host.c | 179 ++++++--
>  3 files changed, 316 insertions(+), 456 deletions(-)
> 
> +		ret = tdx_ext_mem_setup(nr_pages, &ext_mem);
>  		if (ret)
> +			return ret;
>  	}
>  
> +	ret = tdx_ext_init();
> +	if (ret)
> +		goto out_remove_ext_mem;
> +
>  	/*
> +	 * Extensions memory is never reclaimed once assigned, stop tracking it
> +	 * and free the tracking structures.
>  	 */
> +	tdx_page_array_free(ext_mem.chunk);

Wait, these pages belong to the module now, they can't be freed, or I am
missing something?

> +	kfree(ext_mem.pages);

Releasing this makes sense.

>  
>  	pr_info("%lu KB allocated for TDX Module Extensions\n",
>  		nr_pages * PAGE_SIZE / 1024);
>  
>  	return 0;
>  
> -out_flush:
> -	if (ext_mem)
> +out_remove_ext_mem:
> +	if (nr_pages) {
> +		/*
> +		 * TDH.EXT.MEM.ADD only collects required memory. TDX.EXT.INIT
> +		 * does the actual initialization so if it fails some pages may
> +		 * have been touched by the TDX module, flush cache before
> +		 * returning these pages to kernel.
> +		 */
>  		wbinvd_on_all_cpus();
> +		tdx_ext_mem_remove(&ext_mem);

This only releases the last populated chunk, not all previous chunks,
right?