[PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT

Jim Mattson posted 8 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Jim Mattson 1 month, 2 weeks ago
When the vCPU is in guest mode with nested NPT enabled, guest accesses to
IA32_PAT are redirected to the gPAT register, which is stored in
svm->nested.save.g_pat.

Non-guest accesses (e.g. from userspace) to IA32_PAT are always redirected
to hPAT, which is stored in vcpu->arch.pat.

This is architected behavior. It also makes it possible to restore a new
checkpoint on an old kernel with reasonable semantics. After the restore,
gPAT will be lost, and L2 will run on L1's PAT. Note that the old kernel
would have always run L2 on L1's PAT.

Add WARN_ON_ONCE to both svm_get_msr() and svm_set_msr() to flag any
host-initiated accesses originating from KVM itself rather than userspace.

Fixes: 15038e147247 ("KVM: SVM: obey guest PAT")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/svm/nested.c |  9 ---------
 arch/x86/kvm/svm/svm.c    | 37 ++++++++++++++++++++++++++++++-------
 arch/x86/kvm/svm/svm.h    | 17 ++++++++++++++++-
 3 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index dc8275837120..69b577a4915c 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -706,15 +706,6 @@ static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
 	return 0;
 }
 
-void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm)
-{
-	if (!svm->nested.vmcb02.ptr)
-		return;
-
-	/* FIXME: merge g_pat from vmcb01 and vmcb12.  */
-	vmcb_set_gpat(svm->nested.vmcb02.ptr, svm->vmcb01.ptr->save.g_pat);
-}
-
 static void nested_vmcb02_prepare_save(struct vcpu_svm *svm)
 {
 	struct vmcb_ctrl_area_cached *control = &svm->nested.ctl;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 529cbac57814..205bf07896ad 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2837,6 +2837,21 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_AMD64_DE_CFG:
 		msr_info->data = svm->msr_decfg;
 		break;
+	case MSR_IA32_CR_PAT:
+		/*
+		 * When nested NPT is enabled, L2 has a separate PAT from
+		 * L1.  Guest accesses to IA32_PAT while running L2 target
+		 * L2's gPAT; host-initiated accesses always target L1's
+		 * hPAT for backward and forward KVM_GET_MSRS compatibility
+		 * with older kernels.
+		 */
+		WARN_ON_ONCE(msr_info->host_initiated && vcpu->wants_to_run);
+		if (!msr_info->host_initiated && is_guest_mode(vcpu) &&
+		    nested_npt_enabled(svm))
+			msr_info->data = svm->nested.save.g_pat;
+		else
+			msr_info->data = vcpu->arch.pat;
+		break;
 	default:
 		return kvm_get_msr_common(vcpu, msr_info);
 	}
@@ -2920,13 +2935,21 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 
 		break;
 	case MSR_IA32_CR_PAT:
-		ret = kvm_set_msr_common(vcpu, msr);
-		if (ret)
-			break;
-
-		vmcb_set_gpat(svm->vmcb01.ptr, data);
-		if (is_guest_mode(vcpu))
-			nested_vmcb02_compute_g_pat(svm);
+		if (!kvm_pat_valid(data))
+			return 1;
+		/*
+		 * When nested NPT is enabled, L2 has a separate PAT from
+		 * L1.  Guest accesses to IA32_PAT while running L2 target
+		 * L2's gPAT; host-initiated accesses always target L1's
+		 * hPAT for backward and forward KVM_SET_MSRS compatibility
+		 * with older kernels.
+		 */
+		WARN_ON_ONCE(msr->host_initiated && vcpu->wants_to_run);
+		if (!msr->host_initiated && is_guest_mode(vcpu) &&
+		    nested_npt_enabled(svm))
+			svm_set_gpat(svm, data);
+		else
+			svm_set_hpat(svm, data);
 		break;
 	case MSR_IA32_SPEC_CTRL:
 		if (!msr->host_initiated &&
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) {
+		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);
+	}
+}
+
 static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
 {
 	return guest_cpu_cap_has(&svm->vcpu, X86_FEATURE_VNMI) &&
@@ -840,7 +856,6 @@ void nested_copy_vmcb_control_to_cache(struct vcpu_svm *svm,
 void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
 				    struct vmcb_save_area *save);
 void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);
