Skip to contentSkip to conversation

Allow setting RAPL_DIS during SNP_INIT_EX

PATCH In review Tycho Andersen 2 participants 18 messages Feb 23 – Feb 26

Subsystems (6)
KVM Kselftest Crypto API KVM x86 +2 more subsystems
lore
Share & tools
lore
#0 lore raw

cover Allow setting RAPL_DIS during SNP_INIT_EX

From: "Tycho Andersen (AMD)" <[email protected]>

There was support for setting the policy bit, but not the flag during SNP_INIT_EX, which meant VM creation would always fail. Plumb a module parameter for setting the flag during SNP_INIT_EX.

Also clean up some selftests and add a smoke test for RAPL_DISABLE when the module parameter is set appropriately.

Tycho Andersen (AMD) (4):
selftests/kvm: allow retrieving underlying SEV firmware error selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode crypto/ccp: support setting RAPL_DIS in SNP_INIT_EX selftests/kvm: smoke test support for RAPL_DIS

 drivers/crypto/ccp/sev-dev.c                  | 14 +++++
 tools/testing/selftests/kvm/include/x86/sev.h |  7 ++-
 .../selftests/kvm/x86/sev_migrate_tests.c     |  2 +-
 .../selftests/kvm/x86/sev_smoke_test.c        | 61 ++++++++++++++++++-
 4 files changed, 79 insertions(+), 5 deletions(-)

base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f

Signature · 1 line
2.53.0
#1 lore raw

PATCH 1/4 selftests/kvm: allow retrieving underlying SEV firmware error

From: "Tycho Andersen (AMD)" <[email protected]>

In addition to the errno, sometimes it is useful to know the underlying SEV firmware error. Update the raw vm ioctl macro to allow for optionally retrieving this.

---
 tools/testing/selftests/kvm/include/x86/sev.h       | 6 ++++--
 tools/testing/selftests/kvm/x86/sev_migrate_tests.c | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)
tools/testing/selftests/kvm/include/x86/sev.h +4 -2
index 008b4169f5e2..fd11f4222ec2 100644
@@ -76,7 +76,7 @@ static inline u64 snp_default_policy(void)
  * creating an overlay to pass in an "unsigned long" without a cast (casting
  * will make the compiler unhappy due to dereferencing an aliased pointer).
  */
-#define __vm_sev_ioctl(vm, cmd, arg)					\
+#define __vm_sev_ioctl(vm, cmd, arg, errorp)				\
 ({									\
 	int r;								\
 									\
@@ -90,12 +90,14 @@ static inline u64 snp_default_policy(void)
 	} };								\
 									\
 	r = __vm_ioctl(vm, KVM_MEMORY_ENCRYPT_OP, &sev_cmd.raw);	\
+	if (errorp != NULL)						\
+		*((__u32 *)errorp) = sev_cmd.c.error;			\
 	r ?: sev_cmd.c.error;						\
 })
 
 #define vm_sev_ioctl(vm, cmd, arg)					\
 ({									\
-	int ret = __vm_sev_ioctl(vm, cmd, arg);				\
+	int ret = __vm_sev_ioctl(vm, cmd, arg, NULL);			\
 									\
 	__TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd,	ret, vm);		\
 })
tools/testing/selftests/kvm/x86/sev_migrate_tests.c +1 -1
index 0a6dfba3905b..18f3091e0bd8 100644
@@ -232,7 +232,7 @@ static void verify_mirror_allowed_cmds(struct kvm_vm *vm)
 		 * These commands should be disallowed before the data
 		 * parameter is examined so NULL is OK here.
 		 */
-		ret = __vm_sev_ioctl(vm, cmd_id, NULL);
+		ret = __vm_sev_ioctl(vm, cmd_id, NULL, NULL);
 		TEST_ASSERT(
 			ret == -1 && errno == EINVAL,
 			"Should not be able call command: %d. ret: %d, errno: %d",
Signature · 1 line
2.53.0
#2 lore raw

PATCH 2/4 selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

In reply to Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

As in the comment, check to make sure that SEV-ES VMs are allowed before running the test. Otherwise, there is a generic:

    ==== Test Assertion Failure ====
      lib/x86/sev.c:91: !ret
      pid=3678 tid=3678 errno=5 - Input/output error
         1	0x0000000000417000: sev_vm_launch at sev.c:91 (discriminator 4)
         2	0x0000000000417d49: vm_sev_launch at sev.c:191
         3	0x0000000000402a7d: test_sev at sev_smoke_test.c:132
         4	0x000000000040328f: test_sev_smoke at sev_smoke_test.c:198
         5	0x00000000004026c8: main at sev_smoke_test.c:223
         6	0x00007fdde5c2a1c9: ?? ??:0
         7	0x00007fdde5c2a28a: ?? ??:0
         8	0x00000000004027f4: _start at ??:?
      KVM_SEV_LAUNCH_START failed, rc: -1 errno: 5 (Input/output error)
---
 .../selftests/kvm/x86/sev_smoke_test.c        | 37 ++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)
