On Thu, Sep 03, 2015 at 03:31:42PM -0600, Toshi Kani wrote:
> On Thu, 2015-09-03 at 21:51 +0200, Luis R. Rodriguez wrote:
> > On Thu, Sep 03, 2015 at 01:22:42PM -0600, Toshi Kani wrote:
> > > On Thu, 2015-09-03 at 20:40 +0200, Luis R. Rodriguez wrote:
> > > > On Thu, Sep 03, 2015 at 02:10:14PM -0400, Prarit Bhargava wrote:
> > > > > 
> > > > > 
> > > > > On 09/03/2015 01:59 PM, Luis R. Rodriguez wrote:
> > > > > > On Thu, Sep 03, 2015 at 08:17:02AM -0400, Prarit Bhargava wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 09/02/2015 10:45 PM, Luis R. Rodriguez wrote:
> > > > > > > > On Mon, Aug 31, 2015 at 11:05:33AM -0500, Stuart Hayes wrote:
> > > > > > > > > Increase the range of chunk sizes tried in mtrr_cleanup() so 
> > > > > > > > > it is 
> > > > > > > > > able to map large memory configs into MTRRs.
> > > > > > > > > 
> > > > > > > > > Currently, mtrr_cleanup() will fail with large memory 
> > > > > > > > > configurations, because it limits chunk_size to 2GB, which 
> > > > > > > > > means 
> > > > > > > > > that each MTRR can only cover 2GB of memory.  With a memory 
> > > > > > > > > size 
> > > > > > > > > of, say, 256GB, and ten variable MTRRs (such as some recent 
> > > > > > > > > Intel 
> > > > > > > > > CPUs have), it is not possible to set up the MTRRs to cover 
> > > > > > > > > all of
> > > > > > > > > memory.
> > > > > > > > 
> > > > > > > > Linux drivers no longer use MTRR so why is the cleanup needed, 
> > > > > > > > ie, 
> > > > > > > > what would happen if the cleanup is just skipped in your case ?
> > > > > > > 
> > > > > > > The infiniband & video drivers still use MTRR (or at least it was 
> > > > > > > my
> > > > > > > understanding that they do). 
> > > > > > 
> > > > > > There were a few stragglers left on v4.2, I have transformed them 
> > > > > > in the 
> > > > > > latest development changes and those tranformations are now part of 
> > > > > > linux-next. If this is specific to a driver you may want to first 
> > > > > > ensure 
> > > > > > you backport the required patch that transforms the driver to use 
> > > > > > proper 
> > > > > > PAT interfaces, v4.2 should have most updates but there were still 
> > > > > > a few 
> > > > > > left. Just make sure your driver doesn't call mtrr_add() directly 
> > > > > > and if 
> > > > > > it doesn't then you should be OK.
> > > > > > 
> > > > > > > In any case, Stuart -- could you try booting with
> > > > > > > 'disable_mtrr_cleanup' as a kernel parameter?
> > > > > > 
> > > > > > Indeed, please I'd like to hear back. Be sure to have the 
> > > > > > respective 
> > > > > > driver transformation in place, what driver are you using exactly? 
> > > > > > In 
> > > > > > the event that you argue this is still needed I'd like to know 
> > > > > > exaclty 
> > > > > > *why*, the comit log does not mention any of that at all.
> > > > > > 
> > > > > 
> > > > > Well ... we are trying to also fix this in older kernels too, *cough* 
> > > > > RHEL
> > > > > *cough*, so that's where the patch comes from.  If upstream is going 
> > > > > to
> > > > > deprecate/remove mtrr support so be it. 
> > > > 
> > > > Check linux-next, and Documentation/x86/mtrr.txt
> > > > 
> > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Docume
> > > > ntation/x86/mtrr.txt
> > > > 
> > > > The platform use of MTRR is the only thing you should be concerned over 
> > > > but 
> > > > as noted returning MTRR_TYPE_INVALID should suffice if the OS does not 
> > > > make
> > > > any driver use / modifications.
> > > 
> > > The following sentence in the "mtrr.txt" is not correct.  (Sorry I should 
> > > have
> > > caught it earlier.)  mtrr_type_lookup() returns MTRR_TYPE_INVALID when 
> > > MTRRs
> > > are disabled, i.e. MTRRs are not set by neither firmware nor OS.  Most of 
> > > the
> > > firmwares enable them, though.
> > > 
> > > "If MTRRs are only set up by the platform firmware code though and the OS 
> > > does
> > > not make any specific MTRR mapping requests mtrr_type_lookup() should 
> > > always
> > > return MTRR_TYPE_INVALID."
> > 
> > So this should be clarified to say -- that because platform firmware *may* 
> > make
> > use of MTRRs, even if all OS drivers are no longer making use of MTRRs
> > directly, we still need mtrr_type_lookup() to return the right type ?
> 
> That is correct, and I think the doc already states that.  I'd suggest
> the following minor changes to the doc:
> 
> Thanks,
> -Toshi
> 
> ===
> diff --git a/Documentation/x86/mtrr.txt b/Documentation/x86/mtrr.txt
> index dc3e703..db1d8f0 100644
> --- a/Documentation/x86/mtrr.txt
> +++ b/Documentation/x86/mtrr.txt
> @@ -13,15 +13,12 @@ non-PAT systems while a no-op but equally effective on 
> PAT enabled systems.
> 
>  Even if Linux does not use MTRRs directly, some x86 platform firmware may 
> still
>  set up MTRRs early before booting the OS. They do this as some platform
> -firmware may still have implemented access to MTRRs which would be controlled
> -and handled by the platform firmware directly. An example of platform use of
> +firmware may rely on cache types set by MTRRs. An example of platform use of
>  MTRRs is through the use of SMI handlers, one case could be for fan control,
>  the platform code would need uncachable access to some of its fan control
>  registers. Such platform access does not need any Operating System MTRR code 
> in
>  place other than mtrr_type_lookup() to ensure any OS specific mapping 
> requests
> -are aligned with platform MTRR setup. If MTRRs are only set up by the 
> platform
> -firmware code though and the OS does not make any specific MTRR mapping
> -requests mtrr_type_lookup() should always return MTRR_TYPE_INVALID.
> +are aligned with platform MTRR setup.

These are still at odds, for instance, I was under the impression we can
just have the OS return MTRR_TYPE_INVALID if the OS / drivers never used
or set up MTRR, but the platform did, above (not the patch) you seem to be
saying that even if the OS didn't modify MTRRs the OS still needs to return
the appropriately set up MTRR type by firmware. This is different. Can you
clarify?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to