Received-SPF: pass (google.com: domain of linux-kernel+bounces-112135-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 22 Mar 2024 17:28:47 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: "Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	"Aktas, Erdem" <erdemaktas@google.com>,
	Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, "Chen, Bo2" <chen.bo@intel.com>,
	"Yuan, Hang" <hang.yuan@intel.com>,
	"Zhang, Tina" <tina.zhang@intel.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 036/130] KVM: TDX: x86: Add ioctl to get TDX
 systemwide parameters
Message-ID: <20240323002847.GB2357401@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <167f8f7e9b19154d30c7fe8f733f947592eb244c.1708933498.git.isaku.yamahata@intel.com>
 <838fe705-4ebe-43f1-9193-4696baa05aad@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <838fe705-4ebe-43f1-9193-4696baa05aad@intel.com>

On Fri, Mar 22, 2024 at 11:26:17AM +1300,
"Huang, Kai" <kai.huang@intel.com> 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@intel.com>