[PATCH 0/5] KVM: x86: Clean up MSR PAT handling

[PATCH 0/5] KVM: x86: Clean up MSR PAT handling

[Sean Christopherson]

Clean up KVM's handling of MSR PAT.  The PAT is currently lumped in with
MTRRs, and while the PAT does affect memtypes, it's not an MTRR and is even
exempted from KVM's kludgy attempts to play nice with UC memory for guests
with passthrough devices that have non-coherent DMA.

Note, this includes two previously posted patches:

  https://lore.kernel.org/all/20230329081859.2571698-1-guoke@uniontech.com
  https://lore.kernel.org/all/20230331071929.102070-1-haiwenyao@uniontech.com

Ke Guo (1):
  KVM: SVM: Use kvm_pat_valid() directly instead of kvm_mtrr_valid()

Sean Christopherson (3):
  KVM: x86: Use MTRR macros to define possible MTRR MSR ranges
  KVM: x86: WARN if writes to PAT MSR are handled by common KVM code
  KVM: x86: Move PAT MSR handling out of mtrr.c

Wenyao Hai (1):
  KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

 arch/x86/kvm/mtrr.c    | 20 ++++++--------------
 arch/x86/kvm/svm/svm.c |  2 +-
 arch/x86/kvm/vmx/vmx.c |  8 +++-----
 arch/x86/kvm/x86.c     | 18 ++++++++++++++----
 4 files changed, 24 insertions(+), 24 deletions(-)


base-commit: 5c291b93e5d665380dbecc6944973583f9565ee5
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

[Sean Christopherson]

From: Wenyao Hai <haiwenyao@uniontech.com>

Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing
through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr().

Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by

	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:

in kvm_set_msr_common().

Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com>
[sean: massage changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 44fb619803b8..53e249109483 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		    get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
 			get_vmcs12(vcpu)->guest_ia32_pat = data;
 
-		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
+		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT)
 			vmcs_write64(GUEST_IA32_PAT, data);
