Messages in this thread | | | Date | Sat, 15 Jul 2023 13:05:48 +0300 | From | Nikolay Borisov <> | Subject | Re: [PATCH 08/10] x86/tdx: Unify TDX_HYPERCALL and TDX_MODULE_CALL assembly |
| |
<snip>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c > index 0f16ba52ae62..a5e77893b2c0 100644 > --- a/arch/x86/coco/tdx/tdx.c > +++ b/arch/x86/coco/tdx/tdx.c > @@ -51,13 +51,38 @@ > > #define TDREPORT_SUBTYPE_0 0 > > +/* Called from __tdx_hypercall() for unrecoverable failure */ > +static noinstr void __tdx_hypercall_failed(void) > +{ > + instrumentation_begin(); > + panic("TDVMCALL failed. TDX module bug?"); > +}
So what's the deal with this instrumentation here. The instruction is noinstr, so you want to make just the panic call itself instrumentable?, if so where's the instrumentation_end() cal;?No instrumentation_end() call. Actually is this complexity really worth it for the failure case?
AFAICS there is a single call site for __tdx_hypercall_failed so why noot call panic() directly ?
> + > +static inline u64 __tdx_hypercall(struct tdx_module_args *args) > +{ > + u64 ret; > + > + args->rcx = TDVMCALL_EXPOSE_REGS_MASK; > + ret = __tdcall_saved_ret(TDG_VP_VMCALL, args); > + > + /* > + * RAX!=0 indicates a failure of the TDVMCALL mechanism itself and that
nit: Why mention the register explicitly, just say that if __tdcall_saved_ret returns non-zero ....
> + * something has gone horribly wrong with the TDX module. > + */ > + if (ret) > + __tdx_hypercall_failed(); > + > + /* The return status of the hypercall itself is in R10. */ > + return args->r10; > +} > + > /* > - * Wrapper for standard use of __tdx_hypercall with no output aside from > - * return code. > + * Wrapper for standard use of __tdx_hypercall() w/o needing any output > + * register except the return code. > */ > static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15) > { > - struct tdx_hypercall_args args = { > + struct tdx_module_args args = { > .r10 = TDX_HYPERCALL_STANDARD, > .r11 = fn, > .r12 = r12,
<snip>
| |