All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
To: Yinghai Lu <yinghai@kernel.org>, "Luis R. Rodriguez" <mcgrof@suse.com>
Cc: Stuart Hayes <stuart.w.hayes@gmail.com>,
	Prarit Bhargava <prarit@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	Toshi Kani <toshi.kani@hp.com>
Subject: Re: Fwd: [PATCH] x86: Use larger chunks in mtrr_cleanup
Date: Thu, 5 Nov 2015 11:43:59 -0800	[thread overview]
Message-ID: <CAB=NE6X3ix5pSp2u6owraV73CfP+JBh+Ct0Ek8bNvw1Ft-5bGw@mail.gmail.com> (raw)
In-Reply-To: <CAE9FiQWm=PLSpiS0ggrVDLPSp7X_-cc3wUfDqsnmt4oYQc89Eg@mail.gmail.com>

On Thu, Nov 5, 2015 at 11:14 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Sep 14, 2015 at 7:46 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>>
>> Booting with 'disable_mtrr_cleanup' works, but the system I am working with
>> isn't actually failing--it just gets ugly error messages.  And the BIOS on the
>> system I am working with had set up the MTRRs correctly.
>
> Please post boot log and /proc/mtrr for:
> 1. without your patch
> 2. without your patch and with disable_mtrr_cleanup in boot command line.
> 3. with your patch.

Stuart,

to provide some context -- I reached out to Yinghai as he wrote the
original mtrr cleanup code. The commit logs seem to read that a crash
was possible on systems with > 4 GiB RAM with some types of BIOSes...
The cleanup code seems to trigger when variable MTRRs do not exist
that are UC, or when all varible MTRRs that exist are just UC + WB
(Yinghai correct me if I'm wrong). The commit log in question
(95ffa2438d0e9 "x86: mtrr cleanup for converting continuous to
discrete layout, v8") was not very clear about the cause of the crash
-- but suppose the issue here was the BIOS on some systems might want
to create some UC variable MTRRs early on and there was no UC MTRRs
available, and I can only guess the cleanup exists as hack for those
BIOSes. Even if that was the case -- its still not clear *why* the
crash would happen but I suppose a driver mishap can happen without UC
guarantees for some devices the BIOS may want to enable UC MTRR on.

To be able to determine what we do upstream we need to understand the
above first. We also need to understand if the cleanup might also be
implicated by userspace drivers using /proc/mtrr, or if a proprietary
driver exists that does use mtrr_add() directly even though PAT has
been available for ages and all drivers are now properly converted.

With clear answers to the above we'll be able to determine what the
right course of action should be for this patch. For instance I'm
inclined to strive to disable the complex cleanup code if we don't
need it anymore, but if we do need it your patch makes sense. If the
patch makes sense then though are we going to have to keep updating
the segment size *every time* as systems grow? That seems rather
silly. And if PAT is prevalent why are vendors adding MTRRs still? The
cleanup seems complex and a major hack for a fix for some BIOSes, I'd
much rather identify the exact issue and only have a fix to address
that case.

  Luis

  reply	other threads:[~2015-11-05 19:44 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <55E477DE.2060106@gmail.com>
2015-08-31 16:05 ` Stuart Hayes
2015-09-03  2:45   ` Luis R. Rodriguez
2015-09-03 12:17     ` Prarit Bhargava
2015-09-03 17:59       ` Luis R. Rodriguez
2015-09-03 18:10         ` Prarit Bhargava
2015-09-03 18:40           ` Luis R. Rodriguez
2015-09-03 19:22             ` Toshi Kani
2015-09-03 19:51               ` Luis R. Rodriguez
2015-09-03 21:31                 ` Toshi Kani
2015-09-03 22:07                   ` Luis R. Rodriguez
2015-09-03 22:25                     ` Toshi Kani
2015-09-03 22:45                       ` Luis R. Rodriguez
2015-09-03 23:21                         ` Toshi Kani
2015-09-03 23:54                           ` Luis R. Rodriguez
2015-09-04  0:48                             ` Toshi Kani
2015-09-04  1:40                               ` Luis R. Rodriguez
2015-09-04 14:56                                 ` Toshi Kani
2015-09-04  6:51                             ` Jan Beulich
2015-09-14 14:46       ` Stuart Hayes
2015-11-05 19:14         ` Yinghai Lu
2015-11-05 19:43           ` Luis R. Rodriguez [this message]
2016-03-16 20:20             ` Luis R. Rodriguez
2016-03-29 17:07               ` Luis R. Rodriguez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAB=NE6X3ix5pSp2u6owraV73CfP+JBh+Ct0Ek8bNvw1Ft-5bGw@mail.gmail.com' \
    --to=mcgrof@do-not-panic.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mcgrof@suse.com \
    --cc=mingo@redhat.com \
    --cc=prarit@redhat.com \
    --cc=stuart.w.hayes@gmail.com \
    --cc=tglx@linutronix.de \
    --cc=toshi.kani@hp.com \
    --cc=x86@kernel.org \
    --cc=yinghai@kernel.org \
    --subject='Re: Fwd: [PATCH] x86: Use larger chunks in mtrr_cleanup' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.