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

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

On 28/11/24 13:13, Adrian Hunter wrote:
> 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.
> 

IIRC Dave said something like, if the wrapper doesn't add any
value, then it is just as well not to have it at all.

So that option would be to drop patch "x86/virt/tdx: Add SEAMCALL
wrapper to enter/exit TDX guest" with tdh_vp_enter() and instead
just call __seamcall_saved_ret() directly, noting that:

 - __seamcall_saved_ret() is only used for TDH.VP.ENTER
 - KVM seems likely to be the only code that would ever
 need to use TDH.VP.ENTER