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

[Nikolay Borisov <nik.borisov@suse.com>]

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>