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

[Dave Hansen <dave.hansen@intel.com>]

On 6/27/25 04:27, Kirill A. Shutemov wrote:
> On Wed, Jun 25, 2025 at 12:26:09PM -0700, Dave Hansen wrote:
>>> +static atomic_t *tdx_get_pamt_refcount(unsigned long hpa)
>>> +{
>>> +	return &pamt_refcounts[hpa / PMD_SIZE];
>>> +}
>>
>> "get refcount" usually means "get a reference". This is looking up the
>> location of the refcount.
>>
>> I think this needs a better name.
> 
> tdx_get_pamt_ref_ptr()?

How about:

	tdx_find_pamt_refcount()

>>> +	unsigned long vaddr;
>>> +	pte_t entry;
>>> +
>>> +	if (!pte_none(ptep_get(pte)))
>>> +		return 0;
>>
>> This ^ is an optimization, right? Could it be comment appropriately, please?
> 
> Not optimization.
> 
> Calls of apply_to_page_range() can overlap by one page due to
> round_up()/round_down() in alloc_pamt_refcount(). We don't need to
> populate these pages again if they are already populated.
> 
> Will add a comment.

But don't you check it again under the lock?

>>> +	vaddr = __get_free_page(GFP_KERNEL | __GFP_ZERO);
>>> +	if (!vaddr)
>>> +		return -ENOMEM;
>>> +
>>> +	entry = pfn_pte(PFN_DOWN(__pa(vaddr)), PAGE_KERNEL);
>>> +
>>> +	spin_lock(&init_mm.page_table_lock);
>>> +	if (pte_none(ptep_get(pte)))

Right there ^

>>> +		set_pte_at(&init_mm, addr, pte, entry);
>>> +	else
>>> +		free_page(vaddr);
>>> +	spin_unlock(&init_mm.page_table_lock);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int pamt_refcount_depopulate(pte_t *pte, unsigned long addr,
>>> +				    void *data)
>>> +{
>>> +	unsigned long vaddr;
>>> +
>>> +	vaddr = (unsigned long)__va(PFN_PHYS(pte_pfn(ptep_get(pte))));
>>
>> Gah, we really need a kpte_to_vaddr() helper here. This is really ugly.
>> How many of these are in the tree?
> 
> I only found such chain in KASAN code.
> 
> What about this?
> 
>       pte_t entry = ptep_get(pte);
>       struct page *page = pte_page(entry);
> 
> and use __free_page(page) instead free_page(vaddr)?
> 
> The similar thing can be don on allocation side.

That does look better.