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;

On Thu, Jul 24, 2025, Nikunj A Dadhania wrote:

Sean Christopherson seanjc@google.com writes:

...

@@ -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))

Any specific reason to get rid of the first check for GHCB_VERSION_MAX ?

Purely a goof on my part, I didn't intend to drop it.