diff mbox series

[11/25] KVM: TDX: Report kvm_tdx_caps in KVM_TDX_CAPABILITIES

Message ID 20240812224820.34826-12-rick.p.edgecombe@intel.com (mailing list archive)
State New, archived
Headers show
Series TDX vCPU/VM creation | expand

Commit Message

Edgecombe, Rick P Aug. 12, 2024, 10:48 p.m. UTC 
From: Xiaoyao Li <xiaoyao.li@intel.com>

Report raw capabilities of TDX module to userspace isn't so useful
and incorrect, because some of the capabilities might not be supported
by KVM.

Instead, report the KVM capp'ed capbilities to userspace.

Removed the supported_gpaw field. Because CPUID.0x80000008.EAX[23:16] of
KVM_SUPPORTED_CPUID enumerates the 5 level EPT support, i.e., if GPAW52
is supported or not. Note, GPAW48 should be always supported. Thus no
need for explicit enumeration.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
uAPI breakout v1:
 - Code change due to previous patches changed to use exported 'struct
   tdx_sysinfo' pointer.
---
 arch/x86/include/uapi/asm/kvm.h | 14 +++----------
 arch/x86/kvm/vmx/tdx.c          | 36 ++++++++-------------------------
 2 files changed, 11 insertions(+), 39 deletions(-)

Comments

Chao Gao Aug. 13, 2024, 3:35 a.m. UTC | #1 
On Mon, Aug 12, 2024 at 03:48:06PM -0700, Rick Edgecombe wrote:
>From: Xiaoyao Li <xiaoyao.li@intel.com>
>
>Report raw capabilities of TDX module to userspace isn't so useful
>and incorrect, because some of the capabilities might not be supported
>by KVM.
>
>Instead, report the KVM capp'ed capbilities to userspace.
>
>Removed the supported_gpaw field. Because CPUID.0x80000008.EAX[23:16] of
>KVM_SUPPORTED_CPUID enumerates the 5 level EPT support, i.e., if GPAW52
>is supported or not. Note, GPAW48 should be always supported. Thus no
>need for explicit enumeration.
>
>Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>---
>uAPI breakout v1:
> - Code change due to previous patches changed to use exported 'struct
>   tdx_sysinfo' pointer.
>---
> arch/x86/include/uapi/asm/kvm.h | 14 +++----------
> arch/x86/kvm/vmx/tdx.c          | 36 ++++++++-------------------------
> 2 files changed, 11 insertions(+), 39 deletions(-)
>
>diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
>index c9eb2e2f5559..2e3caa5a58fd 100644
>--- a/arch/x86/include/uapi/asm/kvm.h
>+++ b/arch/x86/include/uapi/asm/kvm.h
>@@ -961,18 +961,10 @@ 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];
>+	__u64 supported_attrs;
>+	__u64 supported_xfam;
>+	__u64 reserved[254];

I wonder why this patch and patch 9 weren't squashed together. Many changes
added by patch 9 are removed here.

