Re: [PATCH 08/10] x86/tdx: Unify TDX_HYPERCALL and TDX_MODULE_CALL assembly

[Huang, Kai]

> > +/* 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 ?

W/o this patch, the __tdx_hypercall_failed() is called from the TDX_HYPERCALL
assembly, which is in .noinstr.text, and 'instrumentation_begin()' was needed to
avoid the build warning I suppose.

However now with this patch __tdx_hypercall_failed() is called from
__tdx_hypercall() which is a C function w/o 'noinstr' annotation, thus I believe
instrumentation_begin() and 'noinstr' annotation are not needed anymore.

I didn't notice this while moving this function around and my kernel build test
didn't warn me about this.  I'll change in next version.

In fact, perhaps this patch perhaps is too big for review.  I will also try to
split it to smaller ones.

> 
> > +
> > +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 ....

OK will do.  I basically moved the comment from assembly to here.