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;
+};
+
+/* 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;
--
2.34.1> 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 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
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[];
> +};
> +
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 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 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
© 2016 - 2026 Red Hat, Inc.