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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZFLvytrl+Lj+mC4L@google.com>
Date:   Wed, 3 May 2023 23:36:25 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Kai Huang <kai.huang@...el.com>
Cc:     "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "guoke@...ontech.com" <guoke@...ontech.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "haiwenyao@...ontech.com" <haiwenyao@...ontech.com>
Subject: Re: [PATCH 3/5] KVM: x86: Use MTRR macros to define possible MTRR MSR ranges

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.