Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936260AbcCQVE0 (ORCPT ); Thu, 17 Mar 2016 17:04:26 -0400 Received: from g4t3425.houston.hp.com ([15.201.208.53]:6393 "EHLO g4t3425.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933058AbcCQVEX (ORCPT ); Thu, 17 Mar 2016 17:04:23 -0400 Message-ID: <1458251807.6393.474.camel@hpe.com> Subject: Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code From: Toshi Kani 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 Date: Thu, 17 Mar 2016 15:56:47 -0600 In-Reply-To: <20160315232916.GJ1990@wotan.suse.de> 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> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.4 (3.18.4-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5246 Lines: 131 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 wrote: > >  :  : > > MTRR code does not have to be dead for the virtual machines with no > > MTRR support.  The code just needs to handle the disabled case > > properly.  I consider this is part of 1) that the kernel keeps the MTRR > > state as disabled. > > For Xen it was possible to use PAT without any of the MTRR code running, > 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. > >  : > > We do not need subarch for this case since presence of the MTRR feature > > can be tested with CPUID and MSR. > > But in this case its about two different cases: > > No MTRR - but you need to emulate PAT > No MTRR - but you can just read what the hypervisor already set up > > So it a given MTRR is disabled for both, what we need then is semantics > 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. > > >   : > > AFAIK, MTRR is the only way to specify UC attribute in physical mode on > > x86.  On ia64, one can simply set the UC bit to a physical address to > > specify UC attribute.  I wish we had something similar. > > 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.  I think I've told you before when I mentioned that BIOS might need to manage fan speed.  Virt BIOS does not need to do such thing. > 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? That > 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. > > > > > > 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? > > > > I think we are good as long as we do not modify it.  The complexity > > comes with the modification. > > Ew. I look at that code and cannot comprehend why we'd ever want to keep > it always running. The code is basically no-op on Xen. > > > > > I can read the above description to also say: > > > > > > > > > > "Hey you need to implement PAT with the same skeleton code as > > > > > MTRR" > > > > > > > > No, I did not say that.  MTRR's rendezvous handler can be > > > > generalized to work with both MTRR and PAT.  We do not need two > > > > separate handlers.  In fact, it needs to be a single handler so > > > > that both can be initialized together. > > > > > > 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? > > > > No, it requires the same procedure as MTRR. > > 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. > > > > >   : > > > >  It just means that Linux can enable PAT on virtual CPUs with PAT & > > > > !MTRR capability. > > > > > > > > > > In fact, the kernel disabling MTRRs is the same as 2). > > > > > > > > > > > > > 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... > > > > > > > > > > > > I am interested to know how Xen managed this.  Is this done by > > > > > > the Xen hypervisor initializes guest's PAT on behalf of the > > > > > > guest kernel? > > > > > > > > > > Yup. And the cache read thingy was reading back its own setup, > > > > > which was different than what Linux used by default IIRC. Juergen > > > > > can elaborate more. > > > > > > > > Yeah, I'd like to make sure that my changes won't break it. > > > > > > I checked through code inspection and indeed, it seems it would break > > > Xen's PAT setup. > > > > > > 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. > > > > Please review the next version. > > Sure.. Thanks! -Toshi