tools/testing/selftests/kvm/x86/sev_smoke_test.c +36 -1
index 86ad1c7d068f..c7fda9fc324b 100644
@@ -213,13 +213,48 @@ static void test_sev_smoke(void *guest, uint32_t type, uint64_t policy)
 	}
 }
 
+static bool sev_es_allowed(void)
+{
+	struct kvm_sev_launch_start launch_start = {
+		.policy = SEV_POLICY_ES,
+	};
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int firmware_error, ret;
+	bool supported = true;
+
+	if (!kvm_cpu_has(X86_FEATURE_SEV_ES))
+		return false;
+
+	if (!kvm_cpu_has(X86_FEATURE_SEV_SNP))
+		return true;
+
+	/*
+	 * In some cases when SEV-SNP is enabled, firmware disallows starting
+	 * an SEV-ES VM. When SEV-SNP is enabled try to launch an SEV-ES, and
+	 * check the underlying firmware error for this case.
+	 */
+	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_sev_es_code,
+					 &vcpu);
+
+	ret = __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_START, &launch_start,
+			     &firmware_error);
+	if (ret == -1 && firmware_error == SEV_RET_UNSUPPORTED) {
+		pr_info("SEV-ES not supported with SNP\n");
+		supported = false;
+	}
+
+	kvm_vm_free(vm);
+	return supported;
+}
+
 int main(int argc, char *argv[])
 {
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
 
 	test_sev_smoke(guest_sev_code, KVM_X86_SEV_VM, 0);
 
-	if (kvm_cpu_has(X86_FEATURE_SEV_ES))
+	if (sev_es_allowed())
 		test_sev_smoke(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
 
 	if (kvm_cpu_has(X86_FEATURE_SEV_SNP))
Signature · 1 line
2.53.0
#3 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Quoted 21 lines · Tycho Andersen
tools/testing/selftests/kvm/x86/sev_smoke_test.c +13 -0
index 86ad1c7d068f..c7fda9fc324b 100644
@@ -213,13 +213,48 @@ static void test_sev_smoke(void *guest, uint32_t type, uint64_t policy)
 	}
 }
 
+static bool sev_es_allowed(void)
+{
+	struct kvm_sev_launch_start launch_start = {
+		.policy = SEV_POLICY_ES,
+	};
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int firmware_error, ret;
+	bool supported = true;
+
+	if (!kvm_cpu_has(X86_FEATURE_SEV_ES))
+		return false;
+

tools/testing/selftests/kvm/x86/sev_smoke_test.c static void test_sev_smoke(void *guest, uint32_t type, uint64_t policy) · line 238

Quoted 10 lines
tools/testing/selftests/kvm/x86/sev_smoke_test.c +10 -0
+	if (!kvm_cpu_has(X86_FEATURE_SEV_SNP))
+		return true;
+
+	/*
+	 * In some cases when SEV-SNP is enabled, firmware disallows starting
+	 * an SEV-ES VM. When SEV-SNP is enabled try to launch an SEV-ES, and
+	 * check the underlying firmware error for this case.
+	 */
+	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_sev_es_code,
+					 &vcpu);

If there's a legimate reason why an SEV-ES VM can't be created, then that needs to be explicitly enumerated in some way by the kernel. E.g. is this due to lack of ASIDs due to CipherTextHiding or something? Throwing a noodle to see if it sticks is not an option.

Quoted 26 lines
tools/testing/selftests/kvm/x86/sev_smoke_test.c +13 -1
+
+	ret = __vm_sev_ioctl(vm, KVM_SEV_LAUNCH_START, &launch_start,
+			     &firmware_error);
+	if (ret == -1 && firmware_error == SEV_RET_UNSUPPORTED) {
+		pr_info("SEV-ES not supported with SNP\n");
+		supported = false;
+	}
+
+	kvm_vm_free(vm);
+	return supported;
+}
+
 int main(int argc, char *argv[])
 {
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
 
 	test_sev_smoke(guest_sev_code, KVM_X86_SEV_VM, 0);
 
-	if (kvm_cpu_has(X86_FEATURE_SEV_ES))
+	if (sev_es_allowed())
 		test_sev_smoke(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
 
 	if (kvm_cpu_has(X86_FEATURE_SEV_SNP))
Signature · 1 line
2.53.0
#4 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Hi Sean,

Quoted 28 lines · Sean Christopherson
Quoted 27 lines · Tycho Andersen
tools/testing/selftests/kvm/x86/sev_smoke_test.c +19 -0
index 86ad1c7d068f..c7fda9fc324b 100644
@@ -213,13 +213,48 @@ static void test_sev_smoke(void *guest, uint32_t type, uint64_t policy)
 	}
 }
 