-void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm);
 void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb);
 
 extern struct kvm_x86_nested_ops svm_nested_ops;
-- 
2.53.0.239.g8d8fc8a987-goog
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Yosry Ahmed 1 month, 2 weeks ago
On Thu, Feb 12, 2026 at 07:58:52AM -0800, Jim Mattson wrote:
> When the vCPU is in guest mode with nested NPT enabled, guest accesses to
> IA32_PAT are redirected to the gPAT register, which is stored in
> svm->nested.save.g_pat.
> 
> Non-guest accesses (e.g. from userspace) to IA32_PAT are always redirected
> to hPAT, which is stored in vcpu->arch.pat.
> 
> This is architected behavior. It also makes it possible to restore a new
> checkpoint on an old kernel with reasonable semantics. After the restore,
> gPAT will be lost, and L2 will run on L1's PAT. Note that the old kernel
> would have always run L2 on L1's PAT.
> 
> Add WARN_ON_ONCE to both svm_get_msr() and svm_set_msr() to flag any
> host-initiated accesses originating from KVM itself rather than userspace.
> 
> Fixes: 15038e147247 ("KVM: SVM: obey guest PAT")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/svm/nested.c |  9 ---------
>  arch/x86/kvm/svm/svm.c    | 37 ++++++++++++++++++++++++++++++-------
>  arch/x86/kvm/svm/svm.h    | 17 ++++++++++++++++-
>  3 files changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index dc8275837120..69b577a4915c 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -706,15 +706,6 @@ static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
>  	return 0;
>  }
>  
> -void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm)
> -{
> -	if (!svm->nested.vmcb02.ptr)
> -		return;
> -
> -	/* FIXME: merge g_pat from vmcb01 and vmcb12.  */
> -	vmcb_set_gpat(svm->nested.vmcb02.ptr, svm->vmcb01.ptr->save.g_pat);
> -}
> -
>  static void nested_vmcb02_prepare_save(struct vcpu_svm *svm)
>  {
>  	struct vmcb_ctrl_area_cached *control = &svm->nested.ctl;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 529cbac57814..205bf07896ad 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2837,6 +2837,21 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_AMD64_DE_CFG:
>  		msr_info->data = svm->msr_decfg;
>  		break;
> +	case MSR_IA32_CR_PAT:
> +		/*
> +		 * When nested NPT is enabled, L2 has a separate PAT from
> +		 * L1.  Guest accesses to IA32_PAT while running L2 target
> +		 * L2's gPAT; host-initiated accesses always target L1's
> +		 * hPAT for backward and forward KVM_GET_MSRS compatibility
> +		 * with older kernels.
> +		 */
> +		WARN_ON_ONCE(msr_info->host_initiated && vcpu->wants_to_run);
> +		if (!msr_info->host_initiated && is_guest_mode(vcpu) &&
> +		    nested_npt_enabled(svm))
> +			msr_info->data = svm->nested.save.g_pat;
> +		else
> +			msr_info->data = vcpu->arch.pat;
> +		break;
>  	default:
>  		return kvm_get_msr_common(vcpu, msr_info);
>  	}
> @@ -2920,13 +2935,21 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  
>  		break;
>  	case MSR_IA32_CR_PAT:
> -		ret = kvm_set_msr_common(vcpu, msr);
> -		if (ret)
> -			break;
> -
> -		vmcb_set_gpat(svm->vmcb01.ptr, data);
> -		if (is_guest_mode(vcpu))
> -			nested_vmcb02_compute_g_pat(svm);
> +		if (!kvm_pat_valid(data))
> +			return 1;
> +		/*
> +		 * When nested NPT is enabled, L2 has a separate PAT from
> +		 * L1.  Guest accesses to IA32_PAT while running L2 target
> +		 * L2's gPAT; host-initiated accesses always target L1's
> +		 * hPAT for backward and forward KVM_SET_MSRS compatibility
> +		 * with older kernels.
> +		 */
> +		WARN_ON_ONCE(msr->host_initiated && vcpu->wants_to_run);
> +		if (!msr->host_initiated && is_guest_mode(vcpu) &&
> +		    nested_npt_enabled(svm))
> +			svm_set_gpat(svm, data);
> +		else
> +			svm_set_hpat(svm, data);
>  		break;
>  	case MSR_IA32_SPEC_CTRL:
>  		if (!msr->host_initiated &&
> 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) {
> +		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()?

"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?

Will leave it up to Sean to decide if the naming is good enough, but
honestly I don't want to stall this series, so hopefully any renames can
be done as a follow up or when the series is applied.

> +
>  static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
>  {
>  	return guest_cpu_cap_has(&svm->vcpu, X86_FEATURE_VNMI) &&
> @@ -840,7 +856,6 @@ void nested_copy_vmcb_control_to_cache(struct vcpu_svm *svm,
>  void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
>  				    struct vmcb_save_area *save);
>  void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);
> -void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm);
>  void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb);
>  
>  extern struct kvm_x86_nested_ops svm_nested_ops;
> -- 
> 2.53.0.239.g8d8fc8a987-goog
>
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Sean Christopherson 1 month, 2 weeks ago
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.
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Jim Mattson 1 month, 2 weeks ago
On Fri, Feb 13, 2026 at 7:20 AM Sean Christopherson <seanjc@google.com> wrote:
>
> 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?