-			vcpu->arch.pat = data;
-			break;
-		}
-		ret = kvm_set_msr_common(vcpu, msr_info);
+
+		vcpu->arch.pat = data;
 		break;
 	case MSR_IA32_MCG_EXT_CTL:
 		if ((!msr_info->host_initiated &&
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

[Huang, Kai]

On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> From: Wenyao Hai <haiwenyao@uniontech.com>
> 
> Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing
> through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr().

What's the value of doing so, besides saving a function of kvm_set_msr_common()?

PAT change shouldn't be something frequent so shouldn't in a performance
critical path.  Given the PAT logic on Intel and AMD are basically the same ,
isn't it better to do in kvm_set_msr_common()?

For instance, given mtrr code is also in common x86, if we ever want to add some
additional logic to, i.e. calculate effective memtype, isn't better to do handle
pat in common code too?

> 
> Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by
> 
> 	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
> 
> in kvm_set_msr_common().
> 
> Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com>
> [sean: massage changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 44fb619803b8..53e249109483 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		    get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
>  			get_vmcs12(vcpu)->guest_ia32_pat = data;
>  
> -		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> +		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT)
>  			vmcs_write64(GUEST_IA32_PAT, data);
> -			vcpu->arch.pat = data;
> -			break;
> -		}
> -		ret = kvm_set_msr_common(vcpu, msr_info);
> +
> +		vcpu->arch.pat = data;
>  		break;
>  	case MSR_IA32_MCG_EXT_CTL:
>  		if ((!msr_info->host_initiated &&

Re: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

[Sean Christopherson]

On Wed, May 03, 2023, Kai Huang wrote:
> On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> > From: Wenyao Hai <haiwenyao@uniontech.com>
> > 
> > Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing
> > through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr().
> 
> What's the value of doing so, besides saving a function of kvm_set_msr_common()?

To avoid complicating a very simple operation (writing vcpu->arch.pat), and to
align with SVM.

> PAT change shouldn't be something frequent so shouldn't in a performance
> critical path.  Given the PAT logic on Intel and AMD are basically the same ,
> isn't it better to do in kvm_set_msr_common()?

I could go either way on calling into kvm_set_msr_common().  I agree that
performance isn't a concern.  Hmm, and kvm_set_msr_common() still has a case
statement for MSR_IA32_CR_PAT, so handling the write fully in vendor code won't
impact the code generation for other MSRs.

Though I am leaning towards saying we should either handle loads and stores to
vcpu->arch.pat in common code _or_ vendor code, i.e. either teach VMX and SVM to
handle reads of PAT, or have their write paths call kvm_set_msr_common().  A mix
of both is definitely odd.

I don't have strong preference on which of those two we choose.  I dislike duplicating
logic across VMX and SVM, but on the other hands it's so little code.  I think
I'd vote for handling everything in vendor code, mostly because this gives the
appearance that the write can fail, which is silly and misleading.

		ret = kvm_set_msr_common(vcpu, msr_info);

> For instance, given mtrr code is also in common x86, if we ever want to add some
> additional logic to, i.e. calculate effective memtype, isn't better to do handle
> pat in common code too?

FWIW, I highly doubt we'll ever have code like that.  The truly effective memtype
calculations are too different between Intel and AMD, and doing anything useful
with the guest's effective memtype is likely a fool's errand.

> > Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by
> > 
> > 	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
> > 
> > in kvm_set_msr_common().
> > 
> > Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com>
> > [sean: massage changelog]
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 44fb619803b8..53e249109483 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  		    get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
> >  			get_vmcs12(vcpu)->guest_ia32_pat = data;
> >  
> > -		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> > +		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT)
> >  			vmcs_write64(GUEST_IA32_PAT, data);
> > -			vcpu->arch.pat = data;
> > -			break;
> > -		}
> > -		ret = kvm_set_msr_common(vcpu, msr_info);
> > +
> > +		vcpu->arch.pat = data;
> >  		break;
> >  	case MSR_IA32_MCG_EXT_CTL:
> >  		if ((!msr_info->host_initiated &&

Re: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

[Huang, Kai]

On Wed, 2023-05-03 at 16:25 -0700, Sean Christopherson wrote:
> On Wed, May 03, 2023, Kai Huang wrote:
> > On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> > > From: Wenyao Hai <haiwenyao@uniontech.com>
> > > 
> > > Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing
> > > through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr().
> > 
> > What's the value of doing so, besides saving a function of kvm_set_msr_common()?
> 
> To avoid complicating a very simple operation (writing vcpu->arch.pat), and to
> align with SVM.
> 
> > PAT change shouldn't be something frequent so shouldn't in a performance
> > critical path.  Given the PAT logic on Intel and AMD are basically the same ,
> > isn't it better to do in kvm_set_msr_common()?
> 
> I could go either way on calling into kvm_set_msr_common().  I agree that
> performance isn't a concern.  Hmm, and kvm_set_msr_common() still has a case
> statement for MSR_IA32_CR_PAT, so handling the write fully in vendor code won't
> impact the code generation for other MSRs.
> 
> Though I am leaning towards saying we should either handle loads and stores to
> vcpu->arch.pat in common code _or_ vendor code, i.e. either teach VMX and SVM to
> handle reads of PAT, or have their write paths call kvm_set_msr_common().  A mix
> of both is definitely odd.

Agreed.  Alternatively we can move SVM's setting vcpu->arch.pat to common code.

> 
> I don't have strong preference on which of those two we choose.  I dislike duplicating
> logic across VMX and SVM, but on the other hands it's so little code.  I think
> I'd vote for handling everything in vendor code, mostly because this gives the
> appearance that the write can fail, which is silly and misleading.
> 
> 		ret = kvm_set_msr_common(vcpu, msr_info);

No opinion either.  First glance is having 

	case MSR_IA32_CR_PAT:
		vcpu->arch.pat = data;

in kvm_set_msr_common() is clearer because it is symmetrical to the read path.

Anyway your decision :)

> 
> > For instance, given mtrr code is also in common x86, if we ever want to add some
> > additional logic to, i.e. calculate effective memtype, isn't better to do handle
> > pat in common code too?
> 
> FWIW, I highly doubt we'll ever have code like that.  The truly effective memtype
> calculations are too different between Intel and AMD, and doing anything useful
> with the guest's effective memtype is likely a fool's errand.

I thought the logic of getting effective memtype should be just the same between
Intel and AMD but it seems there's slight difference.  I agree with you it's
unlikely to have such code in common.

But looks setting vcpu->arch.pat in common code is more flexible.  Anyway no
opinion here.

> 
> > > Note, MSR_IA32_CR_PAT is 0x277, and is very subtly handled by
> > > 
> > > 	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
> > > 
> > > in kvm_set_msr_common().
> > > 
> > > Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com>
> > > [sean: massage changelog]
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > >  arch/x86/kvm/vmx/vmx.c | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 44fb619803b8..53e249109483 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -2294,12 +2294,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > >  		    get_vmcs12(vcpu)->vm_exit_controls & VM_EXIT_SAVE_IA32_PAT)
> > >  			get_vmcs12(vcpu)->guest_ia32_pat = data;
> > >  
> > > -		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> > > +		if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT)
> > >  			vmcs_write64(GUEST_IA32_PAT, data);
> > > -			vcpu->arch.pat = data;
> > > -			break;
> > > -		}
> > > -		ret = kvm_set_msr_common(vcpu, msr_info);
> > > +
> > > +		vcpu->arch.pat = data;
> > >  		break;
> > >  	case MSR_IA32_MCG_EXT_CTL:
> > >  		if ((!msr_info->host_initiated &&

Re: [PATCH 1/5] KVM: VMX: Open code writing vCPU's PAT in VMX's MSR handler

[Sean Christopherson]

On Wed, May 03, 2023, Kai Huang wrote:
> On Wed, 2023-05-03 at 16:25 -0700, Sean Christopherson wrote:
> > On Wed, May 03, 2023, Kai Huang wrote:
> > > On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> > > > From: Wenyao Hai <haiwenyao@uniontech.com>
> > > > 
> > > > Open code setting "vcpu->arch.pat" in vmx_set_msr() instead of bouncing
> > > > through kvm_set_msr_common() to get to the same code in kvm_mtrr_set_msr().
> > > 
> > > What's the value of doing so, besides saving a function of kvm_set_msr_common()?
> > 
> > To avoid complicating a very simple operation (writing vcpu->arch.pat), and to
> > align with SVM.
> > 
> > > PAT change shouldn't be something frequent so shouldn't in a performance
> > > critical path.  Given the PAT logic on Intel and AMD are basically the same ,
> > > isn't it better to do in kvm_set_msr_common()?
> > 
> > I could go either way on calling into kvm_set_msr_common().  I agree that
> > performance isn't a concern.  Hmm, and kvm_set_msr_common() still has a case
> > statement for MSR_IA32_CR_PAT, so handling the write fully in vendor code won't
> > impact the code generation for other MSRs.
> > 
> > Though I am leaning towards saying we should either handle loads and stores to
> > vcpu->arch.pat in common code _or_ vendor code, i.e. either teach VMX and SVM to
> > handle reads of PAT, or have their write paths call kvm_set_msr_common().  A mix
> > of both is definitely odd.
> 
> Agreed.  Alternatively we can move SVM's setting vcpu->arch.pat to common code.
> 
> > 
> > I don't have strong preference on which of those two we choose.  I dislike duplicating
> > logic across VMX and SVM, but on the other hands it's so little code.  I think
> > I'd vote for handling everything in vendor code, mostly because this gives the
> > appearance that the write can fail, which is silly and misleading.
> > 
> > 		ret = kvm_set_msr_common(vcpu, msr_info);
> 
> No opinion either.  First glance is having 
> 
> 	case MSR_IA32_CR_PAT:
> 		vcpu->arch.pat = data;
> 
> in kvm_set_msr_common() is clearer because it is symmetrical to the read path.
> 
> Anyway your decision :)

Duh, the obvious answer is to do 

	ret = kvm_set_msr_common(vcpu, msr_info);
	if (ret)
		break;

	<vendor code here>

That's an established pattern for other MSRs, and addresses my main concern of
not unwinding the VMCS updates in the should-be-impossible scenario of
kvm_set_msr_common() failing after the kvm_pat_valid() check.

Thanks Kai!

[PATCH 2/5] KVM: SVM: Use kvm_pat_valid() directly instead of kvm_mtrr_valid()

[Sean Christopherson]

From: Ke Guo <guoke@uniontech.com>

Use kvm_pat_valid() directly instead of bouncing through kvm_mtrr_valid().
The PAT is not an MTRR, and kvm_mtrr_valid() just redirects to
kvm_pat_valid(), i.e. for better or worse, KVM doesn't apply the "zap
SPTEs" logic to guest PAT changes when the VM has a passthrough device
with non-coherent DMA.

Signed-off-by: Ke Guo <guoke@uniontech.com>
[sean: massage changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index eb308c9994f9..db237ccdc957 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2935,7 +2935,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 
 		break;
 	case MSR_IA32_CR_PAT:
-		if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
+		if (!kvm_pat_valid(data))
 			return 1;
 		vcpu->arch.pat = data;
 		svm->vmcb01.ptr->save.g_pat = data;
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH 2/5] KVM: SVM: Use kvm_pat_valid() directly instead of kvm_mtrr_valid()

[Huang, Kai]

> for better or worse, KVM doesn't apply the "zap
> SPTEs" logic to guest PAT changes when the VM has a passthrough device
> with non-coherent DMA.

Is it a bug?

> 
> Signed-off-by: Ke Guo <guoke@uniontech.com>
> [sean: massage changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/svm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index eb308c9994f9..db237ccdc957 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2935,7 +2935,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  
>  		break;
>  	case MSR_IA32_CR_PAT:
> -		if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> +		if (!kvm_pat_valid(data))
>  			return 1;
>  		vcpu->arch.pat = data;
>  		svm->vmcb01.ptr->save.g_pat = data;

Anyway for this change,

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 2/5] KVM: SVM: Use kvm_pat_valid() directly instead of kvm_mtrr_valid()

[Sean Christopherson]

On Wed, May 03, 2023, Kai Huang wrote:
> > for better or worse, KVM doesn't apply the "zap
> > SPTEs" logic to guest PAT changes when the VM has a passthrough device
> > with non-coherent DMA.
> 
> Is it a bug?

No.  KVM's MTRR behavior is using a heuristic to try not to break the VM: if the
VM has non-coherent DMA, then honor UC mapping in the MTRRs as such mappings may
be coverage the non-coherent DMA.

From vmx_get_mt_mask():

	/* We wanted to honor guest CD/MTRR/PAT, but doing so could result in
	 * memory aliases with conflicting memory types and sometimes MCEs.
	 * We have to be careful as to what are honored and when.

The PAT is problematic because it is referenced via the guest PTEs, versus the
MTRRs being tied to the guest physical address, e.g. different virtual mappings
for the same physical address can yield different memtypes via the PAT.  My head
hurts just thinking about how that might interact with shadow paging :-)

Even the MTRRs are somewhat sketchy because they are technically per-CPU, i.e.
two vCPUs could have different memtypes for the same physical address.  But in
practice, sane software/firmware uses consistent MTRRs across all CPUs.

Re: [PATCH 2/5] KVM: SVM: Use kvm_pat_valid() directly instead of kvm_mtrr_valid()

[Huang, Kai]

On Thu, 2023-05-04 at 08:34 -0700, Sean Christopherson wrote:
> On Wed, May 03, 2023, Kai Huang wrote:
> > > for better or worse, KVM doesn't apply the "zap
> > > SPTEs" logic to guest PAT changes when the VM has a passthrough device
> > > with non-coherent DMA.
> > 
> > Is it a bug?
> 
> No.  KVM's MTRR behavior is using a heuristic to try not to break the VM: if the
> VM has non-coherent DMA, then honor UC mapping in the MTRRs as such mappings may
> be coverage the non-coherent DMA.
> 
> From vmx_get_mt_mask():
> 
> 	/* We wanted to honor guest CD/MTRR/PAT, but doing so could result in
> 	 * memory aliases with conflicting memory types and sometimes MCEs.
> 	 * We have to be careful as to what are honored and when.
> 
> The PAT is problematic because it is referenced via the guest PTEs, versus the
> MTRRs being tied to the guest physical address, e.g. different virtual mappings
> for the same physical address can yield different memtypes via the PAT.  My head
> hurts just thinking about how that might interact with shadow paging :-)
> 
> Even the MTRRs are somewhat sketchy because they are technically per-CPU, i.e.
> two vCPUs could have different memtypes for the same physical address.  But in
> practice, sane software/firmware uses consistent MTRRs across all CPUs.

Agreed on all above odds.

But I think the answer to my question is actually we simply don't _need_ to zap
SPTEs (with non-coherent DMA) when guest's IA32_PAT is changed:

1) If EPT is enabled, IIUC guest's PAT is already horned.  VMCS's GUEST_IA32_PAT
always reflects the IA32_PAT that guest wants to set.  EPT's memtype bits are
set according to guest's MTRR.  That means guest changing IA32_PAT doesn't need
to zap EPT PTEs as "EPT PTEs essentially only replaces guest's MTRRs".

2) If EPT is disabled, looking at the code, if I read correctly, the
'shadow_memtype_mask' is 0 for Intel, in which case KVM won't try to set any PAT
memtype bit in shadow MMU PTE, which means the true PAT memtype is always WB and
guest's memtype is never horned (guest's MTRRs are also never actually used by
HW), which should be fine I guess??  My brain refused to go further :)

But anyway back to my question, I think "changing guest's IA32_PAT" shouldn't
result in needing to "zap SPTEs".

Re: [PATCH 2/5] KVM: SVM: Use kvm_pat_valid() directly instead of kvm_mtrr_valid()

[Sean Christopherson]

On Fri, May 05, 2023, Kai Huang wrote:
> On Thu, 2023-05-04 at 08:34 -0700, Sean Christopherson wrote:
> > On Wed, May 03, 2023, Kai Huang wrote:
> > > > for better or worse, KVM doesn't apply the "zap
> > > > SPTEs" logic to guest PAT changes when the VM has a passthrough device
> > > > with non-coherent DMA.
> > > 
> > > Is it a bug?
> > 
> > No.  KVM's MTRR behavior is using a heuristic to try not to break the VM: if the
> > VM has non-coherent DMA, then honor UC mapping in the MTRRs as such mappings may
> > be coverage the non-coherent DMA.
> > 
> > From vmx_get_mt_mask():
> > 
> > 	/* We wanted to honor guest CD/MTRR/PAT, but doing so could result in
> > 	 * memory aliases with conflicting memory types and sometimes MCEs.
> > 	 * We have to be careful as to what are honored and when.
> > 
> > The PAT is problematic because it is referenced via the guest PTEs, versus the
> > MTRRs being tied to the guest physical address, e.g. different virtual mappings
> > for the same physical address can yield different memtypes via the PAT.  My head
> > hurts just thinking about how that might interact with shadow paging :-)
> > 
> > Even the MTRRs are somewhat sketchy because they are technically per-CPU, i.e.
> > two vCPUs could have different memtypes for the same physical address.  But in
> > practice, sane software/firmware uses consistent MTRRs across all CPUs.
> 
> Agreed on all above odds.
> 
> But I think the answer to my question is actually we simply don't _need_ to zap
> SPTEs (with non-coherent DMA) when guest's IA32_PAT is changed:
> 
> 1) If EPT is enabled, IIUC guest's PAT is already horned.  VMCS's GUEST_IA32_PAT
> always reflects the IA32_PAT that guest wants to set.  EPT's memtype bits are
> set according to guest's MTRR.  That means guest changing IA32_PAT doesn't need
> to zap EPT PTEs as "EPT PTEs essentially only replaces guest's MTRRs".

Ah, yes, you're correct.  I thought KVM _always_ set the "ignore guest PAT" bit
in the EPT PTEs, but KVM honors guest PAT when non-coherent DMA is present and
CR0.CD=0.

> 2) If EPT is disabled, looking at the code, if I read correctly, the
> 'shadow_memtype_mask' is 0 for Intel, in which case KVM won't try to set any PAT
> memtype bit in shadow MMU PTE, which means the true PAT memtype is always WB and
> guest's memtype is never horned (guest's MTRRs are also never actually used by
> HW), which should be fine I guess??  My brain refused to go further :)

Yep.  It's entirely possible that VT-d without snoop control simply doesn't work
with shadow paging, but no one has ever cared.

> But anyway back to my question, I think "changing guest's IA32_PAT" shouldn't
> result in needing to "zap SPTEs".

[PATCH 3/5] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

[Sean Christopherson]

Use the MTRR macros to identify the ranges of possible MTRR MSRs instead
of bounding the ranges with a mismash of open coded values and unrelated
MSR indices.  Carving out the gap for the machine check MSRs in particular
is confusing, as it's easy to incorrectly think the case statement handles
MCE MSRs instead of skipping them.

Drop the range-based funneling of MSRs between the end of the MCE MSRs
and MTRR_DEF_TYPE, i.e. 0x2A0-0x2FF, and instead handle MTTR_DEF_TYPE as
the one-off case that it is.

Extract PAT (0x277) as well in anticipation of dropping PAT "handling"
from the MTRR code.

Keep the range-based handling for the variable+fixed MTRRs even though
capturing unknown MSRs 0x214-0x24F is arguably "wrong".  There is a gap in
the fixed MTRRs, 0x260-0x267, i.e. the MTRR code needs to filter out
unknown MSRs anyways, and using a single range generates marginally better
code for the big switch statement.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mtrr.c |  2 +-
 arch/x86/kvm/x86.c  | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
index 9fac1ec03463..d2c428f4ae42 100644
--- a/arch/x86/kvm/mtrr.c
+++ b/arch/x86/kvm/mtrr.c
@@ -28,7 +28,7 @@
 static bool msr_mtrr_valid(unsigned msr)
 {
 	switch (msr) {
-	case 0x200 ... 0x200 + 2 * KVM_NR_VAR_MTRR - 1:
+	case MTRRphysBase_MSR(0) ... MTRRphysMask_MSR(KVM_NR_VAR_MTRR - 1):
 	case MSR_MTRRfix64K_00000:
 	case MSR_MTRRfix16K_80000:
 	case MSR_MTRRfix16K_A0000:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e7f78fe79b32..8b356c9d8a81 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3700,8 +3700,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return 1;
 		}
 		break;
-	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
-	case MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) ... 0x2ff:
+	case MSR_IA32_CR_PAT:
+	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
+	case MSR_MTRRdefType:
 		return kvm_mtrr_set_msr(vcpu, msr, data);
 	case MSR_IA32_APICBASE:
 		return kvm_set_apic_base(vcpu, msr_info);
@@ -4108,9 +4109,10 @@ 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:
 	case MSR_MTRRcap:
-	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
-	case MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) ... 0x2ff:
+	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
+	case MSR_MTRRdefType:
 		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
 	case 0xcd: /* fsb frequency */
 		msr_info->data = 3;
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH 3/5] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

