From mboxrd@z Thu Jan 1 00:00:00 1970 From: Toshi Kani Subject: Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Date: Tue, 29 Mar 2016 18:16:34 -0600 Message-ID: <1459296994.6393.748.camel@hpe.com> References: <1457671546-13486-1-git-send-email-toshi.kani@hpe.com> <1457671546-13486-3-git-send-email-toshi.kani@hpe.com> <20160311092400.GB4347@pd.tnic> <1457722632.6393.130.camel@hpe.com> <20160311221747.GC25147@wotan.suse.de> <1457740571.6393.236.camel@hpe.com> <1457745396.6393.257.camel@hpe.com> <20160315001501.GF25147@wotan.suse.de> <1458085724.6393.425.camel@hpe.com> <20160315232916.GJ1990@wotan.suse.de> <1458251807.6393.474.camel@hpe.com> <1458336958.6393.544.camel@hpe.com> <1459288009.6393.699.camel@hpe.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: "Luis R. Rodriguez" Cc: Boris Ostrovsky , "xen-devel@lists.xensource.com" , Thomas Gleixner , Matt Fleming , Paul Gortmaker , Borislav Petkov , X86 ML , Paul Stewart , Ingo Molnar , Olof Johansson , "linux-kernel@vger.kernel.org" , "H. Peter Anvin" , Juergen Gross , Jan Beulich , Stuart Hayes , Yinghai Lu , Prarit Bhargava List-Id: xen-devel@lists.xenproject.org On Tue, 2016-03-29 at 15:12 -0700, Luis R. Rodriguez wrote: > On Tue, Mar 29, 2016 at 2:46 PM, Toshi Kani wrot= e: > > On Tue, 2016-03-29 at 10:14 -0700, Luis R. Rodriguez wrote: > > > On Fri, Mar 18, 2016 at 2:35 PM, Toshi Kani > > > wrote: =C2=A0: > > >=20 > > > Do we really need UC for the fan? > >=20 > > When you say "we", are you referring Xen guests?=C2=A0=C2=A0Xen gue= sts do not > > need to control the fan, so they do not need UC set in MTRRs. > >=20 > > In general, yes, MMIO registers need UC when they need to be access= ed. >=20 > Curious, what does a BIOS do for fan control when MTRRs are disabled? You mean, when the kernel modified the MTRR setup and disabled them. =C2= =A0BIOS would assume the original setup and still access the registers. =C2=A0T= his may lead to undefined behavior and may result in a system crash. > Also what if a BIOS just set MSR_MTRRdefType to uncachable only ? Many BIOSes actually set the default type to UC. =C2=A0MTRRs then cover= regular memory with WB. > Wouldn't that help simplify the BIOS when systems are known as not > wanting to deal with reading MTRRs on the kernel front, even if its > just to read the setup ? Nope. > I'm trying to determine exactly why a BIOS cannot simply enable use a= n > alternative for what it needs for fan control and let the kernel live > without any MTRR code at run time as an option. Although the > documentation says that the same "procedure" is needed for PAT setup, > I see it possible to split the skeleton of the code and have each > peace of code live separately and compartmentalized, they'd just have > respective calls on the skeleton of the procedure. I agree that the MTRR rendezvous handler can be improved for PAT, but I= do not see a compelling reason to make such change now. =C2=A0With my fix,= I think the code works reasonably for Xen. > > > What is the default for PAT? > >=20 > > There is no such thing as the default for PAT. > >=20 > > > Can't > > > the same be used so that we way by default all ranges match what = is > > > also the default by PAT? Would that really break fan control ? If= we > > > have a match should't we be able to not have to worry about MTRRs= at > > > all in-kernel even on bare metal? > >=20 > > We do not need to know about BIOS impl, such as fan control, etc.=C2= =A0=C2=A0The > > point is that if BIOS sets MTRRs, then the kernel keeps their setup= =2E >=20 > Right, if the kernel no longer uses it directly it seems like an > aweful lot of code to keep updating simply for a BIOS requirement, I'= m > trying to see if we can have the option to live without this > requirement. Please be aware of the hibernation case. I think this procedure involve= s setting MTRRs back to the original setup. > > If (virtual) BIOS does not enable MTRRs, the kernel keeps them > > disabled.=C2=A0=C2=A0We just need not to mess with the setup. >=20 > Sure, thanks! I'm trying to see if we can have a similar option on ba= re > metal. >=20 > > > Another option, which I've alluded to on the Xen thread is skippi= ng > > > over the MTRR space from the e820 map. Is that not possible ? Thi= s > > > could be last resort... but which I'm hinting more for the Xen si= de > > > of things if we *really* need get_mtrr() on the Xen guest side of > > > things... > >=20 > > There is no MTRR space in the e820 map since they are MSRs.=C2=A0=C2= =A0Since Xen > > guests disable MTRRs, I do not think you have any issue here... >=20 > Xen seems to clip the e820 map given to a guest in certain MTRR > conditions, see init_e820(), this calls > machine_specific_memory_setup() which later clips MTRR if > mtrr_top_of_ram(). This is an Intel check that trims the e820 map if > MTRRs were found to be enabled and the default MTRR is not write-back= =2E > If returns the address of the first non write-back variable MTRR, it > uses clip_to_limit() to limit the exposed memory [0], notice how > clip_to_limit() is also used to generally limit exposed memory throug= h > the opt_mem boot parameter as well. Its not exactly clear why that's > done, but this looks very similar to the Linux MTRR cleanup -- see > x86_get_mtrr_mem_range(). >=20 > [0] http://xenbits.xen.org/gitweb/?p=3Dxen.git;a=3Dblob;f=3Dxen/arch/= x86/e820.c It looks to me that the code makes sure all E820_RAM ranges in the e820 table are covered by WB entries of MTRRs. =C2=A0If not, it trims the e8= 20 table. I suppose it tries to react on a case when someone modified MTRRs and resulted in mismatch with the e820 table. =C2=A0I'd think you do not ne= ed this code as long as you do not modify the MTRR setup. Thanks, -Toshi