Sigh. legacy_gpat_semantics is supposed to be set only when
is_guest_mode() and nested_npt_enabled(). I forgot about back-to-back
invocations of KVM_SET_NESTED_STATE. Are there other ways of leaving
guest mode or disabling nested NPT before the next KVM_RUN?

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

My intention was that "gpat" and "hpat" were from the perspective of the vCPU.

I dislike svm_set_l1_pat() and svm_set_l2_pat(). As you point out
above, there is no independent L2 PAT when nested NPT is disabled. I
think that's less obvious than the fact that there is no gPAT from the
vCPU's perspective. My preference is to follow the APM terminology
when possible. Making up our own terms just leads to confusion.
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Sean Christopherson 1 month, 2 weeks ago
On Fri, Feb 13, 2026, Jim Mattson wrote:
> On Fri, Feb 13, 2026 at 7:20 AM Sean Christopherson <seanjc@google.com> wrote:
> > > > +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?
> 
> Sigh. legacy_gpat_semantics is supposed to be set only when
> is_guest_mode() and nested_npt_enabled(). I forgot about back-to-back
> invocations of KVM_SET_NESTED_STATE. Are there other ways of leaving
> guest mode or disabling nested NPT before the next KVM_RUN?

KVM_SET_VCPU_EVENTS will do it if userspace forces a change in SMM state:

		if (!!(vcpu->arch.hflags & HF_SMM_MASK) != events->smi.smm) {
			kvm_leave_nested(vcpu);
			kvm_smm_changed(vcpu, events->smi.smm);
		}

I honestly wasn't even thinking of anything in particular, it just looked weird.

> > > > +           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.
> 
> My intention was that "gpat" and "hpat" were from the perspective of the vCPU.
> 
> I dislike svm_set_l1_pat() and svm_set_l2_pat(). As you point out
> above, there is no independent L2 PAT when nested NPT is disabled. I
> think that's less obvious than the fact that there is no gPAT from the
> vCPU's perspective. My preference is to follow the APM terminology
> when possible. Making up our own terms just leads to confusion.

How about svm_set_pat() and svm_get_gpat()?  Because hPAT doesn't exist when NPT
is unsupported/disabled, but KVM still needs to set the vCPU's emulated PAT value.
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Jim Mattson 1 month, 2 weeks ago
On Fri, Feb 13, 2026 at 2:19 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Feb 13, 2026, Jim Mattson wrote:
> > On Fri, Feb 13, 2026 at 7:20 AM Sean Christopherson <seanjc@google.com> wrote:
> > > > > +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?
> >
> > Sigh. legacy_gpat_semantics is supposed to be set only when
> > is_guest_mode() and nested_npt_enabled(). I forgot about back-to-back
> > invocations of KVM_SET_NESTED_STATE. Are there other ways of leaving
> > guest mode or disabling nested NPT before the next KVM_RUN?
>
> KVM_SET_VCPU_EVENTS will do it if userspace forces a change in SMM state:
>
>                 if (!!(vcpu->arch.hflags & HF_SMM_MASK) != events->smi.smm) {
>                         kvm_leave_nested(vcpu);
>                         kvm_smm_changed(vcpu, events->smi.smm);
>                 }
>
> I honestly wasn't even thinking of anything in particular, it just looked weird.

