Received-SPF: pass (google.com: domain of linux-kernel+bounces-112101-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 22 Mar 2024 16:36:58 -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>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 035/130] KVM: TDX: Add place holder for TDX VM
 specific mem_enc_op ioctl
Message-ID: <20240322233658.GG1994522@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <079540d563ab0f5d8991ad4d3b1546c05dc2fb01.1708933498.git.isaku.yamahata@intel.com>
 <9c35ecd7-e737-441a-99ff-27bda2a9b25d@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <9c35ecd7-e737-441a-99ff-27bda2a9b25d@intel.com>

On Fri, Mar 22, 2024 at 11:10:48AM +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 45b2c2304491..9ea46d143bef 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -567,6 +567,32 @@ struct kvm_pmu_event_filter {
> >   #define KVM_X86_TDX_VM		2
> >   #define KVM_X86_SNP_VM		3
> > +/* Trust Domain eXtension sub-ioctl() commands. */
> > +enum kvm_tdx_cmd_id {
> > +	KVM_TDX_CAPABILITIES = 0,
> > +
> > +	KVM_TDX_CMD_NR_MAX,
> > +};
> > +
> > +struct kvm_tdx_cmd {
> > +	/* enum kvm_tdx_cmd_id */
> > +	__u32 id;
> > +	/* flags for sub-commend. If sub-command doesn't use this, set zero. */
> > +	__u32 flags;
> > +	/*
> > +	 * data for each sub-command. An immediate or a pointer to the actual
> > +	 * data in process virtual address.  If sub-command doesn't use it,
> > +	 * set zero.
> > +	 */
> > +	__u64 data;
> > +	/*
> > +	 * Auxiliary error code.  The sub-command may return TDX SEAMCALL
> > +	 * status code in addition to -Exxx.
> > +	 * Defined for consistency with struct kvm_sev_cmd.
> > +	 */
> > +	__u64 error;
> 
> If the 'error' is for SEAMCALL error, should we rename it to 'hw_error' or
> 'fw_error' or something similar? I think 'error' is too generic.

Ok, will rename it to hw_error.


> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 5edfb99abb89..07a3f0f75f87 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -55,6 +55,32 @@ struct tdx_info {
> >   /* Info about the TDX module. */
> >   static struct tdx_info *tdx_info;
> > +int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
> > +{
> > +	struct kvm_tdx_cmd tdx_cmd;
> > +	int r;
> > +
> > +	if (copy_from_user(&tdx_cmd, argp, sizeof(struct kvm_tdx_cmd)))
> > +		return -EFAULT;
> 
> Add an empty line.
> 
> > +	if (tdx_cmd.error)
> > +		return -EINVAL;
> 
> Add a comment?
> 
> 	/*
> 	 * Userspace should never set @error, which is used to fill
> 	 * hardware-defined error by the kernel.
> 	 */

Sure.


> > +
> > +	mutex_lock(&kvm->lock);
> > +
> > +	switch (tdx_cmd.id) {
> > +	default:
> > +		r = -EINVAL;
> 
> I am not sure whether you should return -ENOTTY to be consistent with the
> previous vt_mem_enc_ioctl() where a TDX-specific IOCTL is issued for non-TDX
> guest.
>
> Here I think the invalid @id means the sub-command isn't valid.

vt_vcpu_mem_enc_ioctl() checks non-TDX case and returns -ENOTTY.  We know that
the guest is TD.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>