[Huang, Kai]

On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> Use the MTRR macros to identify the ranges of possible MTRR MSRs instead
> of bounding the ranges with a mismash of open coded values and unrelated
> MSR indices.  Carving out the gap for the machine check MSRs in particular
> is confusing, as it's easy to incorrectly think the case statement handles
> MCE MSRs instead of skipping them.
> 
> Drop the range-based funneling of MSRs between the end of the MCE MSRs
> and MTRR_DEF_TYPE, i.e. 0x2A0-0x2FF, and instead handle MTTR_DEF_TYPE as
> the one-off case that it is.
> 
> Extract PAT (0x277) as well in anticipation of dropping PAT "handling"
> from the MTRR code.
> 
> Keep the range-based handling for the variable+fixed MTRRs even though
> capturing unknown MSRs 0x214-0x24F is arguably "wrong".  There is a gap in
> the fixed MTRRs, 0x260-0x267, i.e. the MTRR code needs to filter out
> unknown MSRs anyways, 
> 

Looks a little bit half measure, but ...

> and using a single range generates marginally better
> code for the big switch statement.

could you educate why because I am ignorant of compiler behaviour? :)

> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/mtrr.c |  2 +-
>  arch/x86/kvm/x86.c  | 10 ++++++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> index 9fac1ec03463..d2c428f4ae42 100644
> --- a/arch/x86/kvm/mtrr.c
> +++ b/arch/x86/kvm/mtrr.c
> @@ -28,7 +28,7 @@
>  static bool msr_mtrr_valid(unsigned msr)
>  {
>  	switch (msr) {
> -	case 0x200 ... 0x200 + 2 * KVM_NR_VAR_MTRR - 1:
> +	case MTRRphysBase_MSR(0) ... MTRRphysMask_MSR(KVM_NR_VAR_MTRR - 1):
>  	case MSR_MTRRfix64K_00000:
>  	case MSR_MTRRfix16K_80000:
>  	case MSR_MTRRfix16K_A0000:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e7f78fe79b32..8b356c9d8a81 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3700,8 +3700,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			return 1;
>  		}
>  		break;
> -	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
> -	case MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) ... 0x2ff:
> +	case MSR_IA32_CR_PAT:
> +	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
> +	case MSR_MTRRdefType:
>  		return kvm_mtrr_set_msr(vcpu, msr, data);
>  	case MSR_IA32_APICBASE:
>  		return kvm_set_apic_base(vcpu, msr_info);
> @@ -4108,9 +4109,10 @@ 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:
>  	case MSR_MTRRcap:
> -	case 0x200 ... MSR_IA32_MC0_CTL2 - 1:
> -	case MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) ... 0x2ff:
> +	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
> +	case MSR_MTRRdefType:
>  		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
>  	case 0xcd: /* fsb frequency */
>  		msr_info->data = 3;