At the very least, then, kvm_leave_nested() has to clear
legacy_gpat_semantics. I will look for other paths.

> > > > > +           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.
> >
> > My intention was that "gpat" and "hpat" were from the perspective of the vCPU.
> >
> > I dislike svm_set_l1_pat() and svm_set_l2_pat(). As you point out
> > above, there is no independent L2 PAT when nested NPT is disabled. I
> > think that's less obvious than the fact that there is no gPAT from the
> > vCPU's perspective. My preference is to follow the APM terminology
> > when possible. Making up our own terms just leads to confusion.
>
> How about svm_set_pat() and svm_get_gpat()?  Because hPAT doesn't exist when NPT
> is unsupported/disabled, but KVM still needs to set the vCPU's emulated PAT value.

What if we don't break it up this way at all? Instead of distributing
the logic between svm_[gs]set_msr() and a few helper functions, we
could just have svm_[gs]et_msr() call svm_[gs]et_pat(), and all of the
logic can go in these two functions.
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Sean Christopherson 1 month, 1 week ago
On Fri, Feb 13, 2026, Jim Mattson wrote:
> On Fri, Feb 13, 2026 at 2:19 PM Sean Christopherson <seanjc@google.com> wrote:
> > > > > > +           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.
> > >
> > > My intention was that "gpat" and "hpat" were from the perspective of the vCPU.
> > >
> > > I dislike svm_set_l1_pat() and svm_set_l2_pat(). As you point out
> > > above, there is no independent L2 PAT when nested NPT is disabled. I
> > > think that's less obvious than the fact that there is no gPAT from the
> > > vCPU's perspective. My preference is to follow the APM terminology
> > > when possible. Making up our own terms just leads to confusion.
> >
> > How about svm_set_pat() and svm_get_gpat()?  Because hPAT doesn't exist when NPT
> > is unsupported/disabled, but KVM still needs to set the vCPU's emulated PAT value.
> 
> What if we don't break it up this way at all? Instead of distributing
> the logic between svm_[gs]set_msr() and a few helper functions, we
> could just have svm_[gs]et_msr() call svm_[gs]et_pat(), and all of the
> logic can go in these two functions.

I like it.  And AFAICT it largely Just Works, because the calls from
svm_set_nested_state() will always be routed to gpat since the calls are already
guarded with is_guest_mode() + nested_npt_enabled().

Side topic, either as a prep patch (to duplicate code) or as a follow-up patch
(to move the PAT handling in x86.c to vmx.c), the "common" handling of PAT should
be fully forked between VMX and SVM.  As of this patch, it's not just misleading,
it's actively dangerous since calling kvm_get_msr_common() for SVM would get the
wrong value.

FWIW, this is what I ended up with when hacking on top of your patches to see how
this played out.

---
 arch/x86/kvm/svm/nested.c |  4 +--
 arch/x86/kvm/svm/svm.c    | 64 +++++++++++++++++++++++++--------------
 arch/x86/kvm/svm/svm.h    | 19 +-----------
 arch/x86/kvm/vmx/vmx.c    | 10 ++++--
 arch/x86/kvm/x86.c        |  9 ------
 5 files changed, 51 insertions(+), 55 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index d854d29b0bd8..361f189d3967 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -2075,9 +2075,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 
 	if (nested_npt_enabled(svm)) {
 		if (kvm_state->hdr.svm.flags & KVM_STATE_SVM_VALID_GPAT) {
-			svm_set_gpat(svm, kvm_state->hdr.svm.gpat);
+			svm_set_pat(vcpu, kvm_state->hdr.svm.gpat, true);
 		} else {
-			svm_set_gpat(svm, vcpu->arch.pat);
+			svm_set_pat(vcpu, vcpu->arch.pat, true);
 			svm->nested.legacy_gpat_semantics = true;
 		}
 	}
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 93ce0c3232c6..94c3b3cadd54 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -251,6 +251,44 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	return 0;
 }
 
