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>
