Re: [PATCH v2 08/25] x86/virt/tdx: Add SEAMCALL wrappers for TDX page cache management

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

On Wed, 2024-11-13 at 13:08 -0800, Dave Hansen wrote:
> Let's say I see the error get spit out on the console.  I can't make any
> sense out of it from this spot.  I need to go over to the TDX docs or
> tdh_phymem_page_reclaim() to look at the *comment* to figure out what
> these the registers are named.
> 
> The code as proposed has zero self-documenting properties.  It's
> actually completely non-self-documenting.  It isn't _any_ better for
> readability than just doing:
> 
> 	struct tdx_module_args args = {};
> 
> 	for (i = TDX_SEAMCALL_RETRIES; i > 0; i--) {
> 		args.rcx = pa;
> 		err = seamcall_ret(TDH_PHYMEM_PAGE_RECLAIM, &args);
> 		...
> 	}
> 
> 	pr_tdx_error_3(TDH_PHYMEM_PAGE_RECLAIM, err,
> 			args.rcx, args.rdx, args.r8);

If we extracted meaning from the registers and printed those, then we would not
have any new bits that popped up in there. For example, currently r8 has bits
63:3 described as reserved. While expectations around TDX module behavior
changes are still settling, I'd rather have the full register for debugging than
an easy to read error message. But we have actually gone down this road a little
bit already when we adjusted the KVM calling code to stop manually loading the
struct tdx_module_args.

> 
> Also, this is also showing a lack of naming discipline where things are
> named.  The first argument is 'pa' in here but 'page' on the other side:
> 
> > +u64 tdh_phymem_page_reclaim(u64 page, u64 *rcx, u64 *rdx, u64 *r8)
> > +{
> > +	struct tdx_module_args args = {
> > +		.rcx = page,
> 
> I can't tell you how many recompiles it's cost me when I got lazy about
> physical addr vs. virtual addr vs. struct page vs. pfn.

Standardizing on VAs for the SEAMCALL wrappers seems like a good idea. I haven't
checked them all, but seems to be promising so far.

> 
> So, yeah, I'd rather not export seamcall_ret(), but I'd rather do that
> than have a layer of abstraction that's adding little value while it
> also brings obfuscation.

In KVM these types can get even more confusing. There are guest physical address
and virtual addresses as well as the host physical and virtual. So in KVM there
is a typedef for host physical addresses: hpa_t. Previously these wrappers used
it because they are in KVM code. It was:
+static inline u64 tdh_phymem_page_reclaim(hpa_t page, u64 *rcx, u64 *rdx,
+					  u64 *r8)
+{
+	struct tdx_module_args in = {
+		.rcx = page,
+	};
+	u64 ret;
+
+	ret = seamcall_ret(TDH_PHYMEM_PAGE_RECLAIM, &in);
+
+	*rcx = in.rcx;
+	*rdx = in.rdx;
+	*r8 = in.r8;
+
+	return ret;
+}

Moving them to arch/x86 means we need to translate some things between KVM's
parlance and the rest of the kernels. This is extra wrapping. Another example
that was used in the old SEAMCALL wrappers was gpa_t, which KVM uses to refers
to a guest physical address. void * to the host direct map doesn't fit, so we
are back to u64 or a new gpa struct (like in the other thread) to speak to the
arch/x86 layers.

So I think we will need some light layers of abstraction if we keep the wrappers
in arch/x86.