From: "Luis R. Rodriguez" <firstname.lastname@example.org> To: Yinghai Lu <email@example.com>, "Luis R. Rodriguez" <firstname.lastname@example.org> Cc: Stuart Hayes <email@example.com>, Prarit Bhargava <firstname.lastname@example.org>, Thomas Gleixner <email@example.com>, Ingo Molnar <firstname.lastname@example.org>, "H. Peter Anvin" <email@example.com>, Linux Kernel Mailing List <firstname.lastname@example.org>, "the arch/x86 maintainers" <email@example.com>, Toshi Kani <firstname.lastname@example.org> 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 <email@example.com> wrote: > On Mon, Sep 14, 2015 at 7:46 AM, Stuart Hayes <firstname.lastname@example.org> 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
next prev parent 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.email@example.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' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.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.