Re: [PATCH 3/5] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

[Sean Christopherson]

On Wed, May 03, 2023, Huang, Kai wrote:
> On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> > Use the MTRR macros to identify the ranges of possible MTRR MSRs instead
> > of bounding the ranges with a mismash of open coded values and unrelated
> > MSR indices.  Carving out the gap for the machine check MSRs in particular
> > is confusing, as it's easy to incorrectly think the case statement handles
> > MCE MSRs instead of skipping them.
> > 
> > Drop the range-based funneling of MSRs between the end of the MCE MSRs
> > and MTRR_DEF_TYPE, i.e. 0x2A0-0x2FF, and instead handle MTTR_DEF_TYPE as
> > the one-off case that it is.
> > 
> > Extract PAT (0x277) as well in anticipation of dropping PAT "handling"
> > from the MTRR code.
> > 
> > Keep the range-based handling for the variable+fixed MTRRs even though
> > capturing unknown MSRs 0x214-0x24F is arguably "wrong".  There is a gap in
> > the fixed MTRRs, 0x260-0x267, i.e. the MTRR code needs to filter out
> > unknown MSRs anyways,�
> > 
> 
> Looks a little bit half measure, but ...

Yeah, I don't love it, but I couldn't convince myself that precisely identifying
known MTRRs was worth the extra effort.