> 
> 	__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 d89973e554f6..f9faec217ea9 100644
>--- a/arch/x86/kvm/vmx/tdx.c
>+++ b/arch/x86/kvm/vmx/tdx.c
>@@ -49,7 +49,7 @@ 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;
>+	int ret = 0;
> 
> 	/* flags is reserved for future use */
> 	if (cmd->flags)
>@@ -70,39 +70,19 @@ static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
> 		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,
>-	};
>+	caps->supported_attrs = kvm_tdx_caps->supported_attrs;
>+	caps->supported_xfam = kvm_tdx_caps->supported_xfam;
>+	caps->nr_cpuid_configs = kvm_tdx_caps->num_cpuid_config;
> 
> 	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;
>-		}
>-	}
>+	if (copy_to_user(user_caps->cpuid_configs, &kvm_tdx_caps->cpuid_configs,
>+			 kvm_tdx_caps->num_cpuid_config *
>+			 sizeof(kvm_tdx_caps->cpuid_configs[0])))
>+		ret = -EFAULT;
> 
> out:
> 	/* kfree() accepts NULL. */
>-- 
>2.34.1
>
>
Nikolay Borisov Aug. 19, 2024, 10:24 a.m. UTC | #2 
On 13.08.24 г. 6:35 ч., Chao Gao wrote:
> On Mon, Aug 12, 2024 at 03:48:06PM -0700, Rick Edgecombe wrote:
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>
>> Report raw capabilities of TDX module to userspace isn't so useful
>> and incorrect, because some of the capabilities might not be supported
>> by KVM.
>>
>> Instead, report the KVM capp'ed capbilities to userspace.
>>
>> Removed the supported_gpaw field. Because CPUID.0x80000008.EAX[23:16] of
>> KVM_SUPPORTED_CPUID enumerates the 5 level EPT support, i.e., if GPAW52
>> is supported or not. Note, GPAW48 should be always supported. Thus no
>> need for explicit enumeration.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>> ---
>> uAPI breakout v1:
>> - Code change due to previous patches changed to use exported 'struct
>>    tdx_sysinfo' pointer.
>> ---
>> arch/x86/include/uapi/asm/kvm.h | 14 +++----------
>> arch/x86/kvm/vmx/tdx.c          | 36 ++++++++-------------------------
>> 2 files changed, 11 insertions(+), 39 deletions(-)
>>
>> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
>> index c9eb2e2f5559..2e3caa5a58fd 100644
>> --- a/arch/x86/include/uapi/asm/kvm.h
>> +++ b/arch/x86/include/uapi/asm/kvm.h
>> @@ -961,18 +961,10 @@ 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];
>> +	__u64 supported_attrs;
>> +	__u64 supported_xfam;
>> +	__u64 reserved[254];
> 
> I wonder why this patch and patch 9 weren't squashed together. Many changes
> added by patch 9 are removed here.

As far as I can see this patch depends on the code in patch 10 
(kvm_tdx_caps) so this patch definitely must come after changes 
introduced in patch 10. However, patch 9 seems completely independent of 
patch 10, so I think patch 10 should become patch 9, and patch 9/11 
should be squashed into one and become patch 10.

<snip>
Edgecombe, Rick P Aug. 21, 2024, 12:06 a.m. UTC | #3 
On Mon, 2024-08-19 at 13:24 +0300, Nikolay Borisov wrote:
> > I wonder why this patch and patch 9 weren't squashed together. Many changes
> > added by patch 9 are removed here.
> 
> As far as I can see this patch depends on the code in patch 10 
> (kvm_tdx_caps) so this patch definitely must come after changes 
> introduced in patch 10. However, patch 9 seems completely independent of 
> patch 10, so I think patch 10 should become patch 9, and patch 9/11 
> should be squashed into one and become patch 10.

Yes, thanks. The patch order needs to be cleaned up. This posting was mostly
intended to try to settle the whole guest CPU feature API design. I probably
should have tagged it RFC instead of just including the coverletter blurb:
   Please feel free to wait for future revisions to spend time trying to correct
   smaller code issues. But I would greatly appreciate discussion on the overall
   design and how we are weighing the tradeoffs for the uAPI.
diff mbox series

Patch

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index c9eb2e2f5559..2e3caa5a58fd 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -961,18 +961,10 @@  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];
+	__u64 supported_attrs;
+	__u64 supported_xfam;
+	__u64 reserved[254];
 
 	__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 d89973e554f6..f9faec217ea9 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -49,7 +49,7 @@  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;
+	int ret = 0;
 
 	/* flags is reserved for future use */
 	if (cmd->flags)
@@ -70,39 +70,19 @@  static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
 		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,
-	};
+	caps->supported_attrs = kvm_tdx_caps->supported_attrs;
+	caps->supported_xfam = kvm_tdx_caps->supported_xfam;
+	caps->nr_cpuid_configs = kvm_tdx_caps->num_cpuid_config;
 
 	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;
-		}
-	}
+	if (copy_to_user(user_caps->cpuid_configs, &kvm_tdx_caps->cpuid_configs,
+			 kvm_tdx_caps->num_cpuid_config *
+			 sizeof(kvm_tdx_caps->cpuid_configs[0])))
+		ret = -EFAULT;
 
 out:
 	/* kfree() accepts NULL. */