Re: [PATCH v5 08/22] x86/virt/tdx: Shut down TDX module in case of error

[Dave Hansen <dave.hansen@intel.com>]

On 6/26/22 22:26, Kai Huang wrote:
> On Fri, 2022-06-24 at 11:50 -0700, Dave Hansen wrote:
>> So, the last patch was called:
>>
>> 	Implement SEAMCALL function
>>
>> and yet, in this patch, we have a "seamcall()" function.  That's a bit
>> confusing and not covered at *all* in this subject.
>>
>> Further, seamcall() is the *ONLY* caller of __seamcall() that I see in
>> this series.  That makes its presence here even more odd.
>>
>> The seamcall() bits should either be in their own patch, or mashed in
>> with __seamcall().
> 
> Right.  The reason I didn't put the seamcall() into previous patch was it is
> only used in this tdx.c, so it should be static.  But adding a static function
> w/o using it in previous patch will trigger a compile warning.  So I introduced
> here where it is first used.
> 
> One option is I can introduce seamcall() as a static inline function in tdx.h in
> previous patch so there won't be a warning.  I'll change to use this way. 
> Please let me know if you have any comments.

Does a temporary __unused get rid of the warning?

>>>  /*
>>>   * Detect and initialize the TDX module.
>>>   *
>>> @@ -138,7 +195,10 @@ static int init_tdx_module(void)
>>>  
>>>  static void shutdown_tdx_module(void)
>>>  {
>>> -	/* TODO: Shut down the TDX module */
>>> +	struct seamcall_ctx sc = { .fn = TDH_SYS_LP_SHUTDOWN };
>>> +
>>> +	seamcall_on_each_cpu(&sc);
>>> +
>>>  	tdx_module_status = TDX_MODULE_SHUTDOWN;
>>>  }
>>>  
>>> @@ -221,6 +281,9 @@ bool platform_tdx_enabled(void)
>>>   * CPU hotplug is temporarily disabled internally to prevent any cpu
>>>   * from going offline.
>>>   *
>>> + * Caller also needs to guarantee all CPUs are in VMX operation during
>>> + * this function, otherwise Oops may be triggered.
>>
>> I would *MUCH* rather have this be a:
>>
>> 	if (!cpu_feature_enabled(X86_FEATURE_VMX))
>> 		WARN_ONCE("VMX should be on blah blah\n");
>>
>> than just plain oops.  Even a pr_err() that preceded the oops would be
>> nicer than an oops that someone has to go decode and then grumble when
>> their binutils is too old that it can't disassemble the TDCALL.
> 
> I can add this to seamcall():
> 
> 	/*
> 	 * SEAMCALL requires CPU being in VMX operation otherwise it causes
> #UD.
> 	 * Sanity check and return early to avoid Oops.  Note cpu_vmx_enabled()
> 	 * actually only checks whether VMX is enabled but doesn't check
> whether
> 	 * CPU is in VMX operation (VMXON is done).  There's no way to check
> 	 * whether VMXON has been done, but currently enabling VMX and doing
> 	 * VMXON are always done together.
> 	 */
> 	if (!cpu_vmx_enabled())	 {
> 		WARN_ONCE("CPU is not in VMX operation before making
> SEAMCALL");
> 		return -EINVAL;
> 	}
> 
> The reason I didn't do is I'd like to make seamcall() simple, that it only
> returns TDX_SEAMCALL_VMFAILINVALID or the actual SEAMCALL leaf error.  With
> above, this function also returns kernel error code, which isn't good.

I think you're missing the point.  You wasted two lines of code on a
*COMMENT* that doesn't actually help anyone decode an oops.  You could
have, instead, spent two lines on actual code that would have been just
as good or better than a comment *AND* help folks looking at an oops.

It's almost always better to do something actionable in code than to
comment it, unless it's in some crazy fast path.

> Alternatively, we can always add EXTABLE to TDX_MODULE_CALL macro to handle #UD
> and #GP by returning dedicated error codes (please also see my reply to previous
> patch for the code needed to handle), in which case we don't need such check
> here.
> 
> Always handling #UD in TDX_MODULE_CALL macro also has another advantage:  there
> will be no Oops for #UD regardless the issue that "there's no way to check
> whether VMXON has been done" in the above comment.
> 
> What's your opinion?

I think you should explore using the EXTABLE.  Let's see how it looks.