lkml.org 
[lkml]   [2022]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv4 03/30] x86/tdx: Provide common base for SEAMCALL and TDCALL C wrappers
    ...
    > arch/x86/include/asm/tdx.h | 20 ++++++++
    > arch/x86/kernel/asm-offsets.c | 9 ++++
    > arch/x86/virt/tdxcall.S | 91 +++++++++++++++++++++++++++++++++++
    > 3 files changed, 120 insertions(+)
    > create mode 100644 arch/x86/virt/tdxcall.S
    >
    > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
    > index ba8042ce61c2..2f8cb1e53e77 100644
    > --- a/arch/x86/include/asm/tdx.h
    > +++ b/arch/x86/include/asm/tdx.h
    > @@ -8,6 +8,25 @@
    > #define TDX_CPUID_LEAF_ID 0x21
    > #define TDX_IDENT "IntelTDX "
    >
    > +#define TDX_SEAMCALL_VMFAILINVALID 0x8000FF00FFFF0000ULL

    This needs a comment. It's a completely software-defined value, right?

    > +#ifndef __ASSEMBLY__
    > +
    > +/*
    > + * Used to gather the output registers values of the TDCALL and SEAMCALL
    > + * instructions when requesting services from the TDX module.
    > + *
    > + * This is a software only structure and not part of the TDX module/VMM ABI.
    > + */
    > +struct tdx_module_output {
    > + u64 rcx;
    > + u64 rdx;
    > + u64 r8;
    > + u64 r9;
    > + u64 r10;
    > + u64 r11;
    > +};
    > +
    > #ifdef CONFIG_INTEL_TDX_GUEST
    >
    > void __init tdx_early_init(void);
    > @@ -18,4 +37,5 @@ static inline void tdx_early_init(void) { };
    >
    > #endif /* CONFIG_INTEL_TDX_GUEST */
    >
    > +#endif /* !__ASSEMBLY__ */
    > #endif /* _ASM_X86_TDX_H */
    > diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
    > index 9fb0a2f8b62a..7dca52f5cfc6 100644
    > --- a/arch/x86/kernel/asm-offsets.c
    > +++ b/arch/x86/kernel/asm-offsets.c
    > @@ -18,6 +18,7 @@
    > #include <asm/bootparam.h>
    > #include <asm/suspend.h>
    > #include <asm/tlbflush.h>
    > +#include <asm/tdx.h>
    >
    > #ifdef CONFIG_XEN
    > #include <xen/interface/xen.h>
    > @@ -65,6 +66,14 @@ static void __used common(void)
    > OFFSET(XEN_vcpu_info_arch_cr2, vcpu_info, arch.cr2);
    > #endif
    >
    > + BLANK();
    > + OFFSET(TDX_MODULE_rcx, tdx_module_output, rcx);
    > + OFFSET(TDX_MODULE_rdx, tdx_module_output, rdx);
    > + OFFSET(TDX_MODULE_r8, tdx_module_output, r8);
    > + OFFSET(TDX_MODULE_r9, tdx_module_output, r9);
    > + OFFSET(TDX_MODULE_r10, tdx_module_output, r10);
    > + OFFSET(TDX_MODULE_r11, tdx_module_output, r11);
    > +
    > BLANK();
    > OFFSET(BP_scratch, boot_params, scratch);
    > OFFSET(BP_secure_boot, boot_params, secure_boot);
    > diff --git a/arch/x86/virt/tdxcall.S b/arch/x86/virt/tdxcall.S
    > new file mode 100644
    > index 000000000000..90569faedacc
    > --- /dev/null
    > +++ b/arch/x86/virt/tdxcall.S
    > @@ -0,0 +1,91 @@
    > +/* SPDX-License-Identifier: GPL-2.0 */
    > +#include <asm/asm-offsets.h>
    > +#include <asm/tdx.h>
    > +
    > +/*
    > + * TDX guests use the TDCALL instruction to make requests to the
    > + * TDX module and hypercalls to the VMM.
    > + *
    > + * TDX host user SEAMCALL instruction to make requests to TDX module.

    ^ uses??


    > + * They are supported in Binutils >= 2.36.
    > + */

    While you're in there, could we make this a little more structured, please?

    /*
    * TDX_MODULE_CALL - common helper macro for both
    * TDCALL and SEAMCALL instructions.
    *
    * TDCALL - used by TDX guests to make requests to the
    * TDX module and hypercalls to the VMM.
    * SEAMCALL - used by TDX hosts to make requests to the
    * TDX module.
    *
    * Both instruction are supported in Binutils >= 2.36.
    */

    > +#define tdcall .byte 0x66,0x0f,0x01,0xcc
    > +#define seamcall .byte 0x66,0x0f,0x01,0xcf
    > +
    > +.macro TDX_MODULE_CALL host:req
    > + /*
    > + * R12 will be used as temporary storage for struct tdx_module_output
    > + * pointer. Since R12-R15 registers are not used by TDCALL/SEAMCALL
    > + * services supported by this function, it can be reused.
    > + */
    > +
    > + /* Callee saved, so preserve it */
    > + push %r12
    > +
    > + /*
    > + * Push output pointer to stack.
    > + * After the operation, it will be fetched into R12 register.
    > + */
    > + push %r9
    > +
    > + /* Mangle function call ABI into TDCALL/SEAMCALL ABI: */
    > + /* Move Leaf ID to RAX */
    > + mov %rdi, %rax
    > + /* Move input 4 to R9 */
    > + mov %r8, %r9
    > + /* Move input 3 to R8 */
    > + mov %rcx, %r8
    > + /* Move input 1 to RCX */
    > + mov %rsi, %rcx
    > + /* Leave input param 2 in RDX */
    > +
    > + .if \host
    > + seamcall
    > + /*
    > + * SEAMCALL instruction is essentially a VMExit from VMX root
    > + * mode to SEAM VMX root mode. VMfailInvalid (CF=1) indicates
    > + * that the targeted SEAM firmware is not loaded or disabled,
    > + * or P-SEAMLDR is busy with another SEAMCALL. %rax is not
    > + * changed in this case.
    > + *
    > + * Set %rax to TDX_SEAMCALL_VMFAILINVALID for VMfailInvalid.
    > + * This value will never be used as actual SEAMCALL error code.
    > + */

    Why can TDX_SEAMCALL_VMFAILINVALID never be used as an actual SEAMCALL
    error code? Would it be more accurate to say that today's P-SEAMLDR and
    TDX module never use TDX_SEAMCALL_VMFAILINVALID as an actual SEAMCALL
    error code?

    There's an important distinction there. It's possible that the SEAMCALL
    instruction architecture says or implies that it can never have
    TDX_SEAMCALL_VMFAILINVALID as a return value. It's also possible that
    this "never" comes from a guess about what P-SEAMLDRs or TDX modules can do.

    > + jnc .Lno_vmfailinvalid
    > + mov $TDX_SEAMCALL_VMFAILINVALID, %rax
    > +.Lno_vmfailinvalid:
    > + .else
    > + tdcall
    > + .endif
    > +
    > + /*
    > + * Fetch output pointer from stack to R12 (It is used
    > + * as temporary storage)
    > + */
    > + pop %r12
    > +
    > + /* Check for success: 0 - Successful, otherwise failed */
    > + test %rax, %rax
    > + jnz .Lno_output_struct
    > +
    > + /*
    > + * Since this function can be initiated without an output pointer,
    > + * check if caller provided an output struct before storing
    > + * output registers.
    > + */
    > + test %r12, %r12
    > + jz .Lno_output_struct
    > +
    > + /* Copy result registers to output struct: */
    > + movq %rcx, TDX_MODULE_rcx(%r12)
    > + movq %rdx, TDX_MODULE_rdx(%r12)
    > + movq %r8, TDX_MODULE_r8(%r12)
    > + movq %r9, TDX_MODULE_r9(%r12)
    > + movq %r10, TDX_MODULE_r10(%r12)
    > + movq %r11, TDX_MODULE_r11(%r12)
    > +
    > +.Lno_output_struct:
    > + /* Restore the state of R12 register */
    > + pop %r12
    > +.endm

    That doesn't look too bad. Please just send an updated version with the
    revised comments.

    \
     
     \ /
      Last update: 2022-02-24 17:36    [W:8.432 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site