diff mbox series

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

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

Commit Message

Sean Christopherson May 3, 2023, 6:28 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 |  2 +-
 arch/x86/kvm/x86.c  | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Kai Huang May 3, 2023, 11:23 p.m. UTC | #1 
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;
Sean Christopherson May 3, 2023, 11:36 p.m. UTC | #2 
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.
Kai Huang May 3, 2023, 11:49 p.m. UTC | #3 
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.
Yan Zhao May 4, 2023, 9:02 a.m. UTC | #4 
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?
Sean Christopherson May 4, 2023, 3:36 p.m. UTC | #5 
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.
diff mbox series

Patch

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;