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.