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