Sean Christopherson <seanjc@xxxxxxxxxx> 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
