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: Thu, 17 Mar 2016 15:56:47 -0600 Message-ID: <1458251807.6393.474.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> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20160315232916.GJ1990@wotan.suse.de> Sender: linux-kernel-owner@vger.kernel.org To: "Luis R. Rodriguez" Cc: Juergen Gross , Boris Ostrovsky , Matt Fleming , Olof Johansson , Paul Stewart , Borislav Petkov , Ingo Molnar , "H. Peter Anvin" , Thomas Gleixner , Paul Gortmaker , X86 ML , "linux-kernel@vger.kernel.org" , xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On Wed, 2016-03-16 at 00:29 +0100, Luis R. Rodriguez wrote: > On Tue, Mar 15, 2016 at 05:48:44PM -0600, Toshi Kani wrote: > > On Tue, 2016-03-15 at 01:15 +0100, Luis R. Rodriguez wrote: > > > On Fri, Mar 11, 2016 at 06:16:36PM -0700, Toshi Kani wrote: > > > > On Fri, 2016-03-11 at 15:34 -0800, Luis R. Rodriguez wrote: > > > > > On Fri, Mar 11, 2016 at 3:56 PM, Toshi Kani > > > > > wrote: > > > > > > On Fri, 2016-03-11 at 23:17 +0100, Luis R. Rodriguez wrote: > > > > > > > On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrot= e: > > =C2=A0: =C2=A0: > > MTRR code does not have to be dead for the virtual machines with no > > MTRR support. =C2=A0The code just needs to handle the disabled case > > properly. =C2=A0I consider this is part of 1) that the kernel keeps= the MTRR > > state as disabled. >=20 > For Xen it was possible to use PAT without any of the MTRR code runni= ng, > I don't see why its needed then and why other virtual machines that > only need PAT need it. Virtual BIOS does not need MTRRs since it does not manage the platform. > > =C2=A0: > > We do not need subarch for this case since presence of the MTRR fea= ture > > can be tested with CPUID and MSR. >=20 > But in this case its about two different cases: >=20 > No MTRR - but you need to emulate PAT > No MTRR - but you can just read what the hypervisor already set up >=20 > So it a given MTRR is disabled for both, what we need then is semanti= cs > to distinguish between qemu32 and Xen PV. Curious to see what you use= , > in your current new patch it was not clear what you did. X86_FEATURE_PAT tells us if CPU supports PAT. So, the kernel can distinguish the two cases above without knowing qemu32 or Xen. > > >=C2=A0 =C2=A0: > > AFAIK, MTRR is the only way to specify UC attribute in physical mod= e on > > x86. =C2=A0On ia64, one can simply set the UC bit to a physical add= ress to > > specify UC attribute. =C2=A0I wish we had something similar. >=20 > Whoa, you mean on the BIOS? This is BIG deal if true. Why haven't you > just said this a long time ago? Yes, BIOS. =C2=A0I think I've told you before when I mentioned that BIO= S might need to manage fan speed. =C2=A0Virt BIOS does not need to do such thin= g. > On x86 Linux code we now have ioremap_uc() that can't use MTRR behind= the > scenes, why would something like this on the BIOS not be possible? Th= at > ultimately uses set_pte_at(). What limitations are there on the BIOS > that prevent us from just using strong UC for PAT on the BIOS? Because it requires to run in virtual mode with page tables. > > > > The kernel can be ignorant of the MTRR setup as long as it does > > > > not modify it. > > >=20 > > > Sure, we're already there. The kernel no longer modifies the > > > MTRR setup unless of course you boot without PAT enabled. I think > > > we need to move beyond that to ACPI if we can to let regular > > > Linux boots never have to deal with MTRR at all. The code is > > > complex and nasty why not put let folks put a nail on the coffin = for > > > good? > >=20 > > I think we are good as long as we do not modify it. =C2=A0The compl= exity > > comes with the modification. >=20 > Ew. I look at that code and cannot comprehend why we'd ever want to k= eep > it always running. The code is basically no-op on Xen. > > > > > I can read the above description to also say: > > > > >=20 > > > > > "Hey you need to implement PAT with the same skeleton code as > > > > > MTRR" > > > >=20 > > > > No, I did not say that. =C2=A0MTRR's rendezvous handler can be > > > > generalized to work with both MTRR and PAT. =C2=A0We do not nee= d two > > > > separate handlers. =C2=A0In fact, it needs to be a single handl= er so > > > > that both can be initialized together. > > >=20 > > > I'm not sure if that's really needed. Doesn't PAT just require > > > setting the wrmsrl(MSR_IA32_CR_PAT, pat) for each AP? > >=20 > > No, it requires the same procedure as MTRR. >=20 > MTRR has a bunch of junk that is outside of the scope of the generic > procedure which I'd hope we can skip. We can skip the part that modifies MTRR setup. I think that is the part= you think is a junk. > > > > >=C2=A0 =C2=A0: > > > > =C2=A0It just means that Linux can enable PAT on virtual CPUs w= ith PAT & > > > > !MTRR capability. > > > >=20 > > > > > > In fact, the kernel disabling MTRRs is the same as 2). > > > > > >=20 > > > > > > > I'll also note Xen managed to enable PAT only without > > > > > > > enabling MTRR, this was done through pat_init_cache_modes= () > > > > > > > -- not sure if this can be leveraged for qemu32... > > > > > >=20 > > > > > > I am interested to know how Xen managed this.=C2=A0=C2=A0Is= this done by > > > > > > the Xen hypervisor initializes guest's PAT on behalf of the > > > > > > guest kernel? > > > > >=20 > > > > > Yup. And the cache read thingy was reading back its own setup= , > > > > > which was different than what Linux used by default IIRC. Jue= rgen > > > > > can elaborate more. > > > >=20 > > > > Yeah, I'd like to make sure that my changes won't break it. > > >=20 > > > I checked through code inspection and indeed, it seems it would b= reak > > > Xen's PAT setup. > > >=20 > > > For the record: the issue here was code that should not run ran, = that > > > is dead code ran. I'm working towards a generic solution for this= =2E > >=20 > > Please review the next version. >=20 > Sure.. Thanks! -Toshi