Re: [PATCH v3 06/16] x86/virt/tdx: Improve PAMT refcounters allocation for sparse memory

[Huang, Kai]

On Wed, 2025-10-01 at 00:32 +0000, Edgecombe, Rick P wrote:
> On Wed, 2025-09-24 at 16:57 +0800, Binbin Wu wrote:
> > > This will results in @start pointing to the first refcount and @end
> > > pointing to the second, IIUC.
> > > 
> > > So it seems we need:
> > > 
> > >       start = (unsigned long)tdx_find_pamt_refcount(PFN_PHYS(start_pfn));
> > >       end   = (unsigned long)tdx_find_pamt_refcount(PFN_PHYS(end_pfn) - 1));
> > >       start = round_down(start, PAGE_SIZE);
> > >       end   = round_up(end, PAGE_SIZE);
> > 
> > Checked again, this seems to be the right version.
> 
> Thanks both for the analysis. I lazily created a test program to check some edge
> cases and found the original and this version were both buggy. Clearly this code
> needs to be clearer (as actually Dave pointed out in v2 and I failed to
> address). Example (synthetic failure):
> 
> start_pfn = 0x80000
> end_pfn =   0x80001
> 
> Original code: start = 0xff76ba4f9e034000
>                end   = 0xff76ba4f9e034000
> 
> Above fix:     start = 0xff76ba4f9e034000
>                end   = 0xff76ba4f9e034000

Oh I think the problem of the "Above fix" is it fails when @start and @end
are the same and both are already page aligned.

> 
> Part of the problem is that tdx_find_pamt_refcount() expects the hpa passed in
> to be PMD aligned. The other callers of tdx_find_pamt_refcount() also make sure
> that the PA passed in is 2MB aligned before calling, and compute this starting
> with a PFN. So to try to make it easier to read and be correct what do you think
> about the below:
> 
> static atomic_t *tdx_find_pamt_refcount(unsigned long pfn) {
>     unsigned long hpa = ALIGN_DOWN(pfn, PMD_SIZE);

Shouldn't it be:

	hpa = ALIGN_DOWN(PFN_PHYS(pfn), PMD_SIZE));
?
> 
>     return &pamt_refcounts[hpa / PMD_SIZE];
> }
> 
> /*
>  * 'start_pfn' is inclusive and 'end_pfn' is exclusive. 
> 

I think 'end_pfn' is exclusive is a little bit confusing?  It sounds like
the physical range from PFN_PHYS(end_pfn - 1) to PFN_PHYS(end_pfn) is also
exclusive, but it is actually not?  To me it's more like only the physical
address PFN_PHYS(end_pfn) is exclusive.

> Compute the 
>  * page range to be inclusive of the start and end refcount
>  * addresses and at least a page in size. The teardown logic needs
>  * to handle potentially overlapping refcounts mappings resulting
>  * from this.
>  */
> start = (unsigned long)tdx_find_pamt_refcount(start_pfn);
> end   = (unsigned long)tdx_find_pamt_refcount(end_pfn - 1);
> start = ALIGN_DOWN(start, PAGE_SIZE);
> end   = ALIGN_DOWN(end, PAGE_SIZE) + PAGE_SIZE;

This looks fine to me.  I mean the result should be correct, but the
'end_pfn - 1' (due to 'end_pfn' is exclusive) is a bit confusing to me as
said above, but maybe it's only me, so feel free to ignore.

Or, as said above, I think the problem of the "Above fix" is when
calculating the @end we didn't consider the case where it equals to @start
and is already page aligned.  Does below work (assuming
tdx_find_pamt_refcount() still takes 'hpa')?

    start = (unsigned long)tdx_find_pamt_refcount(PFN_PHYS(start_pfn));
    end   = (unsigned long)tdx_find_pamt_refcount(PFN_PHYS(end_pfn) - 1));
    start = PAGE_ALIGN_DOWN(start);
    end   = PAGE_ALIGN_DOWN(end) + PAGE_SIZE;

Anyway, don't have strong opinion here, so up to you.