Re: [PATCHv2 03/12] x86/virt/tdx: Allocate reference counters for PAMT memory

["Huang, Kai" <kai.huang@intel.com>]

> +static int init_pamt_metadata(void)
> +{
> +	size_t size = max_pfn / PTRS_PER_PTE * sizeof(*pamt_refcounts);
> +	struct vm_struct *area;
> +
> +	if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> +		return 0;
> +
> +	/*
> +	 * Reserve vmalloc range for PAMT reference counters. It covers all
> +	 * physical address space up to max_pfn. It is going to be populated
> +	 * from init_tdmr() only for present memory that available for TDX use.
		^
		build_tdx_memlist()

> +	 */
> +	area = get_vm_area(size, VM_IOREMAP);

I am not sure why VM_IOREMAP is used?  Or should we just use vmalloc()?

> +	if (!area)
> +		return -ENOMEM;
> +
> +	pamt_refcounts = area->addr;
> +	return 0;
> +}
> +
> +static void free_pamt_metadata(void)
> +{
> +	size_t size = max_pfn / PTRS_PER_PTE * sizeof(*pamt_refcounts);
> +
> +	if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> +		return;
> +
> +	size = round_up(size, PAGE_SIZE);
> +	apply_to_existing_page_range(&init_mm,
> +				     (unsigned long)pamt_refcounts,
> +				     size, pamt_refcount_depopulate,
> +				     NULL);
> +	vfree(pamt_refcounts);
> +	pamt_refcounts = NULL;
> +}
> +
>  /*
>   * Add a memory region as a TDX memory block.  The caller must make sure
>   * all memory regions are added in address ascending order and don't
> @@ -248,6 +347,10 @@ static int build_tdx_memlist(struct list_head *tmb_list)
>  		ret = add_tdx_memblock(tmb_list, start_pfn, end_pfn, nid);
>  		if (ret)
>  			goto err;
> +
> +		ret = alloc_pamt_refcount(start_pfn, end_pfn);
> +		if (ret)
> +			goto err;

So this would goto the error path, which only calls free_tdx_memlist(),
which frees all existing TDX memory blocks that have already created.

Logically, it would be great to also free PAMT refcount pages too, but they
all can be freed at free_pamt_metadata() eventually, so it's OK.

But I think it would still be helpful to put a comment before
free_tdx_memlist() in the error path to call out.  Something like:

err:
	/*
	 * This only frees all TDX memory blocks that have been created.
	 * All PAMT refcount pages will be freed when init_tdx_module() 
	 * calls free_pamt_metadata() eventually.
	 */
	free_tdx_memlist(tmb_list);
	return ret;

> 
>  
>  	return 0;
> @@ -1110,10 +1213,15 @@ static int init_tdx_module(void)
>  	 */
>  	get_online_mems();
>  
> -	ret = build_tdx_memlist(&tdx_memlist);
> +	/* Reserve vmalloc range for PAMT reference counters */
> +	ret = init_pamt_metadata();
>  	if (ret)
>  		goto out_put_tdxmem;
>  
> +	ret = build_tdx_memlist(&tdx_memlist);
> +	if (ret)
> +		goto err_free_pamt_metadata;
> +
>  	/* Allocate enough space for constructing TDMRs */
>  	ret = alloc_tdmr_list(&tdx_tdmr_list, &tdx_sysinfo.tdmr);
>  	if (ret)
> @@ -1171,6 +1279,8 @@ static int init_tdx_module(void)
>  	free_tdmr_list(&tdx_tdmr_list);
>  err_free_tdxmem:
>  	free_tdx_memlist(&tdx_memlist);
> +err_free_pamt_metadata:
> +	free_pamt_metadata();
>  	goto out_put_tdxmem;
>  }
>