> > and using a single range generates marginally better
> > code for the big switch statement.
> 
> could you educate why because I am ignorant of compiler behaviour? :)

Capturing the entire range instead of filtering out the gaps allows the compiler
to handle multiple MSRs with fewer CMP+Jcc checks.

E.g. think of it like this (I actually missed a gap)

	if (msr >= 0x200 && msr <= 0x26f)

versus

	if ((msr >= 0x200 && msr <= 0x213) || (msr == 0x250) || (msr == 0x258) ||
	    (msr == 0x259) || (msr >= 0x268 && msr <= 0x26f))

Nothing enormous, and it's not like non-fastpath WRMSR is performance critical,
but add in the extra code for precisely capturing the MTRRs in both x86.c _and_
mtrr.c, and IMO it's worth being imprecise.

Re: [PATCH 3/5] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

[Huang, Kai]

On Wed, 2023-05-03 at 23:36 +0000, Sean Christopherson wrote:
> On Wed, May 03, 2023, Huang, Kai wrote:
> > On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> > > Use the MTRR macros to identify the ranges of possible MTRR MSRs instead
> > > of bounding the ranges with a mismash of open coded values and unrelated
> > > MSR indices.  Carving out the gap for the machine check MSRs in particular
> > > is confusing, as it's easy to incorrectly think the case statement handles
> > > MCE MSRs instead of skipping them.
> > > 
> > > Drop the range-based funneling of MSRs between the end of the MCE MSRs
> > > and MTRR_DEF_TYPE, i.e. 0x2A0-0x2FF, and instead handle MTTR_DEF_TYPE as
> > > the one-off case that it is.
> > > 
> > > Extract PAT (0x277) as well in anticipation of dropping PAT "handling"
> > > from the MTRR code.
> > > 
> > > Keep the range-based handling for the variable+fixed MTRRs even though
> > > capturing unknown MSRs 0x214-0x24F is arguably "wrong".  There is a gap in
> > > the fixed MTRRs, 0x260-0x267, i.e. the MTRR code needs to filter out
> > > unknown MSRs anyways,�
> > > 
> > 
> > Looks a little bit half measure, but ...
> 
> Yeah, I don't love it, but I couldn't convince myself that precisely identifying
> known MTRRs was worth the extra effort.
> 
> > > and using a single range generates marginally better
> > > code for the big switch statement.
> > 
> > could you educate why because I am ignorant of compiler behaviour? :)
> 
> Capturing the entire range instead of filtering out the gaps allows the compiler
> to handle multiple MSRs with fewer CMP+Jcc checks.
> 
> E.g. think of it like this (I actually missed a gap)
> 
> 	if (msr >= 0x200 && msr <= 0x26f)
> 
> versus
> 
> 	if ((msr >= 0x200 && msr <= 0x213) || (msr == 0x250) || (msr == 0x258) ||
> 	    (msr == 0x259) || (msr >= 0x268 && msr <= 0x26f))

