diff mbox series

[v2,5/8] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

Message ID 20230511233351.635053-6-seanjc@google.com
State New
Headers
Series KVM: x86: Clean up MSR PAT handling |

Commit Message

Sean Christopherson May 11, 2023, 11:33 p.m. UTC 
  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 |  7 ++++---
 arch/x86/kvm/x86.c  | 10 ++++++----
 2 files changed, 10 insertions(+), 7 deletions(-)

Comments

Kai Huang May 12, 2023, 10:35 a.m. UTC | #1 
On Thu, 2023-05-11 at 16:33 -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
				^
				mishmash?

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

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

One Nit below ...

> ---
>  arch/x86/kvm/mtrr.c |  7 ++++---
>  arch/x86/kvm/x86.c  | 10 ++++++----
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
> index 59851dbebfea..dc213b940141 100644
> --- a/arch/x86/kvm/mtrr.c
> +++ b/arch/x86/kvm/mtrr.c
> @@ -34,7 +34,7 @@ static bool is_mtrr_base_msr(unsigned int msr)
>  static struct kvm_mtrr_range *var_mtrr_msr_to_range(struct kvm_vcpu *vcpu,
>  						    unsigned int msr)
>  {
> -	int index = (msr - 0x200) / 2;
> +	int index = (msr - MTRRphysBase_MSR(0)) / 2;
>  
>  	return &vcpu->arch.mtrr_state.var_ranges[index];
>  }
> @@ -42,7 +42,7 @@ static struct kvm_mtrr_range *var_mtrr_msr_to_range(struct kvm_vcpu *vcpu,
>  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:
> @@ -88,7 +88,8 @@ bool kvm_mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  	}
>  
>  	/* variable MTRRs */
> -	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
> +	WARN_ON(!(msr >= MTRRphysBase_MSR(0) &&
> +		  msr <= MTRRphysMask_MSR(KVM_NR_VAR_MTRR - 1)));
>  
>  	mask = kvm_vcpu_reserved_gpa_bits_raw(vcpu);
>  	if ((msr & 1) == 0) {
> 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:

... Should we put MSR_IA32_CR_PAT after MSR_MTRRcap so it can be symmetric to
kvm_set_msr_common()?

Looks there's no reason to put it before 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.606.ga4b1b128d6-goog
>
Sean Christopherson May 12, 2023, 4:35 p.m. UTC | #2 
On Fri, May 12, 2023, Kai Huang wrote:
> On Thu, 2023-05-11 at 16:33 -0700, Sean Christopherson wrote:
> > 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:
> 
> ... Should we put MSR_IA32_CR_PAT after MSR_MTRRcap so it can be symmetric to
> kvm_set_msr_common()?
> 
> Looks there's no reason to put it before MSR_MTRRcap.

No, it's above MTRRcap for two reasons:

 1. When PAT is moved out of mtrr.c, PAT doesn't get bunded with the other MTRRs
    and so would just need to be hoisted back up.  The end code looks like:

	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:
		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
 
 2. There is no MSR_MTRRcap case statement in kvm_set_msr_common() because it's
    a read-only MSR, i.e. the two can't be symmetric :-)

> > -	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.606.ga4b1b128d6-goog
> > 
>
Kai Huang May 15, 2023, 12:37 a.m. UTC | #3 
On Fri, 2023-05-12 at 09:35 -0700, Sean Christopherson wrote:
> On Fri, May 12, 2023, Kai Huang wrote:
> > On Thu, 2023-05-11 at 16:33 -0700, Sean Christopherson wrote:
> > > 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:
> > 
> > ... Should we put MSR_IA32_CR_PAT after MSR_MTRRcap so it can be symmetric to
> > kvm_set_msr_common()?
> > 
> > Looks there's no reason to put it before MSR_MTRRcap.
> 
> No, it's above MTRRcap for two reasons:
> 
>  1. When PAT is moved out of mtrr.c, PAT doesn't get bunded with the other MTRRs
>     and so would just need to be hoisted back up.  The end code looks like:
> 
> 	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:
> 		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);

Sorry I mistakenly thought MSR_MTRRcap wasn't handled in kvm_mtrr_get_msr(). 
Yes looks good to me.

>  
>  2. There is no MSR_MTRRcap case statement in kvm_set_msr_common() because it's
>     a read-only MSR, i.e. the two can't be symmetric :-)

Do you know why it is a read-only MSR, which enables both FIXED ranges and
(fixed number of) dynamic ranges?

I am asking because there's a x86 series to fake a simple synthetic MTRR which
neither has fixed nor dynamic ranges but only has a default MSR_MTRRdefType:

