Re: [PATCH v3 01/24] x86/tdx: Enhance tdh_mem_page_aug() to support huge pages

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

On 1/7/26 01:12, Yan Zhao wrote:
...
> However, my understanding is that it's better for functions expecting huge pages
> to explicitly receive "folio" instead of "page". This way, people can tell from
> a function's declaration what the function expects. Is this understanding
> correct?

In a perfect world, maybe.

But, in practice, a 'struct page' can still represent huge pages and
*does* represent huge pages all over the kernel. There's no need to cram
a folio in here just because a huge page is involved.

> Passing "start_idx" along with "folio" is due to the requirement of mapping only
> a sub-range of a huge folio. e.g., we allow creating a 2MB mapping starting from
> the nth idx of a 1GB folio.
> 
> On the other hand, if we instead pass "page" to tdh_mem_page_aug() for huge
> pages and have tdh_mem_page_aug() internally convert it to "folio" and
> "start_idx", it makes me wonder if we could have previously just passed "pfn" to
> tdh_mem_page_aug() and had tdh_mem_page_aug() convert it to "page".

As a general pattern, I discourage folks from using pfns and physical
addresses when passing around references to physical memory. They have
zero type safety.

It's also not just about type safety. A 'struct page' also *means*
something. It means that the kernel is, on some level, aware of and
managing that memory. It's not MMIO. It doesn't represent the physical
address of the APIC page. It's not SGX memory. It doesn't have a
Shared/Private bit.

All of those properties are important and they're *GONE* if you use a
pfn. It's even worse if you use a raw physical address.

Please don't go back to raw integers (pfns or paddrs).

>>> -	tdx_clflush_page(page);
>>> +	if (start_idx + npages > folio_nr_pages(folio))
>>> +		return TDX_OPERAND_INVALID;
>>
>> Why is this necessary? Would it be a bug if this happens?
> This sanity check is due to the requirement in KVM that mapping size should be
> no larger than the backend folio size, which ensures the mapping pages are
> physically contiguous with homogeneous page attributes. (See the discussion
> about "EPT mapping size and folio size" in thread [1]).
> 
> Failure of the sanity check could only be due to bugs in the caller (KVM). I
> didn't convert the sanity check to an assertion because there's already a
> TDX_BUG_ON_2() on error following the invocation of tdh_mem_page_aug() in KVM.

We generally don't protect against bugs in callers. Otherwise, we'd have
a trillion NULL checks in every function in the kernel.

The only reason to add caller sanity checks is to make things easier to
debug, and those almost always include some kind of spew:
WARN_ON_ONCE(), pr_warn(), etc...

>>> +	for (int i = 0; i < npages; i++)
>>> +		tdx_clflush_page(folio_page(folio, start_idx + i));
>>
>> All of the page<->folio conversions are kinda hurting my brain. I think
>> we need to decide what the canonical type for these things is in TDX, do
>> the conversion once, and stick with it.
> Got it!
> 
> Since passing in base "page" or base "pfn" may still require the
> wrappers/helpers to internally convert them to "folio" for sanity checks, could
> we decide that "folio" and "start_idx" are the canonical params for functions
> expecting huge pages? Or do you prefer KVM to do the sanity check by itself?

I'm not convinced the sanity check is a good idea in the first place. It
just adds complexity.