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

Huang, Kai
2025-10-01T10:40:59Z

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.