Message-ID: <20191121173444.GA5581@agluck-desk2.amr.corp.intel.com>
Date:   Thu, 21 Nov 2019 09:34:44 -0800
From:   "Luck, Tony" <tony.luck@...el.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v10 6/6] x86/split_lock: Enable split lock detection by
 kernel parameter

On Thu, Nov 21, 2019 at 06:12:14PM +0100, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@...radead.org> wrote:
> 
> > On Thu, Nov 21, 2019 at 07:04:44AM +0100, Ingo Molnar wrote:
> > > * Fenghua Yu <fenghua.yu@...el.com> wrote:
> > 
> > > > +	split_lock_detect
> > > > +			[X86] Enable split lock detection
> > > > +			This is a real time or debugging feature. When enabled
> > > > +			(and if hardware support is present), atomic
> > > > +			instructions that access data across cache line
> > > > +			boundaries will result in an alignment check exception.
> > > > +			When triggered in applications the kernel will send
> > > > +			SIGBUS. The kernel will panic for a split lock in
> > > > +			OS code.
> > > 
> > > It would be really nice to be able to enable/disable this runtime as 
> > > well, has this been raised before, and what was the conclusion?
> > 
> > It has, previous versions had that. Somehow a lot of things went missing
> > and we're back to a broken neutered useless mess.
> > 
> > The problem appears to be that due to hardware design the feature cannot
> > be virtualized, and instead of then disabling it when a VM runs/exists
> > they just threw in the towel and went back to useless mode.. :-(
> > 
> > This feature MUST be default enabled, otherwise everything will
> > be/remain broken and we'll end up in the situation where you can't use
> > it even if you wanted to.
> 
> Agreed.
> 
> > And I can't be arsed to look it up, but we've been making this very 
> > same argument since very early (possible the very first) version.
> 
> Yeah, I now have a distinct deja vu...

You'll notice that we are at version 10 ... lots of things have been tried
in previous versions. This new version is to get the core functionality
in, so we can build fancier features later.  Painful experience has shown
that trying to do this all at once just leads to churn with no progress.

Enabling by default at this point would result in a flurry of complaints
about applications being killed and kernels panicing. That would be
followed by:

#include <linus/all-caps-rant-about-backwards-compatability.h>

and the patches being reverted.

This version can serve a very useful purpose. CI systems with h/w that
supports split lock can enable it and begin the process of finding
and fixing the remaining kernel issues. Especially helpful if they run
randconfig and fuzzers.

We'd also find out which libraries and applications currently use
split locks.

Real-time folks that have identified split lock as a fatal (don't meet
their deadlines issue) could also enable it as is (because it is better
to crash the kernel and have the laser be powered down than to keep
firing long past the point it should have stopped).

Any developer with concerns about their BIOS using split locks can also
enable using this patch and begin testing today.

I'm totally on board with follow up patches providing extra features like:

	A way to enable/disable at run time.

	Providing a way to allow but rate limit applications that cause
	split locks.

	Figuring out something useful to do with virtualization.

Those are all good things to have - but we won't get *any* of them if we
wait until *all* have them have been perfected.

<soapbox>
So let's just take the first step now and solve world hunger tomorrow.
</soapbox>

-Tony

Powered by blists - more mailing lists