+static bool svm_is_access_to_gpat(struct kvm_vcpu *vcpu, bool host_initiated)
+{
+	/*
+	 * When nested NPT is enabled, L2 has a separate PAT from L1.  Guest
+	 * accesses to IA32_PAT while running L2 target L2's gPAT;
+	 * host-initiated accesses always target L1's hPAT for backward and
+	 * forward KVM_SET_MSRS compatibility with older kernels.
+	 */
+	WARN_ON_ONCE(host_initiated && vcpu->wants_to_run);
+
+	return !host_initiated && is_guest_mode(vcpu) &&
+	       nested_npt_enabled(to_svm(vcpu));
+}
+
+void svm_set_pat(struct kvm_vcpu *vcpu, u64 pat, bool host_initiated)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (svm_is_access_to_gpat(vcpu, host_initiated)) {
+		vmcb_set_gpat(svm->nested.vmcb02.ptr, pat);
+		return;
+	}
+
+	svm->vcpu.arch.pat = pat;
+
+	if (!npt_enabled)
+		return;
+
+	vmcb_set_gpat(svm->vmcb01.ptr, pat);
+
+	if (svm->nested.legacy_gpat_semantics) {
+		svm->nested.save.g_pat = pat;
+		vmcb_set_gpat(svm->nested.vmcb02.ptr, pat);
+	} else if (is_guest_mode(&svm->vcpu) && !nested_npt_enabled(svm)) {
+		vmcb_set_gpat(svm->nested.vmcb02.ptr, pat);
+	}
+}
+
 static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -2838,16 +2876,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = svm->msr_decfg;
 		break;
 	case MSR_IA32_CR_PAT:
-		/*
-		 * When nested NPT is enabled, L2 has a separate PAT from
-		 * L1.  Guest accesses to IA32_PAT while running L2 target
-		 * L2's gPAT; host-initiated accesses always target L1's
-		 * hPAT for backward and forward KVM_GET_MSRS compatibility
-		 * with older kernels.
-		 */
-		WARN_ON_ONCE(msr_info->host_initiated && vcpu->wants_to_run);
-		if (!msr_info->host_initiated && is_guest_mode(vcpu) &&
-		    nested_npt_enabled(svm))
+		if (svm_is_access_to_gpat(vcpu, msr_info->host_initiated))
 			msr_info->data = svm->nested.save.g_pat;
 		else
 			msr_info->data = vcpu->arch.pat;
@@ -2937,19 +2966,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	case MSR_IA32_CR_PAT:
 		if (!kvm_pat_valid(data))
 			return 1;
-		/*
-		 * When nested NPT is enabled, L2 has a separate PAT from
-		 * L1.  Guest accesses to IA32_PAT while running L2 target
-		 * L2's gPAT; host-initiated accesses always target L1's
-		 * hPAT for backward and forward KVM_SET_MSRS compatibility
-		 * with older kernels.
-		 */
-		WARN_ON_ONCE(msr->host_initiated && vcpu->wants_to_run);
-		if (!msr->host_initiated && is_guest_mode(vcpu) &&
-		    nested_npt_enabled(svm))
-			svm_set_gpat(svm, data);
-		else
-			svm_set_hpat(svm, data);
+
+		svm_set_pat(vcpu, data, msr->host_initiated);
 		break;
 	case MSR_IA32_SPEC_CTRL:
 		if (!msr->host_initiated &&
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 0bb9fdcb489d..71502db3f679 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -616,24 +616,6 @@ 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) {
-		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_gpat(svm, data);
-}
-
 static inline bool nested_vnmi_enabled(struct vcpu_svm *svm)
 {
 	return guest_cpu_cap_has(&svm->vcpu, X86_FEATURE_VNMI) &&
@@ -780,6 +762,7 @@ void svm_enable_lbrv(struct kvm_vcpu *vcpu);
 void svm_update_lbrv(struct kvm_vcpu *vcpu);
 
 int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer);
+void svm_set_pat(struct kvm_vcpu *vcpu, u64 pat, bool host_initiated);
 void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
 void svm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
 void disable_nmi_singlestep(struct vcpu_svm *svm);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 967b58a8ab9d..546056e690eb 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2141,6 +2141,9 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 #endif
 	case MSR_EFER:
 		return kvm_get_msr_common(vcpu, msr_info);
