From: Sean Christopherson <seanjc@google.com>
To: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Andy Lutomirski <luto@kernel.org>,
Dave Hansen <dave.hansen@intel.com>,
Andi Kleen <ak@linux.intel.com>,
Kirill Shutemov <kirill.shutemov@linux.intel.com>,
Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
Dan Williams <dan.j.williams@intel.com>,
Raj Ashok <ashok.raj@intel.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/1] x86/tdx: Add tdcall() and tdvmcall() helper functions
Date: Fri, 19 Mar 2021 16:55:16 +0000 [thread overview]
Message-ID: <YFTXdG+zZ32gVIPc@google.com> (raw)
In-Reply-To: <20210318213053.203403-1-sathyanarayanan.kuppuswamy@linux.intel.com>
On Thu, Mar 18, 2021, Kuppuswamy Sathyanarayanan wrote:
> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
> index e44e55d1e519..7ae1d25e272b 100644
> --- a/arch/x86/kernel/tdx.c
> +++ b/arch/x86/kernel/tdx.c
> @@ -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,
and even harder to maintain. E.g. the %1 usage falls apart if an 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.
> + " 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, makes it
likely that the kernel will leak information to the VMM, e.g. in the form of
stack data due to a partially initialized "regs".
And although TDVMCALL is anything but speedy, requiring multiple memory
operations just to set a single register is unnecessary. Not to mention
several of these registers are never used in the GHCI-defined TDVMCALLs. And,
since the caller defines the mask (which I also dislike), it's possible/likely
that many of these memory operations are wasteful even for registers that are
used by _some_ TDVMCALLs. Unnecessary accesses are inevitable if we want a
common helper, but this is too much.
> + TDCALL ";"
> + /* Save TDCALL success/failure to regs->rax */
> + " movq %%rax, (%1);"
> + /* Save rcx and rdx contents to regs->r[c-d]x */
> + " movq %%rcx, 8(%1);"
> + " movq %%rdx, 16(%1);"
> + /* Move content of registers R8-R15 regs->r[8-15] */
> + " movq %%r8, 24(%1);"
> + " movq %%r9, 32(%1);"
> + " movq %%r10, 40(%1);"
> + " movq %%r11, 48(%1);"
> + " movq %%r12, 56(%1);"
> + " movq %%r13, 64(%1);"
> + " movq %%r14, 72(%1);"
> + " movq %%r15, 80(%1);"
> +
> + :
> + : "r" (leafid), "r" (regs)
> + : "memory", "rax", "rbx", "rcx", "rdx", "r8",
> + "r9", "r10", "r11", "r12", "r13", "r14", "r15"
All these clobbers mean even more memory operations...
> + );
> +
> +}
> +
> +void tdvmcall(u64 subid, struct tdcall_regs *regs)
> +{
> + /* Expose GPRs R8-R15 to VMM */
> + regs->rcx = 0xff00;
> + /* R10 = 0 (standard TDVMCALL) */
> + regs->r10 = TDVMCALL_STANDARD;
> + /* Save subid to r11 register */
> + regs->r11 = subid;
> +
> + tdcall(TDVMCALL, regs);
This implies the caller is responsible for _all_ error checking. The base
TDCALL should never fail; if it does, something is horribly wrong with TDX-Module
and panicking is absolutely the best option.
The users of this are going to be difficult to read as well since the parameters
are stuff into a struct instead of being passed to a function.
IMO, throwing the bulk of the code in a proper asm subroutine and handling only
the GHCI-defined TDVMCALLs is the way to go. If/when a VMM comes along that
wants to enlighten Linux guests to work with non-GCHI TDVMCALLs, enhancing this
madness can be their problem.
Completely untested...
struct tdvmcall_output {
u64 r12;
u64 r13;
u64 r14;
u64 r15;
}
u64 __tdvmcall(u64 fn, u64 p0, u64 p1, u64 p2, u64 p3,
struct tdvmcall_output *out);
/* 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
/*
* 0 => RAX = TDCALL leaf
* 0 => R10 = standard vs. vendor
* RDI => R11 = TDVMCALL function, e.g. exit reason
* RSI => R12 = input param 0
* RDX => R13 = input param 1
* RCX => R14 = input param 2
* R8 => R15 = input param 3
* MASK => RCX = TDVMCALL register behavior
* R9 => N/A = output struct
*/
xor %eax, %eax
xor %r10d, %r10d
mov %rdi, %r11
mov %rsi, %r12
mov %rdx, %r13
mov %rcx, %r14
mov %r8, %r15
/*
* Expose R10 - R15, i.e. all GPRs that may be used by TDVMCALLs
* defined in the GHCI. Note, RAX and RCX are consumed, but only by
* TDX-Module and so don't need to be listed in the mask.
*/
movl $0xfc00, %ecx
tdcall
/* Panic if TDCALL reports failure. */
test %rax, %rax
jnz 2f
/* Propagate TDVMCALL success/failure to return value. */
mov %r10, %rax
/*
* On success, propagate TDVMCALL outputs values to the output struct,
* if an output struct is provided.
*/
test %rax, %rax
jnz 1f
test %r9, %r9
jz 1f
movq %r12, $TDVMCALL_r12(%r9)
movq %r13, $TDVMCALL_r13(%r9)
movq %r14, $TDVMCALL_r14(%r9)
movq %r15, $TDVMCALL_r15(%r9)
1:
/*
* Zero out registers exposed to the VMM to avoid speculative execution
* with VMM-controlled values.
*/
xor %r10d, %r10d
xor %r11d, %r11d
xor %r12d, %r12d
xor %r13d, %r13d
xor %r14d, %r14d
xor %r15d, %r15d
pop %r12
pop %r13
pop %r14
pop %r15
FRAME_END
ret
2:
ud2
SYM_FUNC_END(__tdvmcall)
/*
* Wrapper for the semi-common case where errors are fatal and there is a
* single output value.
*/
static inline u64 tdvmcall(u64 fn, u64 p0, u64 p1, u64 p2, u64 p3,
struct tdvmcall_output *out)
{
struct tdvmcall_output out;
u64 err;
err = __tdvmcall(fn, p0, p1, p2, p3, &out);
BUG_ON(err);
return out.r11;
}
static void tdx_handle_cpuid(struct pt_regs *regs)
{
struct tdvmcall_output out;
u64 err;
err = __tdvmcall(EXIT_REASON_CPUID, regs->ax, regs->cx, 0, 0, &out);
BUG_ON(err);
regs->ax = out.r11;
regs->bx = out.r12;
regs->cx = out.r13;
regs->dx = out.r14;
}
#define REG_MASK(size) ((1ULL << ((size) * 8)) - 1)
static void tdx_handle_io(struct pt_regs *regs, u32 exit_qual)
{
u8 out = (exit_qual & 8) ? 0 : 1;
u8 size = (exit_qual & 7) + 1;
u16 port = exit_qual >> 16;
u64 val;
/* I/O strings ops are unrolled at build time. */
BUG_ON(exit_qual & 0x10);
if (!tdx_allowed_port(port))
return;
if (out)
val = regs->ax & REG_MASK(size);
else
val = 0;
val = tdvmcall(EXIT_REASON_IO_INSTRUCTION, port, size, out, val);
if (!out) {
/* The upper bits of *AX are preserved for 2 and 1 byte I/O. */
if (size < 4)
val |= (regs->ax & ~REG_MASK(size));
regs->ax = val;
}4
}
static u64 tdx_read_msr_safe(unsigned int msr, int *ret)
{
struct tdvmcall_output out;
u64 err;
WARN_ON_ONCE(tdx_is_context_switched_msr(msr));
if (msr == MSR_CSTAR) {
*ret = 0;
return 0;
}
err = __tdvmcall(EXIT_REASON_MSR_READ, regs->ax, regs->cx, 0, 0, &out);
if (err) {
*ret -EIO;
return 0;
}
return out.r11;
}
next prev parent reply other threads:[~2021-03-19 16:56 UTC|newest]
Thread overview: 161+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-06 3:02 Test Email sathyanarayanan.kuppuswamy
2021-02-05 23:38 ` [RFC v1 00/26] Add TDX Guest Support Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 01/26] x86/paravirt: Introduce CONFIG_PARAVIRT_XL Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 02/26] x86/cpufeatures: Add TDX Guest CPU feature Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 03/26] x86/cpufeatures: Add is_tdx_guest() interface Kuppuswamy Sathyanarayanan
2021-04-01 21:08 ` Dave Hansen
2021-04-01 21:15 ` Kuppuswamy, Sathyanarayanan
2021-04-01 21:19 ` Dave Hansen
2021-04-01 22:25 ` Kuppuswamy, Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 04/26] x86/tdx: Get TD execution environment information via TDINFO Kuppuswamy Sathyanarayanan
2021-02-08 10:00 ` Peter Zijlstra
2021-02-08 19:10 ` Kuppuswamy, Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 05/26] x86/traps: Add #VE support for TDX guest Kuppuswamy Sathyanarayanan
2021-02-08 10:20 ` Peter Zijlstra
2021-02-08 16:23 ` Andi Kleen
2021-02-08 16:33 ` Peter Zijlstra
2021-02-08 16:46 ` Sean Christopherson
2021-02-08 16:59 ` Peter Zijlstra
2021-02-08 19:05 ` Kuppuswamy, Sathyanarayanan
2021-02-08 16:46 ` Andi Kleen
2021-02-12 19:20 ` Dave Hansen
2021-02-12 19:47 ` Andy Lutomirski
2021-02-12 20:06 ` Sean Christopherson
2021-02-12 20:17 ` Dave Hansen
2021-02-12 20:37 ` Sean Christopherson
2021-02-12 20:46 ` Dave Hansen
2021-02-12 20:54 ` Sean Christopherson
2021-02-12 21:06 ` Dave Hansen
2021-02-12 21:37 ` Sean Christopherson
2021-02-12 21:47 ` Andy Lutomirski
2021-02-12 21:48 ` Dave Hansen
2021-02-14 19:33 ` Andi Kleen
2021-02-14 19:54 ` Andy Lutomirski
2021-02-12 20:20 ` Andy Lutomirski
2021-02-12 20:44 ` Sean Christopherson
2021-02-05 23:38 ` [RFC v1 06/26] x86/tdx: Add HLT " Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 07/26] x86/tdx: Wire up KVM hypercalls Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 08/26] x86/tdx: Add MSR support for TDX guest Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 09/26] x86/tdx: Handle CPUID via #VE Kuppuswamy Sathyanarayanan
2021-02-05 23:42 ` Andy Lutomirski
2021-02-07 14:13 ` Kirill A. Shutemov
2021-02-07 16:01 ` Dave Hansen
2021-02-07 20:29 ` Kirill A. Shutemov
2021-02-07 22:31 ` Dave Hansen
2021-02-07 22:45 ` Andy Lutomirski
2021-02-08 17:10 ` Sean Christopherson
2021-02-08 17:35 ` Andy Lutomirski
2021-02-08 17:47 ` Sean Christopherson
2021-03-18 21:30 ` [PATCH v1 1/1] x86/tdx: Add tdcall() and tdvmcall() helper functions Kuppuswamy Sathyanarayanan
2021-03-19 16:55 ` Sean Christopherson [this message]
2021-03-19 17:42 ` Kuppuswamy, Sathyanarayanan
2021-03-19 18:22 ` Dave Hansen
2021-03-19 19:58 ` Kuppuswamy, Sathyanarayanan
2021-03-26 23:38 ` [PATCH v2 1/1] x86/tdx: Add __tdcall() and __tdvmcall() " Kuppuswamy Sathyanarayanan
2021-04-20 17:36 ` Dave Hansen
2021-04-20 19:20 ` Kuppuswamy, Sathyanarayanan
2021-04-20 19:59 ` Dave Hansen
2021-04-20 23:12 ` Kuppuswamy, Sathyanarayanan
2021-04-20 23:42 ` Dave Hansen
2021-04-23 1:09 ` Kuppuswamy, Sathyanarayanan
2021-04-23 1:21 ` Dave Hansen
2021-04-23 1:35 ` Andi Kleen
2021-04-23 15:15 ` Sean Christopherson
2021-04-23 15:28 ` Dan Williams
2021-04-23 15:38 ` Andi Kleen
2021-04-23 15:50 ` Sean Christopherson
2021-04-23 15:47 ` Andi Kleen
2021-04-23 18:18 ` Kuppuswamy, Sathyanarayanan
2021-04-20 23:53 ` Dan Williams
2021-04-20 23:59 ` Kuppuswamy, Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 10/26] x86/io: Allow to override inX() and outX() implementation Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 11/26] x86/tdx: Handle port I/O Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 12/26] x86/tdx: Handle in-kernel MMIO Kuppuswamy Sathyanarayanan
2021-04-01 19:56 ` Dave Hansen
2021-04-01 22:26 ` Sean Christopherson
2021-04-01 22:53 ` Dave Hansen
2021-02-05 23:38 ` [RFC v1 13/26] x86/tdx: Handle MWAIT, MONITOR and WBINVD Kuppuswamy Sathyanarayanan
2021-02-05 23:43 ` Andy Lutomirski
2021-02-05 23:54 ` Kuppuswamy, Sathyanarayanan
2021-02-06 1:05 ` Andy Lutomirski
2021-03-27 0:18 ` [PATCH v1 1/1] " Kuppuswamy Sathyanarayanan
2021-03-27 2:40 ` Andy Lutomirski
2021-03-27 3:40 ` Kuppuswamy, Sathyanarayanan
2021-03-27 16:03 ` Andy Lutomirski
2021-03-27 22:54 ` [PATCH v2 " Kuppuswamy Sathyanarayanan
2021-03-29 17:14 ` Dave Hansen
2021-03-29 21:55 ` Kuppuswamy, Sathyanarayanan
2021-03-29 22:02 ` Dave Hansen
2021-03-29 22:09 ` Kuppuswamy, Sathyanarayanan
2021-03-29 22:12 ` Dave Hansen
2021-03-29 22:42 ` Kuppuswamy, Sathyanarayanan
2021-03-29 23:16 ` [PATCH v3 " Kuppuswamy Sathyanarayanan
2021-03-29 23:23 ` Andy Lutomirski
2021-03-29 23:37 ` Kuppuswamy, Sathyanarayanan
2021-03-29 23:42 ` Sean Christopherson
2021-03-29 23:58 ` Andy Lutomirski
2021-03-30 2:04 ` Andi Kleen
2021-03-30 2:58 ` Andy Lutomirski
2021-03-30 15:14 ` Sean Christopherson
2021-03-30 16:37 ` Andy Lutomirski
2021-03-30 16:57 ` Sean Christopherson
2021-04-07 15:24 ` Andi Kleen
2021-03-31 21:09 ` [PATCH v4 " Kuppuswamy Sathyanarayanan
2021-03-31 21:49 ` Dave Hansen
2021-03-31 22:29 ` Kuppuswamy, Sathyanarayanan
2021-03-31 21:53 ` Sean Christopherson
2021-03-31 22:00 ` Dave Hansen
2021-03-31 22:06 ` Sean Christopherson
2021-03-31 22:11 ` Dave Hansen
2021-03-31 22:28 ` Kuppuswamy, Sathyanarayanan
2021-03-31 22:32 ` Sean Christopherson
2021-03-31 22:34 ` Dave Hansen
2021-04-01 3:28 ` Andi Kleen
2021-04-01 3:46 ` Dave Hansen
2021-04-01 4:24 ` Andi Kleen
2021-04-01 4:51 ` [PATCH v5 " Kuppuswamy Sathyanarayanan
2021-03-29 23:39 ` [PATCH v3 " Sean Christopherson
2021-03-29 23:38 ` Dave Hansen
2021-03-30 4:56 ` [PATCH v1 " Xiaoyao Li
2021-03-30 15:00 ` Andi Kleen
2021-03-30 15:10 ` Dave Hansen
2021-03-30 17:02 ` Kuppuswamy, Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 14/26] ACPI: tables: Add multiprocessor wake-up support Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 15/26] x86/boot: Add a trampoline for APs booting in 64-bit mode Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 16/26] x86/boot: Avoid #VE during compressed boot for TDX platforms Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 17/26] x86/boot: Avoid unnecessary #VE during boot process Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 18/26] x86/topology: Disable CPU hotplug support for TDX platforms Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 19/26] x86/tdx: Forcefully disable legacy PIC for TDX guests Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 20/26] x86/tdx: Introduce INTEL_TDX_GUEST config option Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 21/26] x86/mm: Move force_dma_unencrypted() to common code Kuppuswamy Sathyanarayanan
2021-04-01 20:06 ` Dave Hansen
2021-04-06 15:37 ` Kirill A. Shutemov
2021-04-06 16:11 ` Dave Hansen
2021-04-06 16:37 ` Kirill A. Shutemov
2021-02-05 23:38 ` [RFC v1 22/26] x86/tdx: Exclude Shared bit from __PHYSICAL_MASK Kuppuswamy Sathyanarayanan
2021-04-01 20:13 ` Dave Hansen
2021-04-06 15:54 ` Kirill A. Shutemov
2021-04-06 16:12 ` Dave Hansen
2021-02-05 23:38 ` [RFC v1 23/26] x86/tdx: Make pages shared in ioremap() Kuppuswamy Sathyanarayanan
2021-04-01 20:26 ` Dave Hansen
2021-04-06 16:00 ` Kirill A. Shutemov
2021-04-06 16:14 ` Dave Hansen
2021-02-05 23:38 ` [RFC v1 24/26] x86/tdx: Add helper to do MapGPA TDVMALL Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 25/26] x86/tdx: Make DMA pages shared Kuppuswamy Sathyanarayanan
2021-04-01 21:01 ` Dave Hansen
2021-04-06 16:31 ` Kirill A. Shutemov
2021-04-06 16:38 ` Dave Hansen
2021-04-06 17:16 ` Sean Christopherson
2021-02-05 23:38 ` [RFC v1 26/26] x86/kvm: Use bounce buffers for TD guest Kuppuswamy Sathyanarayanan
2021-04-01 21:17 ` Dave Hansen
2021-02-06 3:04 ` Test Email sathyanarayanan.kuppuswamy
2021-02-06 6:24 ` [RFC v1 00/26] Add TDX Guest Support sathyanarayanan.kuppuswamy
2021-03-31 21:38 ` Kuppuswamy, Sathyanarayanan
2021-04-02 0:02 ` Dave Hansen
2021-04-02 2:48 ` Andi Kleen
2021-04-02 15:27 ` Dave Hansen
2021-04-02 21:32 ` Andi Kleen
2021-04-03 16:26 ` Dave Hansen
2021-04-03 17:28 ` Andi Kleen
2021-04-04 15:02 ` Dave Hansen
2021-04-12 17:24 ` Dan Williams
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=YFTXdG+zZ32gVIPc@google.com \
--to=seanjc@google.com \
--cc=ak@linux.intel.com \
--cc=ashok.raj@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@intel.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=knsathya@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=peterz@infradead.org \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).