Re: [PATCH v7 8/9] KVM: x86: nSVM: Save/restore gPAT with KVM_{GET,SET}_NESTED_STATE

[Jim Mattson]

On Tue, Apr 7, 2026 at 7:14 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Mon, Apr 06, 2026, Jim Mattson wrote:
> > On Mon, Apr 6, 2026 at 4:47 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Mar 27, 2026, Jim Mattson wrote:
> > > > @@ -1918,6 +1921,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> > > >       struct vmcb_save_area_cached save_cached;
> > > >       struct vmcb_ctrl_area_cached ctl_cached;
> > > >       unsigned long cr0;
> > > > +     bool use_separate_l2_pat;
> > >
> > > Land this above "cr0" to preserve the inverted fir tree.
> > >
> > > >       int ret;
> > > >
> > > >       BUILD_BUG_ON(sizeof(struct vmcb_control_area) + sizeof(struct vmcb_save_area) >
> > > > @@ -1993,6 +1997,18 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
> > > >           !nested_vmcb_check_save(vcpu, &save_cached, false))
> > > >               goto out_free;
> > > >
> > > > +     /*
> > > > +      * Validate gPAT when the shared PAT quirk is disabled (i.e. L2
> > > > +      * has its own gPAT). This is done separately from the
> > > > +      * vmcb_save_area_cached validation above, because gPAT is L2
> > > > +      * state, but the vmcb_save_area_cached is populated with L1 state.
> > > > +      */
> > > > +     use_separate_l2_pat =
> > > > +             (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP) &&
> > > > +             !kvm_check_has_quirk(vcpu->kvm,
> > > > +                                  KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
> > >
> > > I vote for either:
> > >
> > >         use_separate_l2_pat = (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP) &&
> > >                               !kvm_check_has_quirk(vcpu->kvm,
> > >                                                    KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT);
> > >
> > LOL! Aren't you the one who keeps complaining that my indentation
> > doesn't line up? Are you schizophrenic?
>
> Huh?  That is aligned.  Perhaps it's whitespace damaged by your MUA?

Indeed. It was.

> > > or
> > >
> > >         use_separate_l2_pat = (ctl_cached.misc_ctl & SVM_MISC_ENABLE_NP);
> > >         if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_NESTED_SVM_SHARED_PAT))
> > >                 use_separate_l2_pat = false;
> >
> > Wow. I really have no idea how to predict what you're going to want
> > the code to look like. How is this better than the original?!?
>
> It doesn't immediately wrap after the "=".  Similar to my view on wrapping before
> function names[*], I find wrapping immediately after an assignment operator to be
> unnecessarily difficult to read as it doesn't provide any context for single-line
> searches.

That's actually a good argument to *never* wrap a line. If a line is
broken at all, the interesting context might follow the line break.

> I'm pretty darn consistent in my dislike for that style: I count 26 instances in
> arch/x86/kvm that match "\s=\n", and only two of those carry my SoB or R-b.  I
> simply missed the wrap in kvm_vcpu_apicv_activated() that was added by commit
> 896046474f8d ("KVM: x86: Introduce kvm_x86_call() to simplify static calls of
> kvm_x86_ops"), and I'll give myself a pass for commit 8764ed55c970 ("KVM: x86:
> Whitelist port 0x7e for pre-incrementing %rip") as that predates treating
> checkpatch's 80 char limit as a soft limit.

Might I suggest that you should provide a tool—something like
checkpatch.pl—that flags style violations?