On Fri, Mar 22, 2024 at 11:10:48AM +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 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@xxxxxxxxx>