+	case MSR_IA32_CR_PAT:
+		msr_info->data = vcpu->arch.pat;
+		break;
 	case MSR_IA32_TSX_CTRL:
 		if (!msr_info->host_initiated &&
 		    !(vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR))
@@ -2468,9 +2471,10 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return 1;
 		goto find_uret_msr;
 	case MSR_IA32_CR_PAT:
-		ret = kvm_set_msr_common(vcpu, msr_info);
-		if (ret)
-			break;
+		if (!kvm_pat_valid(data))
+			return 1;
+
+		vcpu->arch.pat = data;
 
 		if (is_guest_mode(vcpu) &&
 		    get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 416899b5dbe4..41936f83a17f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4025,12 +4025,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return 1;
 		}
 		break;
-	case MSR_IA32_CR_PAT:
-		if (!kvm_pat_valid(data))
-			return 1;
-
-		vcpu->arch.pat = data;
-		break;
 	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
 	case MSR_MTRRdefType:
 		return kvm_mtrr_set_msr(vcpu, msr, data);
@@ -4436,9 +4430,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = kvm_scale_tsc(rdtsc(), ratio) + offset;
 		break;
 	}
-	case MSR_IA32_CR_PAT:
-		msr_info->data = vcpu->arch.pat;
-		break;
 	case MSR_MTRRcap:
 	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
 	case MSR_MTRRdefType:

base-commit: 7539434a6984ba5accfdd8e296fb834558f95df4
--
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Jim Mattson 6 days, 21 hours ago
On Tue, Feb 17, 2026 at 3:27 PM Sean Christopherson <seanjc@google.com> wrote:

> Side topic, either as a prep patch (to duplicate code) or as a follow-up patch
> (to move the PAT handling in x86.c to vmx.c), the "common" handling of PAT should
> be fully forked between VMX and SVM.  As of this patch, it's not just misleading,
> it's actively dangerous since calling kvm_get_msr_common() for SVM would get the
> wrong value.

Though I included this change in v5 and v6, TIL that TDX calls
kvm_[gs]et_msr_common(MSR_IA32_CR_PAT), so the common handling is not
fully forked.
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Yosry Ahmed 6 days, 21 hours ago
On Thu, Mar 26, 2026 at 2:19 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Tue, Feb 17, 2026 at 3:27 PM Sean Christopherson <seanjc@google.com> wrote:
>
> > Side topic, either as a prep patch (to duplicate code) or as a follow-up patch
> > (to move the PAT handling in x86.c to vmx.c), the "common" handling of PAT should
> > be fully forked between VMX and SVM.  As of this patch, it's not just misleading,
> > it's actively dangerous since calling kvm_get_msr_common() for SVM would get the
> > wrong value.
>
> Though I included this change in v5 and v6, TIL that TDX calls
> kvm_[gs]et_msr_common(MSR_IA32_CR_PAT), so the common handling is not
> fully forked.

Do you plan to drop this patch or add PAT handling in
tdx_{get/set}_msr()? If you'll drop it, maybe add a warning if
kvm_[gs]et_msr_common(MSR_IA32_CR_PAT) is called from SVM?
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Jim Mattson 6 days, 21 hours ago
On Thu, Mar 26, 2026 at 2:26 PM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Thu, Mar 26, 2026 at 2:19 PM Jim Mattson <jmattson@google.com> wrote:
> >
> > On Tue, Feb 17, 2026 at 3:27 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > > Side topic, either as a prep patch (to duplicate code) or as a follow-up patch
> > > (to move the PAT handling in x86.c to vmx.c), the "common" handling of PAT should
> > > be fully forked between VMX and SVM.  As of this patch, it's not just misleading,
> > > it's actively dangerous since calling kvm_get_msr_common() for SVM would get the
> > > wrong value.
> >
> > Though I included this change in v5 and v6, TIL that TDX calls
> > kvm_[gs]et_msr_common(MSR_IA32_CR_PAT), so the common handling is not
> > fully forked.
>
> Do you plan to drop this patch or add PAT handling in
> tdx_{get/set}_msr()? If you'll drop it, maybe add a warning if
> kvm_[gs]et_msr_common(MSR_IA32_CR_PAT) is called from SVM?

