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

[Nikolay Borisov]

On 17.07.23 г. 9:35 ч., Huang, Kai wrote:
> > > +/* 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.

Can't you simply call panic() directly? Less going around the code while 
someone is reading it?

<snip>