+static bool sev_es_allowed(void)
+{
+	struct kvm_sev_launch_start launch_start = {
+		.policy = SEV_POLICY_ES,
+	};
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int firmware_error, ret;
+	bool supported = true;
+
+	if (!kvm_cpu_has(X86_FEATURE_SEV_ES))
+		return false;
+
+	if (!kvm_cpu_has(X86_FEATURE_SEV_SNP))
+		return true;
+
+	/*
+	 * In some cases when SEV-SNP is enabled, firmware disallows starting
+	 * an SEV-ES VM. When SEV-SNP is enabled try to launch an SEV-ES, and
Quoted 8 lines

tools/testing/selftests/kvm/x86/sev_smoke_test.c static void test_sev_smoke(void *guest, uint32_t type, uint64_t policy) · line 238

Quoted 4 lines
tools/testing/selftests/kvm/x86/sev_smoke_test.c +4 -0
+	 * check the underlying firmware error for this case.
+	 */
+	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_sev_es_code,
+					 &vcpu);

If there's a legimate reason why an SEV-ES VM can't be created, then that needs to be explicitly enumerated in some way by the kernel. E.g. is this due to lack of ASIDs due to CipherTextHiding or something?

Newer firmware that fixes CVE-2025-48514 won't allow SEV-ES VMs to be started with SNP enabled, there is a footnote (2) about it here:

https://www.amd.com/en/resources/product-security/bulletin/amd-sb-3023.html

Probably should have included this in the patch, sorry.

Quoted 1 line

Throwing a noodle to see if it sticks is not an option.

Sure, we could do some firmware version test to see if it's fixed instead? Or do this same test in the kernel and export that as an ioctl?

Thanks,

Tycho

#5 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Quoted 17 lines · Tycho Andersen
Quoted 11 lines

tools/testing/selftests/kvm/x86/sev_smoke_test.c static void test_sev_smoke(void *guest, uint32_t type, uint64_t policy) · line 238

Quoted 7 lines
tools/testing/selftests/kvm/x86/sev_smoke_test.c +7 -0
+	/*
+	 * In some cases when SEV-SNP is enabled, firmware disallows starting
+	 * an SEV-ES VM. When SEV-SNP is enabled try to launch an SEV-ES, and
+	 * check the underlying firmware error for this case.
+	 */
+	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_sev_es_code,
+					 &vcpu);

If there's a legimate reason why an SEV-ES VM can't be created, then that needs to be explicitly enumerated in some way by the kernel. E.g. is this due to lack of ASIDs due to CipherTextHiding or something?

Newer firmware that fixes CVE-2025-48514 won't allow SEV-ES VMs to be started with SNP enabled, there is a footnote (2) about it here:

https://www.amd.com/en/resources/product-security/bulletin/amd-sb-3023.html

Quoted 7 lines

Probably should have included this in the patch, sorry.

Quoted 1 line

Throwing a noodle to see if it sticks is not an option.

Sure, we could do some firmware version test to see if it's fixed instead? Or do this same test in the kernel and export that as an ioctl?

Uh, no idea what would be ideal, but there absolutely needs to be some way to communicate lack of effective SEV-ES support to userspace, and in a way that doesn't break userspace.

Hrm, I think we also neglected to communicate when SEV and SEV-ES are effectively unusable, e.g. due to CipherTextHiding, so maybe we can kill two birds with one stone? IIRC, we didn't bother enumerating the limitation with CipherTextHiding because making SEV-ES unusable would require a deliberate act from the admin.

"Update firmware" is also an deliberate act, but the side effect of SEV-ES being disabled, not so much.

#6 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Quoted 22 lines · Sean Christopherson
Quoted 21 lines · Tycho Andersen
Quoted 11 lines

tools/testing/selftests/kvm/x86/sev_smoke_test.c static void test_sev_smoke(void *guest, uint32_t type, uint64_t policy) · line 238

Quoted 7 lines
tools/testing/selftests/kvm/x86/sev_smoke_test.c +7 -0
+	/*
+	 * In some cases when SEV-SNP is enabled, firmware disallows starting
+	 * an SEV-ES VM. When SEV-SNP is enabled try to launch an SEV-ES, and
+	 * check the underlying firmware error for this case.
+	 */
+	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_sev_es_code,
+					 &vcpu);

If there's a legimate reason why an SEV-ES VM can't be created, then that needs to be explicitly enumerated in some way by the kernel. E.g. is this due to lack of ASIDs due to CipherTextHiding or something?

Newer firmware that fixes CVE-2025-48514 won't allow SEV-ES VMs to be started with SNP enabled, there is a footnote (2) about it here:

https://www.amd.com/en/resources/product-security/bulletin/amd-sb-3023.html

Probably should have included this in the patch, sorry.

Quoted 1 line

Throwing a noodle to see if it sticks is not an option.

Quoted 7 lines
Quoted 3 lines

Sure, we could do some firmware version test to see if it's fixed instead? Or do this same test in the kernel and export that as an ioctl?

Uh, no idea what would be ideal, but there absolutely needs to be some way to communicate lack of effective SEV-ES support to userspace, and in a way that doesn't break userspace.

Just to clarify, by "doesn't break userspace" here you mean that we shouldn't revoke the SEV_ES bit from the list of supported VM types once we've exposed it? Or you mean preserving the current behavior of CPU supports it => bit is set?

Quoted 4 lines

Hrm, I think we also neglected to communicate when SEV and SEV-ES are effectively unusable, e.g. due to CipherTextHiding, so maybe we can kill two birds with one stone? IIRC, we didn't bother enumerating the limitation with CipherTextHiding because making SEV-ES unusable would require a deliberate act from the admin.

We know these parameters at module load time so we could unset the supported bit, but...

Quoted 2 lines

"Update firmware" is also an deliberate act, but the side effect of SEV-ES being disabled, not so much.

since this could be a runtime thing via DOWNLOAD_FIRMWARE_EX at some point, I guess we need a new RUNTIME_STATUS ioctl or similar. Then the question is: does it live in /dev/sev, or /dev/kvm?

Tycho

#7 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Quoted 27 lines · Tycho Andersen
Quoted 26 lines · Sean Christopherson
Quoted 24 lines · Tycho Andersen
Quoted 11 lines

tools/testing/selftests/kvm/x86/sev_smoke_test.c static void test_sev_smoke(void *guest, uint32_t type, uint64_t policy) · line 238

Quoted 7 lines
tools/testing/selftests/kvm/x86/sev_smoke_test.c +7 -0
+	/*
+	 * In some cases when SEV-SNP is enabled, firmware disallows starting
+	 * an SEV-ES VM. When SEV-SNP is enabled try to launch an SEV-ES, and
+	 * check the underlying firmware error for this case.
+	 */
+	vm = vm_sev_create_with_one_vcpu(KVM_X86_SEV_ES_VM, guest_sev_es_code,
+					 &vcpu);

If there's a legimate reason why an SEV-ES VM can't be created, then that needs to be explicitly enumerated in some way by the kernel. E.g. is this due to lack of ASIDs due to CipherTextHiding or something?

Newer firmware that fixes CVE-2025-48514 won't allow SEV-ES VMs to be started with SNP enabled, there is a footnote (2) about it here:

https://www.amd.com/en/resources/product-security/bulletin/amd-sb-3023.html

Probably should have included this in the patch, sorry.

Quoted 1 line

Throwing a noodle to see if it sticks is not an option.

Sure, we could do some firmware version test to see if it's fixed instead? Or do this same test in the kernel and export that as an ioctl?

Quoted 8 lines
Quoted 3 lines

Uh, no idea what would be ideal, but there absolutely needs to be some way to communicate lack of effective SEV-ES support to userspace, and in a way that doesn't break userspace.

Just to clarify, by "doesn't break userspace" here you mean that we shouldn't revoke the SEV_ES bit from the list of supported VM types once we've exposed it? Or you mean preserving the current behavior of CPU supports it => bit is set?

I didn't have concrete concerns, I just want to make sure we don't do something that would confuse userspace and e.g. prevent using KVM for SNP or something.

Hmm, I like the idea of clearing supported_vm_types. The wrinkle is that "legacy" deployments that use KVM_SEV_INIT instead of KVM_SEV_INIT2 will use KVM_X86_DEFAULT_VM, and probably won't check for SEV and SEV_ES VM types.

Alternatively, or in addition to, we could clear X86_FEATURE_SEV_ES. But clearing SEV_ES while leaving X86_FEATURE_SEV_SNP makes me nervous. KVM doesn't *currently* check for any of those in kvm_cpu_caps, but that could change in the future. And it's somewhat misleading, e.g. because sev_snp_guest() expects sev_es_guest() to be true.

Given that it doesn't make sense for KVM to actively prevent the admin from upgrading the firmware, I think it's ok if KVM can't "gracefully" handle *every* case. E.g. even if KVM clears X86_FEATURE_SEV_ES, userspace could have cached that information at system boot.

Quoted 8 lines
Quoted 4 lines

Hrm, I think we also neglected to communicate when SEV and SEV-ES are effectively unusable, e.g. due to CipherTextHiding, so maybe we can kill two birds with one stone? IIRC, we didn't bother enumerating the limitation with CipherTextHiding because making SEV-ES unusable would require a deliberate act from the admin.

We know these parameters at module load time so we could unset the supported bit, but...

Quoted 6 lines
Quoted 2 lines

"Update firmware" is also an deliberate act, but the side effect of SEV-ES being disabled, not so much.

since this could be a runtime thing via DOWNLOAD_FIRMWARE_EX at some point, I guess we need a new RUNTIME_STATUS ioctl or similar. Then the question is: does it live in /dev/sev, or /dev/kvm?

Ugh. Yeah, updating supported_vm_types definitely seems like the least-awful option.

#8 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Quoted 3 lines · Sean Christopherson

Hmm, I like the idea of clearing supported_vm_types. The wrinkle is that "legacy" deployments that use KVM_SEV_INIT instead of KVM_SEV_INIT2 will use KVM_X86_DEFAULT_VM, and probably won't check for SEV and SEV_ES VM types.

Does that matter? If in the case of CiphertextHiding we would revoke KVM_X86_SEV_VM, users already couldn't start a VM anyway in the configuration.

The firmware update is more tricky, but I don't think you can blame the kernel there...

Quoted 22 lines

Alternatively, or in addition to, we could clear X86_FEATURE_SEV_ES. But clearing SEV_ES while leaving X86_FEATURE_SEV_SNP makes me nervous. KVM doesn't *currently* check for any of those in kvm_cpu_caps, but that could change in the future. And it's somewhat misleading, e.g. because sev_snp_guest() expects sev_es_guest() to be true.

Given that it doesn't make sense for KVM to actively prevent the admin from upgrading the firmware, I think it's ok if KVM can't "gracefully" handle *every* case. E.g. even if KVM clears X86_FEATURE_SEV_ES, userspace could have cached that information at system boot.

Quoted 11 lines
Quoted 4 lines

Hrm, I think we also neglected to communicate when SEV and SEV-ES are effectively unusable, e.g. due to CipherTextHiding, so maybe we can kill two birds with one stone? IIRC, we didn't bother enumerating the limitation with CipherTextHiding because making SEV-ES unusable would require a deliberate act from the admin.

We know these parameters at module load time so we could unset the supported bit, but...

Quoted 2 lines

"Update firmware" is also an deliberate act, but the side effect of SEV-ES being disabled, not so much.

Quoted 6 lines
Quoted 3 lines

since this could be a runtime thing via DOWNLOAD_FIRMWARE_EX at some point, I guess we need a new RUNTIME_STATUS ioctl or similar. Then the question is: does it live in /dev/sev, or /dev/kvm?

Ugh. Yeah, updating supported_vm_types definitely seems like the least-awful option.

Since firmware update only happens on init right now, I think we can add a:

    int sev_firmware_supported_vm_types();

that will do the feature detection from the ccp, and merge that with the results based on asid assignments during module init.

We'll eventually need some callback into KVM to say say "hey the firmware got updated here's a new list of vm types".

Tycho

#9 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Quoted 6 lines · Tycho Andersen
Quoted 3 lines · Sean Christopherson

Hmm, I like the idea of clearing supported_vm_types. The wrinkle is that "legacy" deployments that use KVM_SEV_INIT instead of KVM_SEV_INIT2 will use KVM_X86_DEFAULT_VM, and probably won't check for SEV and SEV_ES VM types.

Does that matter?

Yes, but I don't think it matters so much that it's worth dealing with. For me, being slightly nicer to userspace doesn't justify the risk of confusing KVM.

Quoted 5 lines

If in the case of CiphertextHiding we would revoke KVM_X86_SEV_VM, users already couldn't start a VM anyway in the configuration.

The firmware update is more tricky, but I don't think you can blame the kernel there...

Yeah, that's about where I'm at.

Quoted 29 lines
Quoted 22 lines

Alternatively, or in addition to, we could clear X86_FEATURE_SEV_ES. But clearing SEV_ES while leaving X86_FEATURE_SEV_SNP makes me nervous. KVM doesn't *currently* check for any of those in kvm_cpu_caps, but that could change in the future. And it's somewhat misleading, e.g. because sev_snp_guest() expects sev_es_guest() to be true.

Given that it doesn't make sense for KVM to actively prevent the admin from upgrading the firmware, I think it's ok if KVM can't "gracefully" handle *every* case. E.g. even if KVM clears X86_FEATURE_SEV_ES, userspace could have cached that information at system boot.

Quoted 11 lines
Quoted 4 lines

Hrm, I think we also neglected to communicate when SEV and SEV-ES are effectively unusable, e.g. due to CipherTextHiding, so maybe we can kill two birds with one stone? IIRC, we didn't bother enumerating the limitation with CipherTextHiding because making SEV-ES unusable would require a deliberate act from the admin.

We know these parameters at module load time so we could unset the supported bit, but...

Quoted 2 lines

"Update firmware" is also an deliberate act, but the side effect of SEV-ES being disabled, not so much.

Quoted 6 lines
Quoted 3 lines

since this could be a runtime thing via DOWNLOAD_FIRMWARE_EX at some point, I guess we need a new RUNTIME_STATUS ioctl or similar. Then the question is: does it live in /dev/sev, or /dev/kvm?

Ugh. Yeah, updating supported_vm_types definitely seems like the least-awful option.

Quoted 7 lines

Since firmware update only happens on init right now, I think we can add a:

    int sev_firmware_supported_vm_types();

that will do the feature detection from the ccp, and merge that with the results based on asid assignments during module init.

Ya, I don't have a better idea. Bleeding VM types into the CCP driver might be a bit wonky, though I guess it is uAPI so it's certainly not a KVM-internal detail.

Quoted 2 lines

We'll eventually need some callback into KVM to say say "hey the firmware got updated here's a new list of vm types".

#10 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Quoted 2 lines · Sean Christopherson

Ya, I don't have a better idea. Bleeding VM types into the CCP driver might be a bit wonky, though I guess it is uAPI so it's certainly not a KVM-internal detail.

Turns out this approach breaks the selftests, which is at least one userspace:

# ./sev_init2_tests
Random seed: 0x6b8b4567
==== Test Assertion Failure ====
  x86/sev_init2_tests.c:141: have_sev_es == !!(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_ES_VM))
  pid=12498 tid=12498 errno=0 - Success
     1	0x0000000000402747: main at sev_init2_tests.c:141 (discriminator 2)
     2	0x00007f9adae2a1c9: ?? ??:0
     3	0x00007f9adae2a28a: ?? ??:0
     4	0x0000000000402934: _start at ??:?
  sev-es: KVM_CAP_VM_TYPES (15) does not match cpuid (checking 8)

As near as I can tell qemu doesn't do the same anywhere. SNP guests run fine, and SEV-ES says something reasonable:

qemu-system-x86_64: sev_launch_start: LAUNCH_START ret=1 fw_error=21 'Feature not supported'
qemu-system-x86_64: sev_common_kvm_init: failed to create encryption context
qemu-system-x86_64: failed to initialize kvm: Operation not permitted

Thoughts?

Tycho

#11 lore raw

Re: [PATCH 2/4] selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode

Quoted 18 lines · Tycho Andersen
Quoted 2 lines · Sean Christopherson

Ya, I don't have a better idea. Bleeding VM types into the CCP driver might be a bit wonky, though I guess it is uAPI so it's certainly not a KVM-internal detail.

Turns out this approach breaks the selftests, which is at least one userspace:

# ./sev_init2_tests
Random seed: 0x6b8b4567
==== Test Assertion Failure ====
  x86/sev_init2_tests.c:141: have_sev_es == !!(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_ES_VM))
  pid=12498 tid=12498 errno=0 - Success
     1	0x0000000000402747: main at sev_init2_tests.c:141 (discriminator 2)
     2	0x00007f9adae2a1c9: ?? ??:0
     3	0x00007f9adae2a28a: ?? ??:0
     4	0x0000000000402934: _start at ??:?
  sev-es: KVM_CAP_VM_TYPES (15) does not match cpuid (checking 8)
Quoted 8 lines

As near as I can tell qemu doesn't do the same anywhere. SNP guests run fine, and SEV-ES says something reasonable:

qemu-system-x86_64: sev_launch_start: LAUNCH_START ret=1 fw_error=21 'Feature not supported'
qemu-system-x86_64: sev_common_kvm_init: failed to create encryption context
qemu-system-x86_64: failed to initialize kvm: Operation not permitted

Thoughts?

Breaking selftests is totally fine, they don't count as real users (the whole point is to validate KVM behavior; if we weren't allowed to break selftests, we literally couldn't fix a huge pile of KVM bugs).

Even if a real VMM has a similar sanity check, I wouldn't consider an assertion firing to be a breaking flaw. No matter what, the VMM won't be able to launch an SEV-ES guest.

For selftests, something like this?

	have_sev_es = kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_ES_VM);
	TEST_ASSERT(!have_sev_es || kvm_cpu_has(X86_FEATURE_SEV_ES),
		    "sev-es: SEV_ES_VM supported without SEV_ES in CPUID");

	have_snp = kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SNP_VM);
	TEST_ASSERT(!have_snp || kvm_cpu_has(X86_FEATURE_SEV_SNP),
		    "sev-snp: SNP_VM supported with SEV_SNP in CPUID");
#12 lore raw

PATCH 3/4 crypto/ccp: support setting RAPL_DIS in SNP_INIT_EX

In reply to Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

The kernel allows setting the RAPL_DIS policy bit, but had no way to set the RAPL_DIS bit during SNP_INIT_EX. Setting the policy bit would always result in:

    [  898.840286] ccp 0000:a9:00.5: sev command 0xa0 failed (0x00000007)

Allow setting the RAPL_DIS bit during SNP_INIT_EX via a module parameter. If the hardware does not support RAPL_DIS, log and disable the module parameter.

---
 drivers/crypto/ccp/sev-dev.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
drivers/crypto/ccp/sev-dev.c +14 -0
index 096f993974d1..362126453ef0 100644
@@ -75,6 +75,10 @@ static bool psp_init_on_probe = true;
 module_param(psp_init_on_probe, bool, 0444);
 MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
 
+static bool rapl_disable;
+module_param(rapl_disable, bool, 0444);
+MODULE_PARM_DESC(rapl_disable, "  if true, the RAPL_DIS bit will be set during INIT_EX if supported");
+
 #if IS_ENABLED(CONFIG_PCI_TSM)
 static bool sev_tio_enabled = true;
 module_param_named(tio, sev_tio_enabled, bool, 0444);
@@ -1428,6 +1432,16 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 			data.max_snp_asid = max_snp_asid;
 		}
 
+		if (rapl_disable) {
+			if (sev->snp_feat_info_0.ecx & SNP_RAPL_DISABLE_SUPPORTED) {
+				data.rapl_dis = 1;
+			} else {
+				dev_info(sev->dev,
+					"SEV: RAPL_DIS requested, but not supported");
+				rapl_disable = false;
+			}
+		}
+
 		data.init_rmp = 1;
 		data.list_paddr_en = 1;
 		data.list_paddr = __psp_pa(snp_range_list);
Signature · 1 line
2.53.0
#13 lore raw

Re: [PATCH 3/4] crypto/ccp: support setting RAPL_DIS in SNP_INIT_EX

Quoted 3 lines · Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

The kernel allows setting the RAPL_DIS policy bit, but had no way to set

Please actually say what RAPL_DIS is and does, and explain why this is the correct approach. I genuinely have no idea what the impact of this patch is, (beyond disabling something, obviously).

Quoted 49 lines

the RAPL_DIS bit during SNP_INIT_EX. Setting the policy bit would always result in:

    [  898.840286] ccp 0000:a9:00.5: sev command 0xa0 failed (0x00000007)

Allow setting the RAPL_DIS bit during SNP_INIT_EX via a module parameter. If the hardware does not support RAPL_DIS, log and disable the module parameter.

---
 drivers/crypto/ccp/sev-dev.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
drivers/crypto/ccp/sev-dev.c +14 -0
index 096f993974d1..362126453ef0 100644
@@ -75,6 +75,10 @@ static bool psp_init_on_probe = true;
 module_param(psp_init_on_probe, bool, 0444);
 MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
 
+static bool rapl_disable;
+module_param(rapl_disable, bool, 0444);
+MODULE_PARM_DESC(rapl_disable, "  if true, the RAPL_DIS bit will be set during INIT_EX if supported");
+
 #if IS_ENABLED(CONFIG_PCI_TSM)
 static bool sev_tio_enabled = true;
 module_param_named(tio, sev_tio_enabled, bool, 0444);
@@ -1428,6 +1432,16 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 			data.max_snp_asid = max_snp_asid;
 		}
 
+		if (rapl_disable) {
+			if (sev->snp_feat_info_0.ecx & SNP_RAPL_DISABLE_SUPPORTED) {
+				data.rapl_dis = 1;
+			} else {
+				dev_info(sev->dev,
+					"SEV: RAPL_DIS requested, but not supported");
+				rapl_disable = false;
+			}
+		}
+
 		data.init_rmp = 1;
 		data.list_paddr_en = 1;
 		data.list_paddr = __psp_pa(snp_range_list);
Signature · 1 line
2.53.0
#14 lore raw

Re: [PATCH 3/4] crypto/ccp: support setting RAPL_DIS in SNP_INIT_EX

Quoted 8 lines · Sean Christopherson
Quoted 3 lines · Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

The kernel allows setting the RAPL_DIS policy bit, but had no way to set

Please actually say what RAPL_DIS is and does, and explain why this is the correct approach. I genuinely have no idea what the impact of this patch is, (beyond disabling something, obviously).

Sure, the easiest thing is probably to quote the firmware PDF:

Some processors support the Running Average Power Limit (RAPL) feature which provides information about power utilization of software. RAPL can be disabled using the RAPL_DIS flag in SNP_INIT_EX to disable RAPL while SNP firmware is in the INIT state. Guests may require that RAPL is disabled by using the POLICY.RAPL_DIS guest policy flag.

I'll add this to the message in the next version.

I will also split out the RAPL_DIS stuff from the test fix for the firmware update.

Tycho

#15 lore raw

Re: [PATCH 3/4] crypto/ccp: support setting RAPL_DIS in SNP_INIT_EX

Quoted 10 lines · Tycho Andersen
Quoted 8 lines · Sean Christopherson
Quoted 3 lines · Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

The kernel allows setting the RAPL_DIS policy bit, but had no way to set

