On 3/19/21 10:42 AM, Kuppuswamy, Sathyanarayanan wrote:Ok. Let me use Sean's proposal and submit tested version of this patch.
I think this point is arguable. Based on the review comments I@@ -4,6 +4,58 @@
#include <asm/tdx.h>
#include <asm/cpufeature.h>
+void tdcall(u64 leafid, struct tdcall_regs *regs)
+{
+ asm volatile(
+ /* RAX = leafid (TDCALL LEAF ID) */
+ " movq %0, %%rax;"
+ /* Move regs->r[*] data to regs r[a-c]x, r8-r5 */
+ " movq 8(%1), %%rcx;"
I am super duper opposed to using inline asm. Large blocks are
hard to read,
received so far, people prefer inline assembly compared to asm sub
functions.
It's arguable, but Sean makes a pretty compelling case.
I actually think inline assembly is a monstrosity. It's insanely arcane
and, as I hope you have noted, does not scale nicely beyond one or two
instructions.
and even harder to maintain. E.g. the %1 usage falls apart if anwe can use OFFSET() calls to improve the readability and avoid this
output constraint is added; that can be avoided by defining a local
const/imm (I forget what they're called), but it doesn't help
readability.
issue. Also IMO, any one adding constraints should know how this
would affect the asm code.
This is about *maintainability*. How _easily_ can someone change this
code in the future? Sean's arguing that it's *hard* to correctly add a
constraint. Unfortunately, our supply of omnipotent kernel developers
is a bit short.
For encouragement, we can add a comment to this function about+ " movq 16(%1), %%rdx;"
+ " movq 24(%1), %%r8;"
+ " movq 32(%1), %%r9;"
+ " movq 40(%1), %%r10;"
+ " movq 48(%1), %%r11;"
+ " movq 56(%1), %%r12;"
+ " movq 64(%1), %%r13;"
+ " movq 72(%1), %%r14;"
+ " movq 80(%1), %%r15;"
This is extremely unsafe, and wasteful. Putting the onus on the
caller to zero out unused registers, with no mechanism to
enforce/encourage doing so,
callers responsibility. makes it
likely that the kernel will leak information to the VMM, e.g. inUnless you create sub-functions for each use cases, callers cannot
the form of stack data due to a partially initialized "regs".
avoid this responsibility.
I don't think we're quite at the point where we throw up our hands.
It would be pretty simple to have an initializer that zeros the
registers out, or looks at the argument mask and does it more precisely.
Surely we can do *something*.
/* Offset for fields in tdvmcall_output */
OFFSET(TDVMCALL_r12, tdvmcall_output, r13);
OFFSET(TDVMCALL_r13, tdvmcall_output, r13);
OFFSET(TDVMCALL_r14, tdvmcall_output, r14);
OFFSET(TDVMCALL_r15, tdvmcall_output, r15);
SYM_FUNC_START(__tdvmcall)
FRAME_BEGIN
/* Save/restore non-volatile GPRs that are exposed to the VMM. */
push %r15
push %r14
push %r13
push %r12
I might have some tweaks for the assembly once someone puts a real patch
together. But, that looks a lot more sane than the inline assembly to me.