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