I plan to leave the MSR_IA32_CR_PAT code in kvm_[gs]et_msr_common().
Replicating the code in tdx.c seems like the wrong direction to go.

There's no precedent that I can see for checking the vendor module in
common code (though we do have kvm_x86_ops.name). I could add a
warning if invoked with (vcpu->arch.efer & EFER_SVME). WDYT?
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Yosry Ahmed 6 days, 21 hours ago
On Thu, Mar 26, 2026 at 2:57 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Thu, Mar 26, 2026 at 2:26 PM Yosry Ahmed <yosry@kernel.org> wrote:
> >
> > On Thu, Mar 26, 2026 at 2:19 PM Jim Mattson <jmattson@google.com> wrote:
> > >
> > > On Tue, Feb 17, 2026 at 3:27 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > > Side topic, either as a prep patch (to duplicate code) or as a follow-up patch
> > > > (to move the PAT handling in x86.c to vmx.c), the "common" handling of PAT should
> > > > be fully forked between VMX and SVM.  As of this patch, it's not just misleading,
> > > > it's actively dangerous since calling kvm_get_msr_common() for SVM would get the
> > > > wrong value.
> > >
> > > Though I included this change in v5 and v6, TIL that TDX calls
> > > kvm_[gs]et_msr_common(MSR_IA32_CR_PAT), so the common handling is not
> > > fully forked.
> >
> > Do you plan to drop this patch or add PAT handling in
> > tdx_{get/set}_msr()? If you'll drop it, maybe add a warning if
> > kvm_[gs]et_msr_common(MSR_IA32_CR_PAT) is called from SVM?
>
> I plan to leave the MSR_IA32_CR_PAT code in kvm_[gs]et_msr_common().
> Replicating the code in tdx.c seems like the wrong direction to go.
>
> There's no precedent that I can see for checking the vendor module in
> common code (though we do have kvm_x86_ops.name). I could add a
> warning if invoked with (vcpu->arch.efer & EFER_SVME). WDYT?

Yeah this should work. I assume Sean can decide to drop it when
applying if he dislikes it, but given that he said the code is
misleading and it would be dangerous if we end up calling from SVM --
I personally think the warning will act as documentation for the
former and safeguard for the latter.
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Yosry Ahmed 1 month, 1 week ago
> I like it.  And AFAICT it largely Just Works, because the calls from
> svm_set_nested_state() will always be routed to gpat since the calls are already
> guarded with is_guest_mode() + nested_npt_enabled().
> 
> Side topic, either as a prep patch (to duplicate code) or as a follow-up patch
> (to move the PAT handling in x86.c to vmx.c), the "common" handling of PAT should
> be fully forked between VMX and SVM.  As of this patch, it's not just misleading,
> it's actively dangerous since calling kvm_get_msr_common() for SVM would get the
> wrong value.

+1 on both points.

> FWIW, this is what I ended up with when hacking on top of your patches to see how
> this played out.
> 
> ---
> @@ -2838,16 +2876,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		msr_info->data = svm->msr_decfg;
>  		break;
>  	case MSR_IA32_CR_PAT:
> -		/*
> -		 * When nested NPT is enabled, L2 has a separate PAT from
> -		 * L1.  Guest accesses to IA32_PAT while running L2 target
> -		 * L2's gPAT; host-initiated accesses always target L1's
> -		 * hPAT for backward and forward KVM_GET_MSRS compatibility
> -		 * with older kernels.
> -		 */
> -		WARN_ON_ONCE(msr_info->host_initiated && vcpu->wants_to_run);
> -		if (!msr_info->host_initiated && is_guest_mode(vcpu) &&
> -		    nested_npt_enabled(svm))
> +		if (svm_is_access_to_gpat(vcpu, msr_info->host_initiated))
>  			msr_info->data = svm->nested.save.g_pat;
>  		else
>  			msr_info->data = vcpu->arch.pat;

I'd go a step further here and add svm_get_pat(), then this just
becomes:

	msr_info->data = svm_get_pat(vcpu, msr_info->host_initiated);

