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

[kirill.shutemov@xxxxxxxxxxxxxxx]

On Thu, Jun 26, 2025 at 12:53:29AM +0000, Huang, Kai wrote:
> 
> > +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()

Ack.

> 
> > +	 */
> > +	area = get_vm_area(size, VM_IOREMAP);
> 
> I am not sure why VM_IOREMAP is used? 

It follows vmap_pfn() pattern as usage is similar.

It seems the flag allows vread_iter() to work correct on sparse mappings.

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

Okay.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov