Currently, check_sev_features() is called in multiple places when
processing IGVM files: both when processing the initial VMSA SEV
features from IGVM, as well as when validating the full contents of the
VMSA. Move this to a single point in sev_common_kvm_init() to simplify
the flow, as well as to re-use this function when VMSA SEV features are
being set without using IGVM files.
Since check_sev_features() relies on SVM_SEV_FEAT_SNP_ACTIVE being set
in VMSA SEV features depending on the guest type, set this flag by
default when creating SEV-SNP guests. When using IGVM files, this field
is anyway over-written so that validation in check_sev_features() is
still relevant.
Finally, add a check to ensure SEV features aren't also set through qemu
cli if using IGVM files.
Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
target/i386/sev.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 1057b8ab2c60..243e9493ba8d 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -586,9 +586,6 @@ static int check_vmsa_supported(SevCommonState *sev_common, hwaddr gpa,
vmsa_check.x87_fcw = 0;
vmsa_check.mxcsr = 0;
- if (check_sev_features(sev_common, vmsa_check.sev_features, errp) < 0) {
- return -1;
- }
vmsa_check.sev_features = 0;
if (!buffer_is_zero(&vmsa_check, sizeof(vmsa_check))) {
@@ -1892,20 +1889,29 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
* as SEV_STATE_UNINIT.
*/
if (x86machine->igvm) {
+ if (sev_common->sev_features & ~SVM_SEV_FEAT_SNP_ACTIVE) {
+ error_setg(errp, "%s: SEV features can't be specified when using IGVM files",
+ __func__);
+ return -1;
+ }
if (IGVM_CFG_GET_CLASS(x86machine->igvm)
->process(x86machine->igvm, machine->cgs, true, errp) ==
-1) {
return -1;
}
- /*
- * KVM maintains a bitmask of allowed sev_features. This does not
- * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by KVM
- * itself. Therefore we need to clear this flag.
- */
- args.vmsa_features = sev_common->sev_features &
- ~SVM_SEV_FEAT_SNP_ACTIVE;
}
+ if (check_sev_features(sev_common, sev_common->sev_features, errp) < 0) {
+ return -1;
+ }
+
+ /*
+ * KVM maintains a bitmask of allowed sev_features. This does not
+ * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by KVM
+ * itself. Therefore we need to clear this flag.
+ */
+ args.vmsa_features = sev_common->sev_features & ~SVM_SEV_FEAT_SNP_ACTIVE;
+
ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error);
break;
}
@@ -2518,9 +2524,6 @@ static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
__func__);
return -1;
}
- if (check_sev_features(sev_common, sa->sev_features, errp) < 0) {
- return -1;
- }
sev_common->sev_features = sa->sev_features;
}
return 0;
@@ -3127,6 +3130,7 @@ sev_snp_guest_instance_init(Object *obj)
/* default init/start/finish params for kvm */
sev_snp_guest->kvm_start_conf.policy = DEFAULT_SEV_SNP_POLICY;
+ SEV_COMMON(sev_snp_guest)->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
}
/* guest info specific to sev-snp */
--
2.50.1On 9/11/25 06:54, Naveen N Rao (AMD) wrote:
> Currently, check_sev_features() is called in multiple places when
> processing IGVM files: both when processing the initial VMSA SEV
> features from IGVM, as well as when validating the full contents of the
> VMSA. Move this to a single point in sev_common_kvm_init() to simplify
> the flow, as well as to re-use this function when VMSA SEV features are
> being set without using IGVM files.
>
> Since check_sev_features() relies on SVM_SEV_FEAT_SNP_ACTIVE being set
> in VMSA SEV features depending on the guest type, set this flag by
> default when creating SEV-SNP guests. When using IGVM files, this field
> is anyway over-written so that validation in check_sev_features() is
> still relevant.
There seem to be multiple things going on in this patch and I wonder if it
would be best to split it up into separate smaller patches.
You have setting of SVM_SEV_FEAT_SNP_ACTIVE in sev_features, you have a
new check for sev_features being set when using an IGVM file and you have
the consolidation.
Thanks,
Tom
>
> Finally, add a check to ensure SEV features aren't also set through qemu
> cli if using IGVM files.
>
> Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
> ---
> target/i386/sev.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 1057b8ab2c60..243e9493ba8d 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -586,9 +586,6 @@ static int check_vmsa_supported(SevCommonState *sev_common, hwaddr gpa,
> vmsa_check.x87_fcw = 0;
> vmsa_check.mxcsr = 0;
>
> - if (check_sev_features(sev_common, vmsa_check.sev_features, errp) < 0) {
> - return -1;
> - }
> vmsa_check.sev_features = 0;
>
> if (!buffer_is_zero(&vmsa_check, sizeof(vmsa_check))) {
> @@ -1892,20 +1889,29 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> * as SEV_STATE_UNINIT.
> */
> if (x86machine->igvm) {
> + if (sev_common->sev_features & ~SVM_SEV_FEAT_SNP_ACTIVE) {
> + error_setg(errp, "%s: SEV features can't be specified when using IGVM files",
> + __func__);
> + return -1;
> + }
> if (IGVM_CFG_GET_CLASS(x86machine->igvm)
> ->process(x86machine->igvm, machine->cgs, true, errp) ==
> -1) {
> return -1;
> }
> - /*
> - * KVM maintains a bitmask of allowed sev_features. This does not
> - * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by KVM
> - * itself. Therefore we need to clear this flag.
> - */
> - args.vmsa_features = sev_common->sev_features &
> - ~SVM_SEV_FEAT_SNP_ACTIVE;
> }
>
> + if (check_sev_features(sev_common, sev_common->sev_features, errp) < 0) {
> + return -1;
> + }
> +
> + /*
> + * KVM maintains a bitmask of allowed sev_features. This does not
> + * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by KVM
> + * itself. Therefore we need to clear this flag.
> + */
> + args.vmsa_features = sev_common->sev_features & ~SVM_SEV_FEAT_SNP_ACTIVE;
> +
> ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error);
> break;
> }
> @@ -2518,9 +2524,6 @@ static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
> __func__);
> return -1;
> }
> - if (check_sev_features(sev_common, sa->sev_features, errp) < 0) {
> - return -1;
> - }
> sev_common->sev_features = sa->sev_features;
> }
> return 0;
> @@ -3127,6 +3130,7 @@ sev_snp_guest_instance_init(Object *obj)
>
> /* default init/start/finish params for kvm */
> sev_snp_guest->kvm_start_conf.policy = DEFAULT_SEV_SNP_POLICY;
> + SEV_COMMON(sev_snp_guest)->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
> }
>
> /* guest info specific to sev-snp */
Hi Tom, On Fri, Sep 12, 2025 at 08:39:09AM -0500, Tom Lendacky wrote: > On 9/11/25 06:54, Naveen N Rao (AMD) wrote: > > Currently, check_sev_features() is called in multiple places when > > processing IGVM files: both when processing the initial VMSA SEV > > features from IGVM, as well as when validating the full contents of the > > VMSA. Move this to a single point in sev_common_kvm_init() to simplify > > the flow, as well as to re-use this function when VMSA SEV features are > > being set without using IGVM files. > > > > Since check_sev_features() relies on SVM_SEV_FEAT_SNP_ACTIVE being set > > in VMSA SEV features depending on the guest type, set this flag by > > default when creating SEV-SNP guests. When using IGVM files, this field > > is anyway over-written so that validation in check_sev_features() is > > still relevant. > > There seem to be multiple things going on in this patch and I wonder if it > would be best to split it up into separate smaller patches. > > You have setting of SVM_SEV_FEAT_SNP_ACTIVE in sev_features, you have a > new check for sev_features being set when using an IGVM file and you have > the consolidation. Sure, I started with the premise of unifying the call to check_sev_features() which necessitated the other changes. I will move those as pre-req patches. Thanks for the review, - Naveen
© 2016 - 2025 Red Hat, Inc.