Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers

[Sean Christopherson <seanjc@google.com>]

On Thu, Jan 29, 2026, Rick P Edgecombe wrote:
> On Wed, 2026-01-28 at 17:19 -0800, Sean Christopherson wrote:
> > Honestly, the entire scheme is a mess.  Four days of staring at this
> > and I finally undertand what the code is doing.  The whole "struct
> > tdx_module_array_args" union is completely unnecessary, the resulting
> > args.args crud is ugly, having a pile of duplicate accessors is
> > brittle, the code obfuscates a simple concept, and the end result
> > doesn't provide any actual protection since the kernel will happily
> > overflow the buffer after the WARN.
> 
> The original sin for this, as was spotted by Nikilay in v3, is actually
> that it turns out that the whole variable length thing was intended to
> give the TDX module flexibility *if* it wanted to increase it in the
> future. As in it's not required today. Worse, whether it would actually
> grow in the specific way the code assumes is not covered in the spec.
> Apparently it was based on some past internal discussions. So the
> agreement on v3 was to just support the fixed two page size in the
> spec.

Heh, I was _this_ close to suggesting we add a compile-time assert on the incoming
size of the array (and effective regs array), with a constant max size supported
by the kernel.  It wouldn't eliminate the array shenanigans, but it would let us
make them more or less bombproof.

> Yea it could probably use another DEFINE or two to make it less error
> prone. Vanilla DPAMT has 4 instances of rdx.

For me, it's not just a syntax problem.  It's the approach of getting a pointer
to the middle of structure and then doing a memcpy() at a later point in time.
More below.

> What you have here is close to what I had done when I first took this
> series. But it ran afoul of FORTIFY_SOUCE and required some horrible
> casting to trick it. I wonder if this code will hit that issue too.

AFAICT, FORTIFY_SOURCE doesn't complain.

> Dave didn't like the solution and suggested the union actually:
> https://lore.kernel.org/kvm/355ad607-52ed-42cc-9a48-63aaa49f4c68@intel.com/#t

What you proposed is fundamentally quite different than what I'm proposing.  I'm
not complaining about the union, I'm complaining about providing a helper to grab
a pointer to the middle of a struct and then open coding memcpy() calls using
that pointer.  I find that _extremely_ difficult to grok, because it does a poor
job of capturing the intent (copy these values to this sequence of registers).

The decouple pointer+memcpy() approach also bleeds gory details about how PAMT
pages are passed in multiple args throughout all SEAMCALL APIs that have such
args.  I want the APIs to not have to care about the underlying mechanics of how
PAMT pages are copied from an array to registers, e.g. so that readers of the
code can focus on the semantics of the SEAMCALL and the pieces of logic that are
unique to each SEAMCALL.

In other words, I see the necessity for a union as being a symptom of the flawed
approach.

> I'm aware of your tendency to dislike union based solutions. But since
> this was purely contained to tip, I went with Dave's preference.