I see.  Thanks.

> 
> Nothing enormous, and it's not like non-fastpath WRMSR is performance critical,
> but add in the extra code for precisely capturing the MTRRs in both x86.c _and_
> mtrr.c, and IMO it's worth being imprecise.

Right.  We don't need to catch all holes unless we effectively remove
msr_mtrr_valid() and catch all holes in x86.c I guess.  But seems separating
fixed range vs variable ranges seems more clear in the code.

But as you said we cannot justify having duplicated check in x86.c and mtrr.c,
so no opinion here.

Re: [PATCH 3/5] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

[Yan Zhao]

On Wed, May 03, 2023 at 11:28:50AM -0700, Sean Christopherson wrote:
> Use the MTRR macros to identify the ranges of possible MTRR MSRs instead

What about using MTRR macros to replace other 0x200 in mtrr.c too?

Re: [PATCH 3/5] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

[Sean Christopherson]

On Thu, May 04, 2023, Yan Zhao wrote:
> On Wed, May 03, 2023 at 11:28:50AM -0700, Sean Christopherson wrote:
> > Use the MTRR macros to identify the ranges of possible MTRR MSRs instead
> 
> What about using MTRR macros to replace other 0x200 in mtrr.c too?

Ugh, yes, I 'll convert all of those in v2 (wow, there are a lot of them).

