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@xxxxxxxxx>
> >>> @@ -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
