Re: [PATCH v3 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

[Sean Christopherson <seanjc@google.com>]

On Mon, Feb 10, 2025, Tom Lendacky wrote:
> On 2/7/25 17:34, Kim Phillips wrote:
> > @@ -289,6 +291,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_
> >  #define SVM_SEV_FEAT_RESTRICTED_INJECTION		BIT(3)
> >  #define SVM_SEV_FEAT_ALTERNATE_INJECTION		BIT(4)
> >  #define SVM_SEV_FEAT_DEBUG_SWAP				BIT(5)
> > +#define SVM_SEV_FEAT_ALLOWED_SEV_FEATURES		BIT_ULL(63)
> 
> Hmmm... I believe it is safe to define this bit value, as the Allowed
> SEV features VMCB field shows bits 61:0 being used for the allowed
> features mask and we know that the SEV_FEATURES field is used in the SEV
> Features MSR left-shifted 2 bits, so we only expect bits 61:0 to be used
> and bits 62 and 63 will always be reserved. But, given that I think we
> need two functions:
> 
> - get_allowed_sev_features()
>   keeping it as you have it below, where it returns the
>   sev->vmsa_features bitmap if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is set
>   or 0 if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is not set.
> 
> - get_vmsa_sev_features()
>   which removes the SVM_SEV_FEAT_ALLOWED_SEV_FEATURES bit, since it is
>   not defined in the VMSA SEV_FEATURES definition.

Or just don't add wrappers that do more harm than good?

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index a9e16792cac0..4d0b5a020b65 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -894,15 +894,6 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
        return 0;
 }
 
-static u64 allowed_sev_features(struct kvm_sev_info *sev)
-{
-       if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
-           (sev->vmsa_features & SVM_SEV_FEAT_ALLOWED_SEV_FEATURES))
-               return sev->vmsa_features;
-
-       return 0;
-}
-
 static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
                                    int *error)
 {
@@ -916,7 +907,8 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
                return -EINVAL;
        }
 
-       svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
+       if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES))
+               svm->vmcb->control.allowed_sev_features = sev->vmsa_features;
 
        /* Perform some pre-encryption checks against the VMSA */
        ret = sev_es_sync_vmsa(svm);
@@ -2459,7 +2451,8 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
                struct vcpu_svm *svm = to_svm(vcpu);
                u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
 
-               svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
+               if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES))
+                       svm->vmcb->control.allowed_sev_features = sev->vmsa_features;
 
                ret = sev_es_sync_vmsa(svm);
                if (ret)

> >  #define SVM_SEV_FEAT_INT_INJ_MODES		\
> >  	(SVM_SEV_FEAT_RESTRICTED_INJECTION |	\
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index a2a794c32050..a9e16792cac0 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -894,9 +894,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
> >  	return 0;
> >  }
> >  
> > +static u64 allowed_sev_features(struct kvm_sev_info *sev)
> > +{
> > +	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
> 
> Not sure if the cpu_feature_enabled() check is necessary, as init should
> have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in
> sev_supported_vmsa_features.

Two things missing from this series:

 1: KVM enforcement.  No way is KVM going to rely on userspace to opt-in to
    preventing the guest from enabling features.

 2: Backwards compatilibity if KVM unconditionally enforces ALLOWED_SEV_FEATURES.
    Although maybe there's nothing to do here?  I vaguely recall all of the gated
    features being unsupported, or something...