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

[Tony Lindgren <tony.lindgren@linux.intel.com>]

On Tue, Sep 03, 2024 at 04:04:47PM +0800, Chenyi Qiang wrote:
> 
> 
> 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
> 

OK yes that should take care of the issue, I doubt that this can be
automatically be handled by the caller even a better error code
was returned.

Regards,

Tony