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

[Nikunj A Dadhania <nikunj@amd.com>]

Sean Christopherson <seanjc@google.com> writes:

> 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.

Yes, and had done the previous version because that.

> 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.

Sure, will test and send updated 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))

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

Newer QEMU with support for ghcb_version = 3 and older KVM hypervisor
that still does not have say version 3 supported ?

> +       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;

Makes sense and is clear.

Thanks
Nikunj