Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Chenyi Qiang <chenyi.qiang@intel.com>]

On 9/3/2024 1:44 PM, Tony Lindgren wrote:
> On Tue, Sep 03, 2024 at 10:58:11AM +0800, Chenyi Qiang wrote:
>> On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
>>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>> @@ -543,10 +664,23 @@ static int __tdx_td_init(struct kvm *kvm)
>>>  		}
>>>  	}
>>>  
>>> -	/*
>>> -	 * Note, TDH_MNG_INIT cannot be invoked here.  TDH_MNG_INIT requires a dedicated
>>> -	 * ioctl() to define the configure CPUID values for the TD.
>>> -	 */
>>> +	err = tdh_mng_init(kvm_tdx, __pa(td_params), &rcx);
>>> +	if ((err & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_INVALID) {
>>> +		/*
>>> +		 * Because a user gives operands, don't warn.
>>> +		 * Return a hint to the user because it's sometimes hard for the
>>> +		 * user to figure out which operand is invalid.  SEAMCALL status
>>> +		 * code includes which operand caused invalid operand error.
>>> +		 */
>>> +		*seamcall_err = err;
>>
>> I'm wondering if we could return or output more hint (i.e. the value of
>> rcx) in the case of invalid operand. For example, if seamcall returns
>> with INVALID_OPERAND_CPUID_CONFIG, rcx will contain the CPUID
>> leaf/sub-leaf info.
> 
> Printing a decriptive error here would be nice when things go wrong.
> Probably no need to return that information.
> 
> Sounds like you have a patch already in mind though :) Care to post a
> patch against the current kvm-coco branch? If not, I can do it after all
> the obvious comment changes are out of the way.

According to the comment above, this patch wants to return the hint to
user as the user gives operands. I'm still uncertain if we should follow
this to return value in some way or special-case the
INVALID_OPERAND_CPUID_CONFIG like:

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index c00c73b2ad4c..dd6e3149ff5a 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2476,8 +2476,14 @@ static int __tdx_td_init(struct kvm *kvm, struct
td_params *td_params,
                 * Return a hint to the user because it's sometimes hard
for the
                 * user to figure out which operand is invalid.
SEAMCALL status
                 * code includes which operand caused invalid operand error.
+                *
+                * TDX_OPERAND_INVALID_CPUID_CONFIG contains more info
+                * in rcx (i.e. leaf/sub-leaf), warn it to help figure
+                * out the invalid CPUID config.
                 */
                *seamcall_err = err;
+               if (err == (TDX_OPERAND_INVALID |
TDX_OPERAND_ID_CPUID_CONFIG))
+                       pr_tdx_error_1(TDH_MNG_INIT, err, rcx);
                ret = -EINVAL;
                goto teardown;
        } else if (WARN_ON_ONCE(err)) {
diff --git a/arch/x86/kvm/vmx/tdx_errno.h b/arch/x86/kvm/vmx/tdx_errno.h
index f9dbb3a065cc..311c3f03d398 100644
--- a/arch/x86/kvm/vmx/tdx_errno.h
+++ b/arch/x86/kvm/vmx/tdx_errno.h
@@ -30,6 +30,7 @@
  * detail information
  */
 #define TDX_OPERAND_ID_RCX                     0x01
+#define TDX_OPERAND_ID_CPUID_CONFIG            0x45
 #define TDX_OPERAND_ID_TDR                     0x80
 #define TDX_OPERAND_ID_SEPT                    0x92
 #define TDX_OPERAND_ID_TD_EPOCH                        0xa9

> 
> Regards,
> 
> Tony