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. 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) <[email protected]> > --- > 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 */
