Re: [PATCH v19 036/130] KVM: TDX: x86: Add ioctl to get TDX systemwide parameters

[Isaku Yamahata]

On Fri, Mar 22, 2024 at 11:26:17AM +1300,
"Huang, Kai" <kai.huang@xxxxxxxxx> wrote:

> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index 9ea46d143bef..e28189c81691 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -604,4 +604,21 @@ struct kvm_tdx_cpuid_config {
> >   	__u32 edx;
> >   };
> > +/* supported_gpaw */
> > +#define TDX_CAP_GPAW_48	(1 << 0)
> > +#define TDX_CAP_GPAW_52	(1 << 1)
> > +
> > +struct kvm_tdx_capabilities {
> > +	__u64 attrs_fixed0;
> > +	__u64 attrs_fixed1;
> > +	__u64 xfam_fixed0;
> > +	__u64 xfam_fixed1;
> > +	__u32 supported_gpaw;
> > +	__u32 padding;
> > +	__u64 reserved[251];
> > +
> > +	__u32 nr_cpuid_configs;
> > +	struct kvm_tdx_cpuid_config cpuid_configs[];
> > +};
> > +
> 
> I think you should use __DECLARE_FLEX_ARRAY().
> 
> It's already used in existing KVM UAPI header:
> 
> struct kvm_nested_state {
> 	...
>         union {
>                 __DECLARE_FLEX_ARRAY(struct kvm_vmx_nested_state_data,
> 					 vmx);
>                 __DECLARE_FLEX_ARRAY(struct kvm_svm_nested_state_data,
> 					 svm);
>         } data;
> }

Yes, will use it.


> > +	if (copy_to_user(user_caps->cpuid_configs, &tdx_info->cpuid_configs,
> > +			 tdx_info->num_cpuid_config *
> > +			 sizeof(tdx_info->cpuid_configs[0]))) {
> > +		ret = -EFAULT;
> > +	}
> 
> I think the '{ }' is needed here.

Unnecessary? Will remove braces.


> > +
> > +out:
> > +	/* kfree() accepts NULL. */
> > +	kfree(caps);
> > +	return ret;
> > +}
> > +
> >   int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
> >   {
> >   	struct kvm_tdx_cmd tdx_cmd;
> > @@ -68,6 +121,9 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
> >   	mutex_lock(&kvm->lock);
> >   	switch (tdx_cmd.id) {
> > +	case KVM_TDX_CAPABILITIES:
> > +		r = tdx_get_capabilities(&tdx_cmd);
> > +		break;
> >   	default:
> >   		r = -EINVAL;
> >   		goto out;
> > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > index 473013265bd8..22c0b57f69ca 100644
> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -3,6 +3,9 @@
> >   #define __KVM_X86_TDX_H
> >   #ifdef CONFIG_INTEL_TDX_HOST
> > +
> > +#include "tdx_ops.h"
> > +
> 
> It appears "tdx_ops.h" is used for making SEAMCALLs.
> 
> I don't see this patch uses any SEAMCALL so I am wondering whether this
> chunk is needed here?

Will remove it to move it to an appropriate patch
-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>