Ooh, and I missed that once the SVM usage of kvm_mtrr_valid() goes away, that thing
can be made a static function.

There's quite a bit of cleanup that can be done in the mtrr.c code, I'll see if
there's any other low hanging fruit that can be picked for this series.

[PATCH 4/5] KVM: x86: WARN if writes to PAT MSR are handled by common KVM code

[Sean Christopherson]

WARN and continue if a write to the PAT MSR reaches kvm_set_msr_common()
now that both VMX and SVM handle PAT writes entirely on their own.  Keep
the case statement with a WARN instead of dropping it entirely to document
why KVM's handling of reads and writes isn't symmetrical (reads are still
handled by kvm_get_msr_common().

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8b356c9d8a81..c36256d00250 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3701,6 +3701,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		}
 		break;
 	case MSR_IA32_CR_PAT:
+		/*
+		 * Writes to PAT should be handled by vendor code as both SVM
+		 * and VMX track the guest's PAT in the VMCB/VMCS.
+		 */
+		WARN_ON_ONCE(1);
+		fallthrough;
 	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
 	case MSR_MTRRdefType:
 		return kvm_mtrr_set_msr(vcpu, msr, data);
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH 4/5] KVM: x86: WARN if writes to PAT MSR are handled by common KVM code

[Huang, Kai]

On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> WARN and continue if a write to the PAT MSR reaches kvm_set_msr_common()
> now that both VMX and SVM handle PAT writes entirely on their own.  Keep
> the case statement with a WARN instead of dropping it entirely to document
> why KVM's handling of reads and writes isn't symmetrical (reads are still
> handled by kvm_get_msr_common().

Why not just merge this patch with the next one? 

> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8b356c9d8a81..c36256d00250 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3701,6 +3701,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		}
>  		break;
>  	case MSR_IA32_CR_PAT:
> +		/*
> +		 * Writes to PAT should be handled by vendor code as both SVM
> +		 * and VMX track the guest's PAT in the VMCB/VMCS.
> +		 */
> +		WARN_ON_ONCE(1);
> +		fallthrough;
>  	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
>  	case MSR_MTRRdefType:
>  		return kvm_mtrr_set_msr(vcpu, msr, data);

