Messages in this thread Patch in this message | | | Date | Mon, 19 Jun 2023 14:52:04 +0200 | From | David Hildenbrand <> | Subject | Re: [PATCH v11 05/20] x86/virt/tdx: Add SEAMCALL infrastructure |
| |
On 04.06.23 16:27, Kai Huang wrote: > TDX introduces a new CPU mode: Secure Arbitration Mode (SEAM). This > mode runs only the TDX module itself or other code to load the TDX > module. > > The host kernel communicates with SEAM software via a new SEAMCALL > instruction. This is conceptually similar to a guest->host hypercall, > except it is made from the host to SEAM software instead. The TDX > module establishes a new SEAMCALL ABI which allows the host to > initialize the module and to manage VMs. > > Add infrastructure to make SEAMCALLs. The SEAMCALL ABI is very similar > to the TDCALL ABI and leverages much TDCALL infrastructure. > > SEAMCALL instruction causes #GP when TDX isn't BIOS enabled, and #UD > when CPU is not in VMX operation. Currently, only KVM code mocks with > VMX enabling, and KVM is the only user of TDX. This implementation > chooses to make KVM itself responsible for enabling VMX before using > TDX and let the rest of the kernel stay blissfully unaware of VMX. > > The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The > kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX > first. Architecturally, there is no CPU flag to check whether the CPU > is in VMX operation. Also, if a BIOS were buggy, it could still report > valid TDX private KeyIDs when TDX actually couldn't be enabled. > > Extend the TDX_MODULE_CALL macro to handle #UD and #GP to return error > codes. Introduce two new TDX error codes for them respectively so the > caller can distinguish. > > Also add a wrapper function of SEAMCALL to convert SEAMCALL error code > to the kernel error code, and print out SEAMCALL error code to help the > user to understand what went wrong. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > ---
I agree with Dave that a buggy bios is not a good motivation for this patch. The real strength of this infrastructure IMHO is central error handling and expressive error messages. Maybe it makes some corner cases (reboot -f) easier to handle. That would make a better justification than buggy bios -- and should be spelled out in the patch description.
[...]
> +/* > + * Wrapper of __seamcall() to convert SEAMCALL leaf function error code > + * to kernel error code. @seamcall_ret and @out contain the SEAMCALL > + * leaf function return code and the additional output respectively if > + * not NULL. > + */ > +static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > + u64 *seamcall_ret, > + struct tdx_module_output *out) > +{ > + int cpu, ret = 0; > + u64 sret; > + > + /* Need a stable CPU id for printing error message */ > + cpu = get_cpu(); > + > + sret = __seamcall(fn, rcx, rdx, r8, r9, out); > +
Why not
cpu = get_cpu(); sret = __seamcall(fn, rcx, rdx, r8, r9, out); put_cpu();
> + /* Save SEAMCALL return code if the caller wants it */ > + if (seamcall_ret) > + *seamcall_ret = sret; > + > + /* SEAMCALL was successful */ > + if (!sret) > + goto out;
Why not move that into the switch statement below to avoid th goto? If you do the put_cpu() early, you can avoid "ret" as well.
switch (sret) { case 0: /* SEAMCALL was successful */ return 0; case TDX_SEAMCALL_GP: pr_err_once("[firmware bug]: TDX is not enabled by BIOS.\n"); return -ENODEV; ... }
[...]
> + > static int __init record_keyid_partitioning(u32 *tdx_keyid_start, > u32 *nr_tdx_keyids) > { > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > new file mode 100644 > index 000000000000..48ad1a1ba737 > --- /dev/null > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _X86_VIRT_TDX_H > +#define _X86_VIRT_TDX_H > + > +#include <linux/types.h> > + > +struct tdx_module_output; > +u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > + struct tdx_module_output *out); > +#endif > diff --git a/arch/x86/virt/vmx/tdx/tdxcall.S b/arch/x86/virt/vmx/tdx/tdxcall.S > index 49a54356ae99..757b0c34be10 100644 > --- a/arch/x86/virt/vmx/tdx/tdxcall.S > +++ b/arch/x86/virt/vmx/tdx/tdxcall.S > @@ -1,6 +1,7 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > #include <asm/asm-offsets.h> > #include <asm/tdx.h> > +#include <asm/asm.h> > > /* > * TDCALL and SEAMCALL are supported in Binutils >= 2.36. > @@ -45,6 +46,7 @@ > /* Leave input param 2 in RDX */ > > .if \host > +1: > seamcall > /* > * SEAMCALL instruction is essentially a VMExit from VMX root > @@ -57,10 +59,23 @@ > * This value will never be used as actual SEAMCALL error code as > * it is from the Reserved status code class. > */ > - jnc .Lno_vmfailinvalid > + jnc .Lseamcall_out > mov $TDX_SEAMCALL_VMFAILINVALID, %rax > -.Lno_vmfailinvalid: > + jmp .Lseamcall_out > +2: > + /* > + * SEAMCALL caused #GP or #UD. By reaching here %eax contains > + * the trap number. Convert the trap number to the TDX error > + * code by setting TDX_SW_ERROR to the high 32-bits of %rax. > + * > + * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction > + * only accepts 32-bit immediate at most.
Not sure if that comment is really helpful here. It's a common pattern for large immediates, no?
> + */ > + mov $TDX_SW_ERROR, %r12 > + orq %r12, %rax > > + _ASM_EXTABLE_FAULT(1b, 2b) > +.Lseamcall_out: > .else > tdcall > .endif
-- Cheers,
David / dhildenb
| |