Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <77c90075-79d4-7cc7-f266-1b67e586513b@intel.com>
Date:   Mon, 27 Jun 2022 13:46:48 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v5 08/22] x86/virt/tdx: Shut down TDX module in case of
 error
Content-Language: en-US
To:     Kai Huang <kai.huang@intel.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     seanjc@google.com, pbonzini@redhat.com, len.brown@intel.com,
        tony.luck@intel.com, rafael.j.wysocki@intel.com,
        reinette.chatre@intel.com, dan.j.williams@intel.com,
        peterz@infradead.org, ak@linux.intel.com,
        kirill.shutemov@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        isaku.yamahata@intel.com
References: <cover.1655894131.git.kai.huang@intel.com>
 <89fffc70cdbb74c80bb324364b712ec41e5f8b91.1655894131.git.kai.huang@intel.com>
 <765a20f1-681d-33c2-68e9-24cc249fe6f9@intel.com>
 <cc90e5f8be0c6f48a144240d4569b15bd4b75dd8.camel@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <cc90e5f8be0c6f48a144240d4569b15bd4b75dd8.camel@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.