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.