https://lore.kernel.org/lkml/20230509233641.GGZFrZCTDH7VwUMp5R@fat_crate.local/T/

The main use cases are Xen PV guests and SEV-SNP guests running under
Hyper-V, but it appears TDX guest is also desired to have similar handling,
because: 

1) TDX module exposes MTRR in CPUID to guest, but handles nothing about MTRR
MSRs but only injects #VE.

2) TDX module always maps guest private memory as WB (and ignores guest's PAT
IIUC);

3) For shared memory, w/o non-coherent DMA guest's MTRRs are ignored by KVM
anyway.  TDX doesn't officially support non-trusted device assignment AFAICT.
Even we want to consider non-coherent DMA, it would only add confusion to honor
guest's MTRRs since they can point to private memory, which is always mapped as
WB.

So basically looks there's no value to exposing FIXED and dynamic MTRR ranges to
TDX guest.
Sean Christopherson May 15, 2023, 5:49 p.m. UTC | #4 
On Mon, May 15, 2023, Kai Huang wrote:
> On Fri, 2023-05-12 at 09:35 -0700, Sean Christopherson wrote:
> > On Fri, May 12, 2023, Kai Huang wrote:
> > > On Thu, 2023-05-11 at 16:33 -0700, Sean Christopherson wrote:
> > > > 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:
> > > 
> > > ... Should we put MSR_IA32_CR_PAT after MSR_MTRRcap so it can be symmetric to
> > > kvm_set_msr_common()?
> > > 
> > > Looks there's no reason to put it before MSR_MTRRcap.
> > 
> > No, it's above MTRRcap for two reasons:
> > 
> >  1. When PAT is moved out of mtrr.c, PAT doesn't get bunded with the other MTRRs
> >     and so would just need to be hoisted back up.  The end code looks like:
> > 
> > 	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:
> > 		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
> 
> Sorry I mistakenly thought MSR_MTRRcap wasn't handled in kvm_mtrr_get_msr(). 
> Yes looks good to me.
> 
> >  
> >  2. There is no MSR_MTRRcap case statement in kvm_set_msr_common() because it's
> >     a read-only MSR, i.e. the two can't be symmetric :-)
> 
> Do you know why it is a read-only MSR, which enables both FIXED ranges and
> (fixed number of) dynamic ranges?

MTTRcap doesn't "enable" anything, it's a capabilities MSR (MTRR Capability is
its given name in the SDM), similar to ARCH_CAPABILITIES, PERF_CAPABILITIES, etc.
They're all essentially CPUID leafs, but presumably are MSRs due to being relevant
only to CPL0.  Or maybe some higher ups at Intel just spin a wheel to determine
whether to use a CPUID leaf or an MSR.  :-)

> I am asking because there's a x86 series to fake a simple synthetic MTRR which
> neither has fixed nor dynamic ranges but only has a default MSR_MTRRdefType:
> 
> https://lore.kernel.org/lkml/20230509233641.GGZFrZCTDH7VwUMp5R@fat_crate.local/T/
> 
> The main use cases are Xen PV guests and SEV-SNP guests running under
> Hyper-V, but it appears TDX guest is also desired to have similar handling,
> because:�
> 
> 1) TDX module exposes MTRR in CPUID to guest, but handles nothing about MTRR
> MSRs but only injects #VE.
> 
> 2) TDX module always maps guest private memory as WB (and ignores guest's PAT
> IIUC);
> 
> 3) For shared memory, w/o non-coherent DMA guest's MTRRs are ignored by KVM
> anyway.  TDX doesn't officially support non-trusted device assignment AFAICT.
> Even we want to consider non-coherent DMA, it would only add confusion to honor
> guest's MTRRs since they can point to private memory, which is always mapped as
> WB.

Yeah, I think the best option is for KVM to disallow attaching non-coherent DMA
to TDX VMs.  AFAIK, there's no use case for such a setup.

> So basically looks there's no value to exposing FIXED and dynamic MTRR ranges to
> TDX guest.

