Re: [PATCH v1 05/11] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions

Kuppuswamy, Sathyanarayanan
2021-06-14T19:46:10Z

On 6/14/21 1:47 AM, Borislav Petkov wrote:
> On Tue, Jun 01, 2021 at 07:21:30PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> +/*
>> + * __tdx_module_call() - Helper function used by TDX guests to request
>> + * services from the TDX module (does not include VMM services).
>> + *
>> + * This function serves as a wrapper to move user call arguments to the
>> + * correct registers as specified by "tdcall" ABI
>
> Please state here explicitly what the TDCALL ABI is. I see below "moved
> to" which translates the x86_64 ABI into your ABI but please state it
> here explicitly what it is (which register is what in a tabellary form)
> so that it is crystal clear and the code can be followed easily.

Ok. I will include the TDCALL ABI details here.

>
>> and shares it with the
>> + * TDX module. If the "tdcall" operation is successful and a valid
>
> Use TDCALL everywhere here in the comments to refer to the
> insn/operation.

Ok. I will fix it in next version.

>
>> + * "struct tdx_module_output" pointer is available (in "out" argument),
>> + * output from the TDX module is saved to the memory specified in the
>> + * "out" pointer. Also the status of the "tdcall" operation is returned
>> + * back to the user as a function return value.
>> + *
>> + * @fn (RDI) - TDCALL Leaf ID, moved to RAX
>> + * @rcx (RSI) - Input parameter 1, moved to RCX
>> + * @rdx (RDX) - Input parameter 2, moved to RDX
>> + * @r8 (RCX) - Input parameter 3, moved to R8
>> + * @r9 (R8) - Input parameter 4, moved to R9
>> + *
>> + * @out (R9) - struct tdx_module_output pointer
>> + * stored temporarily in R12 (not
>> + * shared with the TDX module)
>> + *
>> + * Return status of tdcall via RAX.
>> + *
>> + * NOTE: This function should not be used for TDX hypercall
>> + * use cases.
>
> What does that mean? I think you wanna say here that this function calls
> the *module* and not the hypervisor...

Yes, you are right. But I can remove that comment. It does not add much
value.

>> + */
>> + push %r12 /* Callee saved, so preserve it */
>> + mov %r9, %r12 /* Move output pointer to R12 */
>
> Please make all those side comments, top comments by moving them over
> the line they refer to.

Ok. I will move it up.

>

>> +
>> + /* Check if caller provided an output struct */
>> + test %r12, %r12
>> + jz 1f
>
> Why is that check done *after* the TDCALL and not before?
>
> You can have TDCALL leaf functions without output?

Yes. It is possible to call tdx_module_call() without output
pointer.

Examples are TDREPORT and TDACCEPTPAGE.

>
> If so, just say so.

I will include comment about it.

>> + * do_tdx_hypercall() - Helper function used by TDX guests to request
>> + * services from the VMM. All requests are made via the TDX module
>> + * using "TDCALL" instruction.
>> + *
>> + * This function is created to contain common code between vendor
>> + * specific and standard type TDX hypercalls. So the caller of this
>> + * function had to set the TDVMCALL type in the R10 register before
>> + * calling it.
>> + *
>> + * This function serves as a wrapper to move user call arguments to the
>> + * correct registers as specified by "tdcall" ABI and shares it with VMM
>
> As above - document the ABI explicitly here pls.

Ok. I will add the ABI details in next version.

>

>
> ^ Superfluous line.

will remove it.

>
>> + */
>> +SYM_FUNC_START_LOCAL(do_tdx_hypercall)
>> + /* Save non-volatile GPRs that are exposed to the VMM. */
>
> "Save callee-s ved GPRs as mandated by the x86_64 ABI"
>
> because you're callee and you have to save them. :)
>
>> + push %r15
>> + push %r14
>> + push %r13
>> + push %r12
>> +
>> + /* Leave hypercall output pointer in R9, it's not clobbered by VMM */
>
> Guaranteed by what, exactly?
>
> I'm sure you have enough stack to push another u64 value and then
> restore it after the TDCALL so that you don't have to care what the VMM
> does wrt R9.

Since we don't mark R9 as shared in RCX register, we don't expect VMM to
use it. But I am not sure whether TDX module will guarantee it. So, for our
use case, I can use stack for it.

>
>> +
>> + /* Mangle function call ABI into TDCALL ABI: */
>> + xor %eax, %eax /* Move TDCALL leaf ID (TDVMCALL (0)) to RAX */
>
> If leaf function 0 is calling the HV, then says so instead of writing
> "Move" but having an XOR there.

May be I should define a macro for it and use Mov to keep it uniform
with other register updates.

