| Message ID | 20240812224820.34826-10-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX vCPU/VM creation | expand |
On 8/13/2024 6:48 AM, Rick Edgecombe wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > TDX KVM needs system-wide information about the TDX module, store it in > struct tdx_info. Release the allocated memory on module unloading by > hardware_unsetup() callback. It seems the shortlog and changelog are stale or mismatched. > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > uAPI breakout v1: > - Mention about hardware_unsetup(). (Binbin) > - Added Reviewed-by. (Binbin) > - Eliminated tdx_md_read(). (Kai) > - Include "x86_ops.h" to tdx.c as the patch to initialize TDX module > doesn't include it anymore. > - Introduce tdx_vm_ioctl() as the first tdx func in x86_ops.h > > v19: > - Added features0 > - Use tdx_sys_metadata_read() > - Fix error recovery path by Yuan > > Change v18: > - Newly Added > --- > arch/x86/include/uapi/asm/kvm.h | 28 +++++++++++++ > arch/x86/kvm/vmx/tdx.c | 70 +++++++++++++++++++++++++++++++++ > 2 files changed, 98 insertions(+) > [...] > + > #endif /* _ASM_X86_KVM_H */ > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c > index de14e80d8f3a..90b44ebaf864 100644 > --- a/arch/x86/kvm/vmx/tdx.c > +++ b/arch/x86/kvm/vmx/tdx.c > @@ -3,6 +3,7 @@ > #include <asm/tdx.h> > #include "capabilities.h" > #include "x86_ops.h" > +#include "mmu.h" Is this needed by this patch? > #include "tdx.h" > > #undef pr_fmt > @@ -30,6 +31,72 @@ static void __used tdx_guest_keyid_free(int keyid) > ida_free(&tdx_guest_keyid_pool, keyid); > } > > [...]
On 8/13/2024 6:48 AM, Rick Edgecombe wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > TDX KVM needs system-wide information about the TDX module, store it in > struct tdx_info. Release the allocated memory on module unloading by > hardware_unsetup() callback. > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > uAPI breakout v1: > - Mention about hardware_unsetup(). (Binbin) > - Added Reviewed-by. (Binbin) > - Eliminated tdx_md_read(). (Kai) > - Include "x86_ops.h" to tdx.c as the patch to initialize TDX module > doesn't include it anymore. > - Introduce tdx_vm_ioctl() as the first tdx func in x86_ops.h > > v19: > - Added features0 > - Use tdx_sys_metadata_read() > - Fix error recovery path by Yuan > > Change v18: > - Newly Added > --- > arch/x86/include/uapi/asm/kvm.h | 28 +++++++++++++ > arch/x86/kvm/vmx/tdx.c | 70 +++++++++++++++++++++++++++++++++ > 2 files changed, 98 insertions(+) > > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h > index d91f1bad800e..47caf508cca7 100644 > --- a/arch/x86/include/uapi/asm/kvm.h > +++ b/arch/x86/include/uapi/asm/kvm.h > @@ -952,4 +952,32 @@ struct kvm_tdx_cmd { > __u64 hw_error; > }; > > +#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) > + > +struct kvm_tdx_cpuid_config { > + __u32 leaf; > + __u32 sub_leaf; > + __u32 eax; > + __u32 ebx; > + __u32 ecx; > + __u32 edx; > +}; I am wondering if there is any specific reason to define a new structure instead of using 'struct kvm_cpuid_entry2'? > + > +/* 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[]; > +}; > +
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c > index de14e80d8f3a..90b44ebaf864 100644 > --- a/arch/x86/kvm/vmx/tdx.c > +++ b/arch/x86/kvm/vmx/tdx.c > @@ -3,6 +3,7 @@ > #include <asm/tdx.h> > #include "capabilities.h" > #include "x86_ops.h" > +#include "mmu.h" Is the header file still needed? > #include "tdx.h" > > #undef pr_fmt > @@ -30,6 +31,72 @@ static void __used tdx_guest_keyid_free(int keyid) > ida_free(&tdx_guest_keyid_pool, keyid); > } > > +static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) > +{ > + const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; > + struct kvm_tdx_capabilities __user *user_caps; > + struct kvm_tdx_capabilities *caps = NULL; > + int i, ret = 0; > + > + /* flags is reserved for future use */ > + if (cmd->flags) > + return -EINVAL; > + > + caps = kmalloc(sizeof(*caps), GFP_KERNEL); > + if (!caps) > + return -ENOMEM; > + > + user_caps = u64_to_user_ptr(cmd->data); > + if (copy_from_user(caps, user_caps, sizeof(*caps))) { > + ret = -EFAULT; > + goto out; > + } > + > + if (caps->nr_cpuid_configs < td_conf->num_cpuid_config) { > + ret = -E2BIG; How about output the correct num_cpuid_config to userspace as a hint, to avoid user blindly retries. > + goto out; > + } > + > + *caps = (struct kvm_tdx_capabilities) { > + .attrs_fixed0 = td_conf->attributes_fixed0, > + .attrs_fixed1 = td_conf->attributes_fixed1, > + .xfam_fixed0 = td_conf->xfam_fixed0, > + .xfam_fixed1 = td_conf->xfam_fixed1, > + .supported_gpaw = TDX_CAP_GPAW_48 | > + ((kvm_host.maxphyaddr >= 52 && > + cpu_has_vmx_ept_5levels()) ? TDX_CAP_GPAW_52 : 0), > + .nr_cpuid_configs = td_conf->num_cpuid_config, > + .padding = 0, > + }; > + > + if (copy_to_user(user_caps, caps, sizeof(*caps))) { > + ret = -EFAULT; > + goto out; > + } > + > + for (i = 0; i < td_conf->num_cpuid_config; i++) { > + struct kvm_tdx_cpuid_config cpuid_config = { > + .leaf = (u32)td_conf->cpuid_config_leaves[i], > + .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, > + .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, > + .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, > + .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, > + .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, > + }; > + > + if (copy_to_user(&(user_caps->cpuid_configs[i]), &cpuid_config, ^ ^ I think the brackets could be removed. > + sizeof(struct kvm_tdx_cpuid_config))) { sizeof(cpuid_config) could be better. Thanks, Yilun
On Wed, 2024-08-14 at 14:18 +0800, Binbin Wu wrote: > > +#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) > > + > > +struct kvm_tdx_cpuid_config { > > + __u32 leaf; > > + __u32 sub_leaf; > > + __u32 eax; > > + __u32 ebx; > > + __u32 ecx; > > + __u32 edx; > > +}; > > I am wondering if there is any specific reason to define a new structure > instead of using 'struct kvm_cpuid_entry2'? GOod question. I don't think so.
On Wed, Aug 21, 2024 at 12:11:16AM +0000, Edgecombe, Rick P wrote: > On Wed, 2024-08-14 at 14:18 +0800, Binbin Wu wrote: > > > +#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) > > > + > > > +struct kvm_tdx_cpuid_config { > > > + __u32 leaf; > > > + __u32 sub_leaf; > > > + __u32 eax; > > > + __u32 ebx; > > > + __u32 ecx; > > > + __u32 edx; > > > +}; > > > > I am wondering if there is any specific reason to define a new structure > > instead of using 'struct kvm_cpuid_entry2'? > > GOod question. I don't think so. I'll do a patch for this. Regards, Tony
On Tue, Aug 13, 2024 at 02:47:17PM +0800, Binbin Wu wrote: > On 8/13/2024 6:48 AM, Rick Edgecombe wrote: > > From: Isaku Yamahata <isaku.yamahata@intel.com> > > --- a/arch/x86/kvm/vmx/tdx.c > > +++ b/arch/x86/kvm/vmx/tdx.c > > @@ -3,6 +3,7 @@ > > #include <asm/tdx.h> > > #include "capabilities.h" > > #include "x86_ops.h" > > +#include "mmu.h" > > Is this needed by this patch? Needed but looks like it should have been introduced only in patch "KVM: x86: Introduce KVM_TDX_GET_CPUID" for kvm_gfn_direct_bits(). Regards, Tony
On Thu, Aug 15, 2024 at 03:59:26PM +0800, Xu Yilun wrote: > > --- a/arch/x86/kvm/vmx/tdx.c > > +++ b/arch/x86/kvm/vmx/tdx.c > > @@ -3,6 +3,7 @@ > > #include <asm/tdx.h> > > #include "capabilities.h" > > #include "x86_ops.h" > > +#include "mmu.h" > > Is the header file still needed? It's needed for kvm_gfn_direct_bits(), but should have been added in a later patch. > > +static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) > > +{ > > + const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; > > + struct kvm_tdx_capabilities __user *user_caps; > > + struct kvm_tdx_capabilities *caps = NULL; > > + int i, ret = 0; > > + > > + /* flags is reserved for future use */ > > + if (cmd->flags) > > + return -EINVAL; > > + > > + caps = kmalloc(sizeof(*caps), GFP_KERNEL); > > + if (!caps) > > + return -ENOMEM; > > + > > + user_caps = u64_to_user_ptr(cmd->data); > > + if (copy_from_user(caps, user_caps, sizeof(*caps))) { > > + ret = -EFAULT; > > + goto out; > > + } > > + > > + if (caps->nr_cpuid_configs < td_conf->num_cpuid_config) { > > + ret = -E2BIG; > > How about output the correct num_cpuid_config to userspace as a hint, > to avoid user blindly retries. Hmm do we want to add also positive numbers for errors for this function? > > + for (i = 0; i < td_conf->num_cpuid_config; i++) { > > + struct kvm_tdx_cpuid_config cpuid_config = { > > + .leaf = (u32)td_conf->cpuid_config_leaves[i], > > + .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, > > + .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, > > + .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, > > + .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, > > + .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, > > + }; > > + > > + if (copy_to_user(&(user_caps->cpuid_configs[i]), &cpuid_config, > ^ ^ > > I think the brackets could be removed. > > > + sizeof(struct kvm_tdx_cpuid_config))) { > > sizeof(cpuid_config) could be better. Looks like these both already changed in a later patch "KVM: TDX: Report kvm_tdx_caps in KVM_TDX_CAPABILITIES". Regards, Tony
> > > +static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) > > > +{ > > > + const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; > > > + struct kvm_tdx_capabilities __user *user_caps; > > > + struct kvm_tdx_capabilities *caps = NULL; > > > + int i, ret = 0; > > > + > > > + /* flags is reserved for future use */ > > > + if (cmd->flags) > > > + return -EINVAL; > > > + > > > + caps = kmalloc(sizeof(*caps), GFP_KERNEL); > > > + if (!caps) > > > + return -ENOMEM; > > > + > > > + user_caps = u64_to_user_ptr(cmd->data); > > > + if (copy_from_user(caps, user_caps, sizeof(*caps))) { > > > + ret = -EFAULT; > > > + goto out; > > > + } > > > + > > > + if (caps->nr_cpuid_configs < td_conf->num_cpuid_config) { > > > + ret = -E2BIG; > > > > How about output the correct num_cpuid_config to userspace as a hint, > > to avoid user blindly retries. > > Hmm do we want to add also positive numbers for errors for this function? No. I think maybe update the user_caps->nr_cpuid_configs when returning -E2BIG. Similar to KVM_GET_MSR_INDEX_LIST. Thanks, Yilun
On Mon, Sep 02, 2024 at 09:25:00AM +0800, Xu Yilun wrote: > > > > +static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) > > > > +{ > > > > + const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; > > > > + struct kvm_tdx_capabilities __user *user_caps; > > > > + struct kvm_tdx_capabilities *caps = NULL; > > > > + int i, ret = 0; > > > > + > > > > + /* flags is reserved for future use */ > > > > + if (cmd->flags) > > > > + return -EINVAL; > > > > + > > > > + caps = kmalloc(sizeof(*caps), GFP_KERNEL); > > > > + if (!caps) > > > > + return -ENOMEM; > > > > + > > > > + user_caps = u64_to_user_ptr(cmd->data); > > > > + if (copy_from_user(caps, user_caps, sizeof(*caps))) { > > > > + ret = -EFAULT; > > > > + goto out; > > > > + } > > > > + > > > > + if (caps->nr_cpuid_configs < td_conf->num_cpuid_config) { > > > > + ret = -E2BIG; > > > > > > How about output the correct num_cpuid_config to userspace as a hint, > > > to avoid user blindly retries. > > > > Hmm do we want to add also positive numbers for errors for this function? > > No. I think maybe update the user_caps->nr_cpuid_configs when returning > -E2BIG. Similar to KVM_GET_MSR_INDEX_LIST. OK thanks for clarifying, yes that sounds nice. Regards, Tony
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h index d91f1bad800e..47caf508cca7 100644 --- a/arch/x86/include/uapi/asm/kvm.h +++ b/arch/x86/include/uapi/asm/kvm.h @@ -952,4 +952,32 @@ struct kvm_tdx_cmd { __u64 hw_error; }; +#define KVM_TDX_CPUID_NO_SUBLEAF ((__u32)-1) + +struct kvm_tdx_cpuid_config { + __u32 leaf; + __u32 sub_leaf; + __u32 eax; + __u32 ebx; + __u32 ecx; + __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[]; +}; + #endif /* _ASM_X86_KVM_H */ diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index de14e80d8f3a..90b44ebaf864 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -3,6 +3,7 @@ #include <asm/tdx.h> #include "capabilities.h" #include "x86_ops.h" +#include "mmu.h" #include "tdx.h" #undef pr_fmt @@ -30,6 +31,72 @@ static void __used tdx_guest_keyid_free(int keyid) ida_free(&tdx_guest_keyid_pool, keyid); } +static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) +{ + const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf; + struct kvm_tdx_capabilities __user *user_caps; + struct kvm_tdx_capabilities *caps = NULL; + int i, ret = 0; + + /* flags is reserved for future use */ + if (cmd->flags) + return -EINVAL; + + caps = kmalloc(sizeof(*caps), GFP_KERNEL); + if (!caps) + return -ENOMEM; + + user_caps = u64_to_user_ptr(cmd->data); + if (copy_from_user(caps, user_caps, sizeof(*caps))) { + ret = -EFAULT; + goto out; + } + + if (caps->nr_cpuid_configs < td_conf->num_cpuid_config) { + ret = -E2BIG; + goto out; + } + + *caps = (struct kvm_tdx_capabilities) { + .attrs_fixed0 = td_conf->attributes_fixed0, + .attrs_fixed1 = td_conf->attributes_fixed1, + .xfam_fixed0 = td_conf->xfam_fixed0, + .xfam_fixed1 = td_conf->xfam_fixed1, + .supported_gpaw = TDX_CAP_GPAW_48 | + ((kvm_host.maxphyaddr >= 52 && + cpu_has_vmx_ept_5levels()) ? TDX_CAP_GPAW_52 : 0), + .nr_cpuid_configs = td_conf->num_cpuid_config, + .padding = 0, + }; + + if (copy_to_user(user_caps, caps, sizeof(*caps))) { + ret = -EFAULT; + goto out; + } + + for (i = 0; i < td_conf->num_cpuid_config; i++) { + struct kvm_tdx_cpuid_config cpuid_config = { + .leaf = (u32)td_conf->cpuid_config_leaves[i], + .sub_leaf = td_conf->cpuid_config_leaves[i] >> 32, + .eax = (u32)td_conf->cpuid_config_values[i].eax_ebx, + .ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32, + .ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx, + .edx = td_conf->cpuid_config_values[i].ecx_edx >> 32, + }; + + if (copy_to_user(&(user_caps->cpuid_configs[i]), &cpuid_config, + sizeof(struct kvm_tdx_cpuid_config))) { + ret = -EFAULT; + break; + } + } + +out: + /* kfree() accepts NULL. */ + kfree(caps); + return ret; +} + int tdx_vm_ioctl(struct kvm *kvm, void __user *argp) { struct kvm_tdx_cmd tdx_cmd; @@ -48,6 +115,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;