On Thu, Feb 13, 2025, Kim Phillips wrote:
> On 2/11/25 3:46 PM, Sean Christopherson wrote:
> > On Mon, Feb 10, 2025, Tom Lendacky wrote:
> > > On 2/7/25 17:34, Kim Phillips wrote:
> > > > 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...
>
> This contradicts your review comment from the previous version of the series [1].
First off, my comment was anything but decisive. I don't see how anyone can read
this and come away thinking "this is exactly what Sean wants".
This may need additional uAPI so that userspace can opt-in. Dunno. I hope guests
aren't abusing features, but IIUC, flipping this on has the potential to break
existing VMs, correct?
Second, there's a clear question there that went unanswered. Respond to questions
and elaborate as needed until we're all on the same page. Don't just send patches.
Third, letting userspace opt-in to something doesn't necessarily mean giving
userspace full control. Which is the entire reason I asked the question about
whether or not this can break userspace. E.g. we can likely get away with only
making select features opt-in, and enforcing everything else by default.
I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM
cooperation, so enforcing those shouldn't break anything.
It's still not clear to me that we don't have a bug with DEBUG_SWAP. AIUI,
DEBUG_SWAP is allowed by default. I.e. if ALLOWED_FEATURES is unsupported, then
the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge.
So _maybe_ we have to let userspace opt-in to enforcing DEBUG_SWAP, but I suspect
we can get away with fully enabling ALLOWED_FEATURES without userspace's blessing.
> If KVM enforces ALLOWED_SEV_FEATURES, it can break existing VMs, thus
> the explicit userspace allowed-sev-features=on opt-in [2].
>
> Thanks,
>
> Kim
>
> [1] https://lore.kernel.org/kvm/ZsfKYHFkWA-Rh23C@xxxxxxxxxx/
> [2] https://lore.kernel.org/kvm/20250207233327.130770-1-kim.phillips@xxxxxxx/