>
>> + mov %rdi, %r11 /* Move TDVMCALL function id to R11 */
>
> If I'm reading that pdf correctly, it says:
>
> "R11 TDG.VP.VMCALL sub-function if R10 is 0 (see enumeration below)"

Yes, it is the sub function id. I will fix the comment in next version.

>
>> + mov %rsi, %r12 /* Move input 1 to R12 */
>> + mov %rdx, %r13 /* Move input 2 to R13 */
>> + mov %rcx, %r14 /* Move input 1 to R14 */
>> + mov %r8, %r15 /* Move input 1 to R15 */
>> + /* Caller of do_tdx_hypercall() will set TDVMCALL type in R10 */
>
> Ah, there it is. Yuck.
>
> How about passing that vendor-specific leaf set on the stack like all
> the other sane ABIs do when they need more than 6 input params passed
> through regs?
>
> I don't like a caller function prepping registers for the callee.

do_tdx_hypercall() is defined and used only in this assembly file.
It is the helper function for __tdx_hypercall() and
__tdx_hypercall_vendor_kvm() functions which needs different values in
R10 register.

But, I am fine with passing it via stack, if this is recommended.

Please let me know.

>
>> +
>> + movl $TDVMCALL_EXPOSE_REGS_MASK, %ecx
>> +
>> + tdcall
>> +
>> + /*
>> + * Non-zero RAX values indicate a failure of TDCALL itself.
>> + * Panic for those. This value is unrelated to the hypercall
>> + * result in R10.
>> + */
>> + test %rax, %rax
>> + jnz 2f
>> +
>> + /* Move hypercall error code to RAX to return to user */
>> + mov %r10, %rax
>> +
>> + /* Check for hypercall success: 0 - Successful, otherwise failed */
>> + test %rax, %rax
>> + jnz 1f
>> +
>> + /* Check if caller provided an output struct */
>
> Same as for __tdx_module_call

It is possible to call it without output pointer. I will include comments
about it.

>
>> + test %r9, %r9
>> + jz 1f
>> +
>> + /* Copy hypercall result registers to output struct: */
>> + movq %r11, TDX_HYPERCALL_r11(%r9)
>> + movq %r12, TDX_HYPERCALL_r12(%r9)
>> + movq %r13, TDX_HYPERCALL_r13(%r9)
>> + movq %r14, TDX_HYPERCALL_r14(%r9)
>> + movq %r15, TDX_HYPERCALL_r15(%r9)
>> +1:
>> + /*
>> + * Zero out registers exposed to the VMM to avoid
>
> Why if you...
>
>> + * speculative execution with VMM-controlled values.
>> + * This needs to include all registers present in
>> + * TDVMCALL_EXPOSE_REGS_MASK.
>> + */
>> + xor %r10d, %r10d
>> + xor %r11d, %r11d
>> + xor %r12d, %r12d
>> + xor %r13d, %r13d
>> + xor %r14d, %r14d
>> + xor %r15d, %r15d
>> +
>> + /* Restore non-volatile GPRs that are exposed to the VMM. */
>> + pop %r12
>> + pop %r13
>> + pop %r14
>> + pop %r15
>
> ... are going to overwrite most of them here?
>
> I.e., you can clear only R10 and R11 and the rest will be overwritten by
> the callee-saved values.

Yes. You are correct. I can clear only R10-R11.

>
>> +
>> + ret
>> +2:
>> + /*
>> + * Reaching here means failure in TDCALL execution. This is
>> + * not supposed to happen in hypercalls. It means the TDX
>> + * module is in buggy state. So panic.
>> + */
>> + ud2
>
> How is the user going to know that the module has a bug? Are we issuing
> an error message somewhere before that panic or the guest screen will
> remain black/freeze and the poor luser won't have a clue what happened?

With the trace support, they should be able to see the flow before making
the tdx_*_call(). That should be enough clue for debug right?

>
>> + if (err)
>> + pr_warn_ratelimited("TDVMCALL fn:%llx failed with err:%llx\n",
>
> Prefix those hex values with "0x" so that it is clear what the number
> format is. Ditto below.
>
>> + fn, err);
>
> You can leave those to stick out and not break the line. Ditto below.

Ok. I will follow your recommendation. I have done it this way to fix
checkpatch reports.

>
>> +
>> + return err;
>> +}
>> +
>> +/*
>> + * Wrapper for the semi-common case where user need single output
>> + * value (R11). Callers of this function does not care about the
>> + * hypercall error code (mainly for IN or MMIO usecase).
>> + */
>> +static inline u64 tdx_hypercall_out_r11(u64 fn, u64 r12, u64 r13,
>
> No need to hardcode the register which has the retval in the function
> name - just call it tdx_hypercall_out() or so.

If we need helper functions for other output registers in future, we might
have to add the suffix.

>> --
>> 2.25.1
>>
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer