Allow setting RAPL_DIS during SNP_INIT_EX
Subsystems (6)
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
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
@@ -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
@@ -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
PATCH 2/4 selftests/kvm: check that SEV-ES VMs are allowed in SEV-SNP mode
In reply to Tycho AndersenFrom: "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
@@ -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
Sean Christopherson
maintainerRe: [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@@ -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
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@@ -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
Sean Christopherson
maintainerRe: [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.
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
Sean Christopherson
maintainerRe: [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_ESbit 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_EXat some point, I guess we need a newRUNTIME_STATUSioctl 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.
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 useKVM_SEV_INITinstead ofKVM_SEV_INIT2will useKVM_X86_DEFAULT_VM, and probably won't check for SEV andSEV_ESVM 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 clearingSEV_ESwhile leavingX86_FEATURE_SEV_SNPmakes me nervous. KVM doesn't *currently* check for any of those inkvm_cpu_caps, but that could change in the future. And it's somewhat misleading, e.g. becausesev_snp_guest()expectssev_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_EXat some point, I guess we need a newRUNTIME_STATUSioctl or similar. Then the question is: does it live in/dev/sev, or/dev/kvm?Ugh. Yeah, updating
supported_vm_typesdefinitely 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
Sean Christopherson
maintainerRe: [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 useKVM_SEV_INITinstead ofKVM_SEV_INIT2will useKVM_X86_DEFAULT_VM, and probably won't check for SEV andSEV_ESVM 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 clearingSEV_ESwhile leavingX86_FEATURE_SEV_SNPmakes me nervous. KVM doesn't *currently* check for any of those inkvm_cpu_caps, but that could change in the future. And it's somewhat misleading, e.g. becausesev_snp_guest()expectssev_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_EXat some point, I guess we need a newRUNTIME_STATUSioctl or similar. Then the question is: does it live in/dev/sev, or/dev/kvm?Ugh. Yeah, updating
supported_vm_typesdefinitely 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".
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
Sean Christopherson
maintainerRe: [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_STARTret=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 permittedThoughts?
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");
PATCH 3/4 crypto/ccp: support setting RAPL_DIS in SNP_INIT_EX
In reply to Tycho AndersenFrom: "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
@@ -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
Sean Christopherson
maintainerRe: [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_DISpolicy 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_DISbit duringSNP_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_DISbit duringSNP_INIT_EXvia a module parameter. If the hardware does not supportRAPL_DIS, log and disable the module parameter.Signed-off-by Tycho Andersen (AMD) <[email protected]>--- drivers/crypto/ccp/sev-dev.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)drivers/crypto/ccp/sev-dev.c +14
-0@@ -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
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_DISpolicy bit, but had no way to setPlease actually say what
RAPL_DISis 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
Sean Christopherson
maintainerRe: [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_DISpolicy bit, but had no way to setPlease actually say what
RAPL_DISis 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_DISflag inSNP_INIT_EXto 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.
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_DISpolicy bit, but had no way to setPlease actually say what
RAPL_DISis 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_DISflag inSNP_INIT_EXto 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.cfeels 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
PATCH 4/4 selftests/kvm: smoke test support for RAPL_DIS
In reply to Tycho AndersenFrom: "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
@@ -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
@@ -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