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

Yosry Ahmed
2025-12-09T18:35:08Z

On Tue, Dec 09, 2025 at 10:26:31AM -0800, Sean Christopherson wrote:
> 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".

Ack.

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

I see, so clear SVM_NESTED_CTL_NP_ENABLE in
__nested_copy_vmcb_control_to_cache() instead.

If I drop the guest_cpu_cap_has() check here I will want to leave a
comment so that it's obvious to readers that SVM_NESTED_CTL_NP_ENABLE is
sanitized elsewhere if the guest cannot use NPTs. Alternatively, I can
just keep the guest_cpu_cap_has() check as documentation and a second
line of defense.

Any preferences?