Re: [PATCH 4/5] KVM: x86: WARN if writes to PAT MSR are handled by common KVM code

[Sean Christopherson]

On Wed, May 03, 2023, Kai Huang wrote:
> On Wed, 2023-05-03 at 11:28 -0700, Sean Christopherson wrote:
> > WARN and continue if a write to the PAT MSR reaches kvm_set_msr_common()
> > now that both VMX and SVM handle PAT writes entirely on their own.  Keep
> > the case statement with a WARN instead of dropping it entirely to document
> > why KVM's handling of reads and writes isn't symmetrical (reads are still
> > handled by kvm_get_msr_common().
> 
> Why not just merge this patch with the next one?

Hmm, good question.  IIRC, I originally had the last patch delete the case
statement and so wanted a bisection point, but I agree that having this as a
standalone patch is silly.  I'll squash it with patch 5 in v2.

Thanks!

[PATCH 5/5] KVM: x86: Move PAT MSR handling out of mtrr.c

[Sean Christopherson]

Drop handling of MSR_IA32_CR_PAT from mtrr.c now that SVM and VMX handle
writes without bouncing through kvm_set_msr_common().  PAT isn't truly an
MTRR even though it affects memory types, and more importantly isn't
factored into KVM's handling of non-coherent DMA.

The read path is and always has been trivial, i.e. burying it in the MTRR
code does more harm than good.

Inject #GP unconditionally for the PAT "handlng" in kvm_set_msr_common(),
as that code is reached if and only if KVM is buggy.  Simply deleting the
condition would provide similar functionality, but warning and explicitly
alerting userspace that KVM is buggy is desirable.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mtrr.c | 18 +++++-------------
 arch/x86/kvm/x86.c  |  4 +++-
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
index d2c428f4ae42..76b132ba9df2 100644
--- a/arch/x86/kvm/mtrr.c
+++ b/arch/x86/kvm/mtrr.c
@@ -41,7 +41,6 @@ static bool msr_mtrr_valid(unsigned msr)
 	case MSR_MTRRfix4K_F0000:
 	case MSR_MTRRfix4K_F8000:
 	case MSR_MTRRdefType:
-	case MSR_IA32_CR_PAT:
 		return true;
 	}
 	return false;
@@ -60,9 +59,7 @@ bool kvm_mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	if (!msr_mtrr_valid(msr))
 		return false;
 
-	if (msr == MSR_IA32_CR_PAT) {
-		return kvm_pat_valid(data);
-	} else if (msr == MSR_MTRRdefType) {
+	if (msr == MSR_MTRRdefType) {
 		if (data & ~0xcff)
 			return false;
 		return valid_mtrr_type(data & 0xff);
@@ -310,8 +307,7 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr)
 	gfn_t start, end;
 	int index;
 
-	if (msr == MSR_IA32_CR_PAT || !tdp_enabled ||
-	      !kvm_arch_has_noncoherent_dma(vcpu->kvm))
+	if (!tdp_enabled || !kvm_arch_has_noncoherent_dma(vcpu->kvm))
 		return;
 
 	if (!mtrr_is_enabled(mtrr_state) && msr != MSR_MTRRdefType)
@@ -382,8 +378,6 @@ int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 		*(u64 *)&vcpu->arch.mtrr_state.fixed_ranges[index] = data;
 	else if (msr == MSR_MTRRdefType)
 		vcpu->arch.mtrr_state.deftype = data;
-	else if (msr == MSR_IA32_CR_PAT)
-		vcpu->arch.pat = data;
 	else
 		set_var_mtrr_msr(vcpu, msr, data);
 
@@ -411,13 +405,11 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 		return 1;
 
 	index = fixed_msr_to_range_index(msr);
-	if (index >= 0)
+	if (index >= 0) {
 		*pdata = *(u64 *)&vcpu->arch.mtrr_state.fixed_ranges[index];
-	else if (msr == MSR_MTRRdefType)
+	} else if (msr == MSR_MTRRdefType) {
 		*pdata = vcpu->arch.mtrr_state.deftype;
-	else if (msr == MSR_IA32_CR_PAT)
-		*pdata = vcpu->arch.pat;
-	else {	/* Variable MTRRs */
+	} else {	/* Variable MTRRs */
 		int is_mtrr_mask;
 
 		index = (msr - 0x200) / 2;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c36256d00250..3aa93401a398 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3706,7 +3706,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		 * and VMX track the guest's PAT in the VMCB/VMCS.
 		 */
 		WARN_ON_ONCE(1);
-		fallthrough;
+		return 1;
 	case MTRRphysBase_MSR(0) ... MSR_MTRRfix4K_F8000:
 	case MSR_MTRRdefType:
 		return kvm_mtrr_set_msr(vcpu, msr, data);
@@ -4116,6 +4116,8 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		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:
-- 
2.40.1.495.gc816e09b53d-goog