Re: [PATCH v2] KVM: SEV: Enforce minimum GHCB version requirement for SEV-SNP guests

[Sean Christopherson <seanjc@google.com>]

On Wed, Jul 16, 2025, Tom Lendacky wrote:
> On 7/16/25 00:56, Nikunj A Dadhania wrote:
> > ---
> >  arch/x86/kvm/svm/sev.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 95668e84ab86..fdc1309c68cb 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -406,6 +406,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
> >  	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> >  	struct sev_platform_init_args init_args = {0};
> >  	bool es_active = vm_type != KVM_X86_SEV_VM;
> > +	bool snp_active = vm_type == KVM_X86_SNP_VM;
> >  	u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
> >  	int ret;
> >  
> > @@ -424,6 +425,9 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
> >  	if (unlikely(sev->active))
> >  		return -EINVAL;
> >  
> > +	if (snp_active && data->ghcb_version && data->ghcb_version < 2)
> > +		return -EINVAL;
> > +
> 
> Would it make sense to move this up a little bit so that it follows the
> other ghcb_version check? This way the checks are grouped.

Yes, because there's a lot going on here, and this:

  data->ghcb_version && data->ghcb_version < 2

is an unnecesarily bizarre way of writing

  data->ghcb_version == 1

And *that* is super confusing because it begs the question of why version 0 is
ok, but version 1 is not.  And then further down I see this: 

	/*
	 * Currently KVM supports the full range of mandatory features defined
	 * by version 2 of the GHCB protocol, so default to that for SEV-ES
	 * guests created via KVM_SEV_INIT2.
	 */
	if (sev->es_active && !sev->ghcb_version)
		sev->ghcb_version = GHCB_VERSION_DEFAULT;

Rather than have a funky sequence with odd logic, set data->ghcb_version before
the SNP check.  We should also tweak the comment, because "Currently" implies
that KVM might *drop* support for mandatory features, and that definitely isn't
going to happen.  And because the reader shouldn't have to go look at sev_guest_init()
to understand what's special about KVM_SEV_INIT2.

Lastly, I think we should open code '2' and drop GHCB_VERSION_DEFAULT, because:

 - it's a conditional default
 - is not enumerated to userspace
 - changing GHCB_VERSION_DEFAULT will impact ABI and could break existing setups
 - will result in a stale if GHCB_VERSION_DEFAULT is modified
 - this new check makes me want to assert GHCB_VERSION_DEFAULT > 2

As a result, if we combine all of the above, then we effectively end up with:

	if (es_active && !data->ghcb_version)
		data->ghcb_version = GHCB_VERSION_DEFAULT;

	BUILD_BUG_ON(GHCB_VERSION_DEFAULT != 2);

which is quite silly.

So this?  Completely untested, and should probably be split over 2-3 patches.

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 2fbdebf79fbb..f068cd466ae3 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -37,7 +37,6 @@
 #include "trace.h"
 
 #define GHCB_VERSION_MAX       2ULL
-#define GHCB_VERSION_DEFAULT   2ULL
 #define GHCB_VERSION_MIN       1ULL
 
 #define GHCB_HV_FT_SUPPORTED   (GHCB_HV_FT_SNP | GHCB_HV_FT_SNP_AP_CREATION)
@@ -405,6 +404,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 {
        struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
        struct sev_platform_init_args init_args = {0};
+       bool snp_active = vm_type == KVM_X86_SNP_VM;
        bool es_active = vm_type != KVM_X86_SEV_VM;
        u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
        int ret;
@@ -418,7 +418,18 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        if (data->vmsa_features & ~valid_vmsa_features)
                return -EINVAL;
 
-       if (data->ghcb_version > GHCB_VERSION_MAX || (!es_active && data->ghcb_version))
+       if (!es_active && data->ghcb_version)
+               return -EINVAL;
+
+       /*
+        * KVM supports the full range of mandatory features defined by version
+        * 2 of the GHCB protocol, so default to that for SEV-ES guests created
+        * via KVM_SEV_INIT2 (KVM_SEV_INIT forces version 1).
+        */
+       if (es_active && !data->ghcb_version)
+               data->ghcb_version = 2;
+
+       if (snp_active && data->ghcb_version < 2)
                return -EINVAL;
 
        if (unlikely(sev->active))
@@ -429,15 +440,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        sev->vmsa_features = data->vmsa_features;
        sev->ghcb_version = data->ghcb_version;
 
-       /*
-        * Currently KVM supports the full range of mandatory features defined
-        * by version 2 of the GHCB protocol, so default to that for SEV-ES
-        * guests created via KVM_SEV_INIT2.
-        */
-       if (sev->es_active && !sev->ghcb_version)
-               sev->ghcb_version = GHCB_VERSION_DEFAULT;
-
-       if (vm_type == KVM_X86_SNP_VM)
+       if (snp_active)
                sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
 
        ret = sev_asid_new(sev);
@@ -455,7 +458,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        }
 
        /* This needs to happen after SEV/SNP firmware initialization. */
-       if (vm_type == KVM_X86_SNP_VM) {
+       if (snp_active) {
                ret = snp_guest_req_init(kvm);
                if (ret)
                        goto e_free;