Please actually say what RAPL_DIS is and does, and explain why this is the correct approach. I genuinely have no idea what the impact of this patch is, (beyond disabling something, obviously).

Quoted 8 lines

Sure, the easiest thing is probably to quote the firmware PDF:

Some processors support the Running Average Power Limit (RAPL) feature which provides information about power utilization of software. RAPL can be disabled using the RAPL_DIS flag in SNP_INIT_EX to disable RAPL while SNP firmware is in the INIT state. Guests may require that RAPL is disabled by using the POLICY.RAPL_DIS guest policy flag.

Ah, I assume this about disabling RAPL to mitigate a potential side channel? If so, please call that out in the changelog.

And does this disable RAPL for _everything_? Or does it just disable RAPL for SNP VMs? If it's the former, then burying this in drivers/crypto/ccp/sev-dev.c feels wrong.

#16 lore raw

Re: [PATCH 3/4] crypto/ccp: support setting RAPL_DIS in SNP_INIT_EX

Quoted 20 lines · Sean Christopherson
Quoted 10 lines · Tycho Andersen
Quoted 8 lines · Sean Christopherson
Quoted 3 lines · Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

The kernel allows setting the RAPL_DIS policy bit, but had no way to set

Please actually say what RAPL_DIS is and does, and explain why this is the correct approach. I genuinely have no idea what the impact of this patch is, (beyond disabling something, obviously).

Quoted 8 lines

Sure, the easiest thing is probably to quote the firmware PDF:

Some processors support the Running Average Power Limit (RAPL) feature which provides information about power utilization of software. RAPL can be disabled using the RAPL_DIS flag in SNP_INIT_EX to disable RAPL while SNP firmware is in the INIT state. Guests may require that RAPL is disabled by using the POLICY.RAPL_DIS guest policy flag.

Quoted 6 lines

Ah, I assume this about disabling RAPL to mitigate a potential side channel? If so, please call that out in the changelog.

And does this disable RAPL for _everything_? Or does it just disable RAPL for SNP VMs? If it's the former, then burying this in drivers/crypto/ccp/sev-dev.c feels wrong.

Presumably you're right on both counts, but I've asked our firmware team to clarify exactly what happens.

I guess that means it should be kvm-amd.rapl_disable?

Thanks,

Tycho

#17 lore raw

PATCH 4/4 selftests/kvm: smoke test support for RAPL_DIS

In reply to Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

If the hardware supports the RAPL_DIS policy bit and the ccp has been loaded with the RAPL_DIS bit set, make sure a VM can actually start using it.

---
 tools/testing/selftests/kvm/include/x86/sev.h |  1 +
 .../selftests/kvm/x86/sev_smoke_test.c        | 24 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)
tools/testing/selftests/kvm/include/x86/sev.h +1 -0
index fd11f4222ec2..e9a566ff6df1 100644
@@ -28,6 +28,7 @@ enum sev_guest_state {
 #define SNP_POLICY_SMT		(1ULL << 16)
 #define SNP_POLICY_RSVD_MBO	(1ULL << 17)
 #define SNP_POLICY_DBG		(1ULL << 19)
+#define SNP_POLICY_RAPL_DIS	(1ULL << 23)
 
 #define GHCB_MSR_TERM_REQ	0x100
 
tools/testing/selftests/kvm/x86/sev_smoke_test.c +23 -1
index c7fda9fc324b..e4cf5b99b19a 100644
@@ -248,6 +248,18 @@ static bool sev_es_allowed(void)
 	return supported;
 }
 
+static u64 supported_policy_mask(void)
+{
+	int kvm_fd = open_kvm_dev_path_or_exit();
+	u64 policy_mask = 0;
+
+	kvm_device_attr_get(kvm_fd, KVM_X86_GRP_SEV,
+			    KVM_X86_SNP_POLICY_BITS,
+			    &policy_mask);
+	close(kvm_fd);
+	return policy_mask;
+}
+
 int main(int argc, char *argv[])
 {
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
@@ -257,8 +269,18 @@ int main(int argc, char *argv[])
 	if (sev_es_allowed())
 		test_sev_smoke(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
 
-	if (kvm_cpu_has(X86_FEATURE_SEV_SNP))
+	if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
+		int supported_policy = supported_policy_mask();
+
 		test_sev_smoke(guest_snp_code, KVM_X86_SNP_VM, snp_default_policy());
 
+		if (supported_policy & SNP_POLICY_RAPL_DIS &&
+		    kvm_get_module_param_bool("ccp", "rapl_disable")) {
+			uint32_t policy = snp_default_policy() | SNP_POLICY_RAPL_DIS;
+
+			test_sev_smoke(guest_snp_code, KVM_X86_SNP_VM, policy);
+		}
+	}
+
 	return 0;
 }
Signature · 1 line
2.53.0