Re: [PATCH RFC 1/7] x86/virt/tdx: Add SEAMCALL wrapper to enter/exit TDX guest

[Adrian Hunter <adrian.hunter@intel.com>]

On 25/11/24 15:40, Adrian Hunter wrote:
> On 22/11/24 18:33, Dave Hansen wrote:
>> On 11/22/24 03:10, Adrian Hunter wrote:
>>> +struct tdh_vp_enter_tdcall {
>>> +	u64	reg_mask	: 32,
>>> +		vm_idx		:  2,
>>> +		reserved_0	: 30;
>>> +	u64	data[TDX_ERR_DATA_PART_2];
>>> +	u64	fn;	/* Non-zero for hypercalls, zero otherwise */
>>> +	u64	subfn;
>>> +	union {
>>> +		struct tdh_vp_enter_vmcall 		vmcall;
>>> +		struct tdh_vp_enter_gettdvmcallinfo	gettdvmcallinfo;
>>> +		struct tdh_vp_enter_mapgpa		mapgpa;
>>> +		struct tdh_vp_enter_getquote		getquote;
>>> +		struct tdh_vp_enter_reportfatalerror	reportfatalerror;
>>> +		struct tdh_vp_enter_cpuid		cpuid;
>>> +		struct tdh_vp_enter_mmio		mmio;
>>> +		struct tdh_vp_enter_hlt			hlt;
>>> +		struct tdh_vp_enter_io			io;
>>> +		struct tdh_vp_enter_rd			rd;
>>> +		struct tdh_vp_enter_wr			wr;
>>> +	};
>>> +};
>>
>> Let's say someone declares this:
>>
>> struct tdh_vp_enter_mmio {
>> 	u64	size;
>> 	u64	mmio_addr;
>> 	u64	direction;
>> 	u64	value;
>> };
>>
>> How long is that going to take you to debug?
> 
> When adding a new hardware definition, it would be sensible
> to check the hardware definition first before checking anything
> else.
> 
> However, to stop existing members being accidentally moved,
> could add:
> 
> #define CHECK_OFFSETS_EQ(reg, member) \
> 	BUILD_BUG_ON(offsetof(struct tdx_module_args, reg) != offsetof(union tdh_vp_enter_args, member));
> 
> 	CHECK_OFFSETS_EQ(r12, tdcall.mmio.size);
> 	CHECK_OFFSETS_EQ(r13, tdcall.mmio.direction);
> 	CHECK_OFFSETS_EQ(r14, tdcall.mmio.mmio_addr);
> 	CHECK_OFFSETS_EQ(r15, tdcall.mmio.value);
> 

Note, struct tdh_vp_enter_tdcall is an output format.  The tdcall
arguments come directly from the guest with no validation by the
TDX Module.  It could be rubbish, or even malicious rubbish.  The
exit handlers validate the values before using them.

WRT the TDCALL input format (response by the host VMM), 'ret_code'
and 'failed_gpa' could use types other than 'u64', but the other
members are really 'u64'.

/* TDH.VP.ENTER Input Format #2 : Following a previous TDCALL(TDG.VP.VMCALL) */
struct tdh_vp_enter_in {
	u64	__vcpu_handle_and_flags; /* Don't use. tdh_vp_enter() will take care of it */
	u64	unused[3];
	u64	ret_code;
	union {
		u64 gettdvmcallinfo[4];
		struct {
			u64	failed_gpa;
		} mapgpa;
		struct {
			u64	unused;
			u64	eax;
			u64	ebx;
			u64	ecx;
			u64	edx;
		} cpuid;
		/* Value read for IO, MMIO or RDMSR */
		struct {
			u64	value;
		} read;
	};
};

Another different alternative could be to use an opaque structure,
not visible to KVM, and then all accesses to it become helper
functions like:

struct tdx_args;

int tdx_args_get_mmio(struct tdx_args *args,
		      enum tdx_access_size *size,
		      enum tdx_access_dir *direction,
		      gpa_t *addr,
		      u64 *value);

void tdx_args_set_failed_gpa(struct tdx_args *args, gpa_t gpa);
void tdx_args_set_ret_code(struct tdx_args *args, enum tdx_ret_code ret_code);
etc

For the 'get' functions, that would tend to imply the helpers
would do some validation.