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