Re: [RFC PATCH 1/7] target/i386: SEV: Consolidate SEV feature validation to common init path

Tom Lendacky
2025-09-12T13:39:13Z

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) <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 */