It's more consistent with svm_set_msr(), and completely abstracts the L1
vs. L2 PAT logic with the helpers.

> @@ -2937,19 +2966,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	case MSR_IA32_CR_PAT:
>  		if (!kvm_pat_valid(data))
>  			return 1;
> -		/*
> -		 * When nested NPT is enabled, L2 has a separate PAT from
> -		 * L1.  Guest accesses to IA32_PAT while running L2 target
> -		 * L2's gPAT; host-initiated accesses always target L1's
> -		 * hPAT for backward and forward KVM_SET_MSRS compatibility
> -		 * with older kernels.
> -		 */
> -		WARN_ON_ONCE(msr->host_initiated && vcpu->wants_to_run);
> -		if (!msr->host_initiated && is_guest_mode(vcpu) &&
> -		    nested_npt_enabled(svm))
> -			svm_set_gpat(svm, data);
> -		else
> -			svm_set_hpat(svm, data);
> +
> +		svm_set_pat(vcpu, data, msr->host_initiated);
>  		break;
>  	case MSR_IA32_SPEC_CTRL:
>  		if (!msr->host_initiated &&
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Sean Christopherson 1 month, 1 week ago
On Tue, Feb 17, 2026, Yosry Ahmed wrote:
> > @@ -2838,16 +2876,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  		msr_info->data = svm->msr_decfg;
> >  		break;
> >  	case MSR_IA32_CR_PAT:
> > -		/*
> > -		 * When nested NPT is enabled, L2 has a separate PAT from
> > -		 * L1.  Guest accesses to IA32_PAT while running L2 target
> > -		 * L2's gPAT; host-initiated accesses always target L1's
> > -		 * hPAT for backward and forward KVM_GET_MSRS compatibility
> > -		 * with older kernels.
> > -		 */
> > -		WARN_ON_ONCE(msr_info->host_initiated && vcpu->wants_to_run);
> > -		if (!msr_info->host_initiated && is_guest_mode(vcpu) &&
> > -		    nested_npt_enabled(svm))
> > +		if (svm_is_access_to_gpat(vcpu, msr_info->host_initiated))
> >  			msr_info->data = svm->nested.save.g_pat;
> >  		else
> >  			msr_info->data = vcpu->arch.pat;
> 
> I'd go a step further here and add svm_get_pat(), then this just
> becomes:
> 
> 	msr_info->data = svm_get_pat(vcpu, msr_info->host_initiated);
> 
> It's more consistent with svm_set_msr(), and completely abstracts the L1
> vs. L2 PAT logic with the helpers.

Either way works for me.
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Yosry Ahmed 1 month, 2 weeks ago
On Fri, Feb 13, 2026 at 07:20:28AM -0800, Sean Christopherson wrote:
> Please trim your replies.  Scrolling through 100+ lines of quoted text to find
> the ~12 lines of context that actually matter is annoying.

Ack.
 
> Actually, calling svm_set_l2_pat() when !is_guest_mode() is wrong too, no?  E.g.
> shouldn't we end up with this?

legacy_gpat_semantics should only be set in guest mode, and it's cleared
in pre-run, so before we can exit guest mode IIUC. But having the guard
in place for both cases is probably simpler anyway.

> 
>   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);
> 	}
>   }
>
Re: [PATCH v4 4/8] KVM: x86: nSVM: Redirect IA32_PAT accesses to either hPAT or gPAT
Posted by Yosry Ahmed 1 month, 2 weeks ago
On Fri, Feb 13, 2026 at 03:43:12PM +0000, Yosry Ahmed wrote:
> On Fri, Feb 13, 2026 at 07:20:28AM -0800, Sean Christopherson wrote:
> > Please trim your replies.  Scrolling through 100+ lines of quoted text to find
> > the ~12 lines of context that actually matter is annoying.
> 
> Ack.
>  
> > Actually, calling svm_set_l2_pat() when !is_guest_mode() is wrong too, no?  E.g.
> > shouldn't we end up with this?
> 
> legacy_gpat_semantics should only be set in guest mode, and it's cleared
> in pre-run, so before we can exit guest mode IIUC.

Never mind that, Jim just mentioned the cases of back-to-back
KVM_SET_NESTED_STATE.