Re: [PATCH v6 01/11] x86/virt/tdx: Simplify tdmr_get_pamt_sz()

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

Thanks for the review!

On Tue, 2026-07-07 at 16:22 -0700, Sohil Mehta wrote:
> On 5/25/2026 7:35 PM, Rick Edgecombe wrote:
> > For each memory region that the TDX module might use (called TDMR), three
> > separate traditional PAMT allocations are needed. 
> 
> > One for each supported page size (1GB, 2MB, 4KB).
> Missing verb in this sentence. Maybe use a '-' to merge it with the
> previous sentence.

Hmm, yea I'll update it to:

There is one for each supported page size (1GB, 2MB, 4KB).

> 
> 
> > There are some commonalities in the math needed to calculate the base and
> > size for each smaller allocation, and so an effort was made to share logic
> > across the three. Unfortunately doing this turned out unnaturally tortured,
> > with a loop iterating over the three page sizes, only to call into a
> > function with cases statement for each page size. 
> 
> Did you mean "..with a case statement for each.."?

yep.

> 
> 
> 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 967482ae3c801..487f389f52f4b 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -516,31 +516,21 @@ static __init int fill_out_tdmrs(struct list_head *tmb_list,
> >    * Calculate PAMT size given a TDMR and a page size.  The returned
> >    * PAMT size is always aligned up to 4K page boundary.
> >    */
> > -static __init unsigned long tdmr_get_pamt_sz(struct tdmr_info *tdmr, int pgsz,
> > -					     u16 pamt_entry_size)
> > +static __init unsigned long tdmr_get_pamt_sz(struct tdmr_info *tdmr, int pgsz)
> >   {
> >   	unsigned long pamt_sz, nr_pamt_entries;
> > +	const int tdx_pg_size_shift[] = { PAGE_SHIFT, PMD_SHIFT, PUD_SHIFT };
> 
> Both of these consts go hand-in-hand, right? I would write it as
> tdx_pg_size_shift[TDX_PS_NR] to make the connection obvious.

Yea that seems reasonable.

> 
> Just curious, why is TDX_PS_NR defined as (TDX_PS_1G + 1)? I don't think
> we are planning to add TDX_PS_256G anytime soon. But, should
> TDX_PS_4K..TDX_PS_NR be an enum?

Yea probably. The point of this patch is to improve the code such the following
patch can not make it worse. So I'd think it's best not to add extra cleanups to
it. I'll add this to the list. But we probably have more pressing cleanups.