From: "Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@linux.intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Andy Lutomirski <luto@kernel.org>, Peter H Anvin <hpa@zytor.com>,
Dave Hansen <dave.hansen@intel.com>,
Tony Luck <tony.luck@intel.com>,
Dan Williams <dan.j.williams@intel.com>,
Andi Kleen <ak@linux.intel.com>,
Kirill Shutemov <kirill.shutemov@linux.intel.com>,
Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
Sean Christopherson <seanjc@google.com>,
linux-kernel@vger.kernel.org, x86@kernel.org
Subject: Re: [PATCH v1 05/11] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions
Date: Mon, 14 Jun 2021 12:45:45 -0700 [thread overview]
Message-ID: <b0dff409-d084-bfc1-c260-e1732b5e8ee5@linux.intel.com> (raw)
In-Reply-To: <YMcXvzD2o7rWsl0W@zn.tnic>
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
next prev parent reply other threads:[~2021-06-14 19:46 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-02 2:21 [PATCH v1 00/11] Add TDX Guest Support (Initial support) Kuppuswamy Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 01/11] x86/paravirt: Move halt paravirt calls under CONFIG_PARAVIRT Kuppuswamy Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 02/11] x86/tdx: Introduce INTEL_TDX_GUEST config option Kuppuswamy Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 03/11] x86/cpufeatures: Add TDX Guest CPU feature Kuppuswamy Sathyanarayanan
2021-06-07 14:32 ` Tom Lendacky
2021-06-07 16:59 ` Kuppuswamy, Sathyanarayanan
2021-06-10 12:28 ` Borislav Petkov
2021-06-10 14:28 ` Kuppuswamy, Sathyanarayanan
2021-06-10 14:29 ` Kirill A. Shutemov
2021-06-10 14:35 ` Borislav Petkov
2021-06-10 14:41 ` Kirill A. Shutemov
2021-06-10 15:56 ` Borislav Petkov
2021-06-12 21:02 ` [PATCH v2 03/12] " Kuppuswamy Sathyanarayanan
2021-06-16 9:52 ` Borislav Petkov
2021-06-16 16:57 ` Kuppuswamy, Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 04/11] x86/x86: Add is_tdx_guest() interface Kuppuswamy Sathyanarayanan
2021-06-10 19:59 ` Borislav Petkov
2021-06-10 21:01 ` Kuppuswamy, Sathyanarayanan
2021-06-10 21:07 ` Borislav Petkov
2021-06-12 21:04 ` [PATCH v2 04/12] x86/x86: Add early_is_tdx_guest() interface Kuppuswamy Sathyanarayanan
2021-06-17 17:05 ` Borislav Petkov
2021-06-18 19:14 ` Kuppuswamy, Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 05/11] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions Kuppuswamy Sathyanarayanan
2021-06-14 8:47 ` Borislav Petkov
2021-06-14 19:45 ` Kuppuswamy, Sathyanarayanan [this message]
2021-06-14 20:11 ` Borislav Petkov
2021-06-14 21:37 ` Kuppuswamy, Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 06/11] x86/tdx: Get TD execution environment information via TDINFO Kuppuswamy Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 07/11] x86/traps: Add #VE support for TDX guest Kuppuswamy Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 08/11] x86/tdx: Add HLT " Kuppuswamy Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 09/11] x86/tdx: Wire up KVM hypercalls Kuppuswamy Sathyanarayanan
2021-06-12 21:08 ` [PATCH v2 10/12] " Kuppuswamy Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 10/11] x86/tdx: Add MSR support for TDX guest Kuppuswamy Sathyanarayanan
2021-06-02 2:21 ` [PATCH v1 11/11] x86/tdx: Handle CPUID via #VE Kuppuswamy Sathyanarayanan
-- strict thread matches above, loose matches on Subject: below --
2021-06-02 2:18 [PATCH v1 00/11] Add TDX Guest Support (Initial support) Kuppuswamy Sathyanarayanan
2021-06-02 2:18 ` [PATCH v1 05/11] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions Kuppuswamy Sathyanarayanan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b0dff409-d084-bfc1-c260-e1732b5e8ee5@linux.intel.com \
--to=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=ak@linux.intel.com \
--cc=bp@alien8.de \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@intel.com \
--cc=hpa@zytor.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=knsathya@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.