Re: [PATCH v2 04/13] KVM: nSVM: Fix consistency checks for NP_ENABLE

Sean Christopherson
2025-12-09T18:26:33Z

On Tue, Dec 09, 2025, Yosry Ahmed wrote:
> On Tue, Dec 09, 2025 at 08:27:39AM -0800, Sean Christopherson wrote:
> > On Mon, Nov 10, 2025, Yosry Ahmed wrote:
> > > @@ -400,7 +405,12 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu)
> > > struct vcpu_svm *svm = to_svm(vcpu);
> > > struct vmcb_ctrl_area_cached *ctl = &svm->nested.ctl;
> > >
> > > - return __nested_vmcb_check_controls(vcpu, ctl);
> > > + /*
> > > + * Make sure we did not enter guest mode yet, in which case
> >
> > No pronouns.
>
> I thought that rule was for commit logs.

In KVM x86, it's a rule everywhere. Pronouns often add ambiguity, and it's much
easier to have a hard "no pronouns" rule than to try and enforce an inherently
subjective "is this ambiguous or not" rule.

> There are plenty of 'we's in the KVM x86 code (and all x86 code for that
> matter) :P

Ya, KVM is an 18+ year old code base. There's also a ton of bare "unsigned" usage,
and other things that are frowned upon and/or flagged by checkpatch. I'm all for
cleaning things up when touching the code, but I'm staunchly against "tree"-wide
cleanups just to make checkpatch happy, and so there's quite a few historical
violations of the current "rules".

> > > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> > > index f6fb70ddf7272..3e805a43ffcdb 100644
> > > --- a/arch/x86/kvm/svm/svm.h
> > > +++ b/arch/x86/kvm/svm/svm.h
> > > @@ -552,7 +552,8 @@ static inline bool gif_set(struct vcpu_svm *svm)
> > >
> > > static inline bool nested_npt_enabled(struct vcpu_svm *svm)
> > > {
> > > - return svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
> > > + return guest_cpu_cap_has(&svm->vcpu, X86_FEATURE_NPT) &&
> > > + svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
> >
> > I would rather rely on Kevin's patch to clear unsupported features.
>
> Not sure how Kevin's patch is relevant here, could you please clarify?

Doh, Kevin's patch only touches intercepts. What I was trying to say is that I
would rather sanitize the snapshot (the approach Kevin's patch takes with the
intercepts), as opposed to guarding the accessor. That way we can't have bugs
where KVM checks svm->nested.ctl.nested_ctl directly and bypasses the caps check.