Please trim your replies. Scrolling through 100+ lines of quoted text to find
the ~12 lines of context that actually matter is annoying.
On Fri, Feb 13, 2026, Yosry Ahmed wrote:
> On Thu, Feb 12, 2026 at 07:58:52AM -0800, Jim Mattson wrote:
> > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> > index a49c48459e0b..88549705133f 100644
> > --- a/arch/x86/kvm/svm/svm.h
> > +++ b/arch/x86/kvm/svm/svm.h
> > @@ -607,6 +607,22 @@ static inline bool nested_npt_enabled(struct vcpu_svm
> > *svm)
> > return svm->nested.ctl.misc_ctl & SVM_MISC_ENABLE_NP;
> > }
> >
> > +static inline void svm_set_gpat(struct vcpu_svm *svm, u64 data)
> > +{
> > + svm->nested.save.g_pat = data;
> > + vmcb_set_gpat(svm->nested.vmcb02.ptr, data);
> > +}
> > +
> > +static inline void svm_set_hpat(struct vcpu_svm *svm, u64 data)
> > +{
> > + svm->vcpu.arch.pat = data;
> > + if (npt_enabled) {
Peeking at the future patches, if we make this:
if (!npt_enabled)
return;
then we can end up with this:
if (npt_enabled)
return;
vmcb_set_gpat(svm->vmcb01.ptr, data);
if (is_guest_mode(&svm->vcpu) && !nested_npt_enabled(svm))
vmcb_set_gpat(svm->nested.vmcb02.ptr, data);
if (svm->nested.legacy_gpat_semantics)
svm_set_l2_pat(svm, data);
Because legacy_gpat_semantics can only be true if npt_enabled is true. Without
that guard, KVM _looks_ buggy because it's setting gpat in the VMCB even when
it shouldn't exist.
Actually, calling svm_set_l2_pat() when !is_guest_mode() is wrong too, no? E.g.
shouldn't we end up with this?
static inline void svm_set_l1_pat(struct vcpu_svm *svm, u64 data)
{
svm->vcpu.arch.pat = data;
if (npt_enabled)
return;
vmcb_set_gpat(svm->vmcb01.ptr, data);
if (is_guest_mode(&svm->vcpu)) {
if (svm->nested.legacy_gpat_semantics)
svm_set_l2_pat(svm, data);
else if (!nested_npt_enabled(svm))
vmcb_set_gpat(svm->nested.vmcb02.ptr, data);
}
}
> > + vmcb_set_gpat(svm->vmcb01.ptr, data);
> > + if (is_guest_mode(&svm->vcpu) && !nested_npt_enabled(svm))
> > + vmcb_set_gpat(svm->nested.vmcb02.ptr, data);
> > + }
> > +}
>
> Is it me, or is it a bit confusing that svm_set_gpat() sets L2's gPAT
> not L1's, and svm_set_hpat() calls vmcb_set_gpat()?
It's not just you. I don't find it confusing per se, more that it's really
subtle.
> "gpat" means different things in the context of the VMCB or otherwise,
> which kinda makes sense but is also not super clear. Maybe
> svm_set_l1_gpat() and svm_set_l2_gpat() is more clear?
I think just svm_set_l1_pat() and svm_set_l2_pat(), because gpat straight up
doesn't exist when NPT is disabled/unsupported.
