linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: "Yu, Fenghua" <fenghua.yu@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Shanbhogue, Vedvyas" <vedvyas.shanbhogue@intel.com>,
	"Luck, Tony" <tony.luck@intel.com>, H Peter Anvin <hpa@zytor.com>,
	Andy Lutomirski <luto@kernel.org>,
	"Shankar, Ravi V" <ravi.v.shankar@intel.com>,
	"Li, Xiaoyao" <xiaoyao.li@intel.com>, x86 <x86@kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC] x86/bus_lock: Enable bus lock detection
Date: Wed, 29 Jul 2020 11:46:14 -0700	[thread overview]
Message-ID: <20200729184614.GI27751@linux.intel.com> (raw)
In-Reply-To: <e23b04a2adc54a5dbca48271987de822@intel.com>

On Wed, Jul 29, 2020 at 11:09:16AM -0700, Yu, Fenghua wrote:
> > > Some CPUs have ability to notify the kernel by an #DB trap after the
> > > instruction acquires a bus lock and is executed. This allows the
> > > kernel to enforce user application throttling or mitigations and also
> > > provides a better environment to debug kernel split lock issues since
> > > the kernel can continue instead of crashing.
> > >
> > > #DB for bus lock detect fixes all issues in #AC for split lock detect:
> > 
> > Fixes "all" issues... and creates some new ones, e.g. there are use cases
> > where preventing the split lock from happening in the first place is strongly
> > desired.  It's why that train wreck exists.
> 
> Bus Lock Detection doesn't replace Split Lock Detection. If both features
> are enabled, default behavior is warning from bus lock, fatal behavior is
> still from split lock. See the behavior table as follows.
> 
> > 
> > > 1) It's architectural ... just need to look at one CPUID bit to know it
> > >    exists
> > > 2) The IA32_DEBUGCTL MSR, which reports bus lock in #DB, is per-thread.
> > >    So each process or guest can have different behavior.
> > > 3) It has support for VMM/guests (new VMEXIT codes, etc).
> > >
> > > Use the existing kernel command line option "split_lock_detect=" to
> > > handle #DB for bus lock:
> > 
> > Are SLD and BLD mutually exclusive?  Can we even guarantee that given the
> > track record of SLD?  If not, we'll likely want to allow the user to choose
> > between SDL and BLD via split_lock_detect.
> 
> The two hardware features can be enabled on the same platform.
> But they are mutually exclusive in the kernel because #AC from SLD happens
> before the instruction is executed and #DB happens after the instruction is
> executed.
> 
> Right now, if both of them are enabled, "warn" behavior goes to
> bus lock and "fatal" behavior goes to split lock.
> 
> Do you want the user to override the behaviors by something like this?
> 
> split_lock_detect=warn[,sld]: if given "sld" while both features are enabled,
> warn behavior is from split lock instead of bus lock detection.
> 
> split_lock_detect=fatal[,bld]: if given "bld" while both features are enabled,
> fatal behavior is from bus lock detection.

IMO these should be completely independent features (that happen to share
some code).

BLD in fatal mode doesn't make any sense because it can't be fatal without
a completely different implementation, e.g. the bus lock has already
happened and the application can eat the SIGBUS.  The current SLD code
works because the split lock is prevented entirely, i.e. eating SIGBUS
doesn't allow the application to make forward progress.

Smushing the two into a single option is confusing, e.g. from the table
below it's not at all clear what will happen if sld=fatal, both features
are supported, and the kernel generates a split lock.

Given that both SLD (per-core, not architectural) and BLD (#DB recursion and
inverted DR6 flag) have warts, it would be very nice to enable/disable them
independently.  The lock to non-WB behavior for BLD may also be problematic,
e.g. maybe it turns out that fixing drivers to avoid locks to non-WB isn't
as straightforward as avoiding split locks.

> > >  /*
> > >   * Default to sld_off because most systems do not support split lock
> > > detection
> > > - * split_lock_setup() will switch this to sld_warn on systems that
> > > support
> > > - * split lock detect, unless there is a command line override.
> > > + * sld_state_setup() will switch this to sld_warn on systems that
> > > + support
> > > + * split lock/bus lock detect, unless there is a command line override.
> > >   */
> > >  static enum split_lock_detect_state sld_state __ro_after_init =
> > > sld_off;  static u64 msr_test_ctrl_cache __ro_after_init;
> > > +/* Split lock detection is enabled if it's true. */ static bool sld;
> > > +/* Bus lock detection is enabled if it's true. */ static bool bld;
> > 
> > Why can't these be tracked/reflected in X86_FEATURE_*?
> 
> sld and bld are enabled depending on kernel parameter "split_lock_detect=".
> They are not static and cannot be tracked by static X86_FEATURE_*.

X86_FEATURE_* flags aren't static, the kernel sets/clears them all over the
place.

  reply	other threads:[~2020-07-29 18:46 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-17 21:35 [PATCH RFC] x86/bus_lock: Enable bus lock detection Fenghua Yu
2020-07-29  3:02 ` Sean Christopherson
2020-07-29  8:50   ` peterz
2020-07-29 18:09   ` Yu, Fenghua
2020-07-29 18:46     ` Sean Christopherson [this message]
2020-07-29 19:42       ` Fenghua Yu
2020-07-29 20:00         ` Sean Christopherson
2020-07-29 20:09           ` peterz
2020-07-29 20:35           ` Fenghua Yu
2020-07-29 20:39             ` Sean Christopherson
2020-07-29 22:07               ` Fenghua Yu
2020-07-29 23:28                 ` Sean Christopherson
2020-07-29  8:49 ` peterz
2020-07-29 20:40   ` Fenghua Yu
2020-07-29 21:09     ` peterz
2020-07-30 10:08       ` Thomas Gleixner

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=20200729184614.GI27751@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=bp@alien8.de \
    --cc=fenghua.yu@intel.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=ravi.v.shankar@intel.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=vedvyas.shanbhogue@intel.com \
    --cc=x86@kernel.org \
    --cc=xiaoyao.li@intel.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).