Agreed.
Kai Huang May 15, 2023, 10:21 p.m. UTC | #5 
On Mon, 2023-05-15 at 10:49 -0700, Sean Christopherson wrote:
> On Mon, May 15, 2023, Kai Huang wrote:
> > On Fri, 2023-05-12 at 09:35 -0700, Sean Christopherson wrote:
> > > On Fri, May 12, 2023, Kai Huang wrote:
> > > > On Thu, 2023-05-11 at 16:33 -0700, Sean Christopherson wrote:
> > > > > 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:
> > > > 
> > > > ... Should we put MSR_IA32_CR_PAT after MSR_MTRRcap so it can be symmetric to
> > > > kvm_set_msr_common()?
> > > > 
> > > > Looks there's no reason to put it before MSR_MTRRcap.
> > > 
> > > No, it's above MTRRcap for two reasons:
> > > 
> > >  1. When PAT is moved out of mtrr.c, PAT doesn't get bunded with the other MTRRs
> > >     and so would just need to be hoisted back up.  The end code looks like:
> > > 
> > > 	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:
> > > 		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
> > 
> > Sorry I mistakenly thought MSR_MTRRcap wasn't handled in kvm_mtrr_get_msr(). 
> > Yes looks good to me.
> > 
> > >  
> > >  2. There is no MSR_MTRRcap case statement in kvm_set_msr_common() because it's
> > >     a read-only MSR, i.e. the two can't be symmetric :-)
> > 
> > Do you know why it is a read-only MSR, which enables both FIXED ranges and
> > (fixed number of) dynamic ranges?
> 
> MTTRcap doesn't "enable" anything, it's a capabilities MSR (MTRR Capability is
> its given name in the SDM), similar to ARCH_CAPABILITIES, PERF_CAPABILITIES, etc.
> They're all essentially CPUID leafs, but presumably are MSRs due to being relevant
> only to CPL0.  Or maybe some higher ups at Intel just spin a wheel to determine
> whether to use a CPUID leaf or an MSR.  :-)

I meant it may make sense to allow Qemu to configure it.  Anyway thanks!

> 
> > I am asking because there's a x86 series to fake a simple synthetic MTRR which
> > neither has fixed nor dynamic ranges but only has a default MSR_MTRRdefType:
> > 
> > https://lore.kernel.org/lkml/20230509233641.GGZFrZCTDH7VwUMp5R@fat_crate.local/T/
> > 
> > The main use cases are Xen PV guests and SEV-SNP guests running under
> > Hyper-V, but it appears TDX guest is also desired to have similar handling,
> > because:�
> > 
> > 1) TDX module exposes MTRR in CPUID to guest, but handles nothing about MTRR
> > MSRs but only injects #VE.
> > 
> > 2) TDX module always maps guest private memory as WB (and ignores guest's PAT
> > IIUC);
> > 
> > 3) For shared memory, w/o non-coherent DMA guest's MTRRs are ignored by KVM
> > anyway.  TDX doesn't officially support non-trusted device assignment AFAICT.
> > Even we want to consider non-coherent DMA, it would only add confusion to honor
> > guest's MTRRs since they can point to private memory, which is always mapped as
> > WB.
> 
> Yeah, I think the best option is for KVM to disallow attaching non-coherent DMA
> to TDX VMs.  AFAIK, there's no use case for such a setup.

+Isaku,

Presumably the only case is assigning GPU to TDX VMs, but again not sure we need
to consider this as AFAICT TDX *officially* doesn't support untrusted device
passthrough.  Isaku may have more information.

And Iskau,

Do you have any comments here, especially considering TDX 2.0 support for TEE-
IO?  We probably need a solution that is future extensible.

> 
> > So basically looks there's no value to exposing FIXED and dynamic MTRR ranges to
> > TDX guest.
> 
> Agreed.
diff mbox series

Patch

diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c
index 59851dbebfea..dc213b940141 100644
--- a/arch/x86/kvm/mtrr.c
+++ b/arch/x86/kvm/mtrr.c
@@ -34,7 +34,7 @@  static bool is_mtrr_base_msr(unsigned int msr)
 static struct kvm_mtrr_range *var_mtrr_msr_to_range(struct kvm_vcpu *vcpu,
 						    unsigned int msr)
 {
-	int index = (msr - 0x200) / 2;
+	int index = (msr - MTRRphysBase_MSR(0)) / 2;
 
 	return &vcpu->arch.mtrr_state.var_ranges[index];
 }
@@ -42,7 +42,7 @@  static struct kvm_mtrr_range *var_mtrr_msr_to_range(struct kvm_vcpu *vcpu,
 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:
@@ -88,7 +88,8 @@  bool kvm_mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	}
 
 	/* variable MTRRs */
-	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
+	WARN_ON(!(msr >= MTRRphysBase_MSR(0) &&
+		  msr <= MTRRphysMask_MSR(KVM_NR_VAR_MTRR - 1)));
 
 	mask = kvm_vcpu_reserved_gpa_bits_raw(vcpu);
 	if ((msr & 1) == 0) {
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;