Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v4 2/4] x86/bus_lock: Handle warn and fatal in #DB for bus lock | Date | Wed, 27 Jan 2021 22:16:15 +0100 |
| |
On Tue, Nov 24 2020 at 20:52, Fenghua Yu wrote:
> #DB for bus lock is enabled by bus lock detection bit 2 in DEBUGCTL MSR > while #AC for split lock is enabled by split lock detection bit 29 in > TEST_CTRL MSR. > > Delivery of #DB for bus lock in userspace clears DR6[11]. To avoid > confusion in identifying #DB, #DB handler sets the bit to 1 before > returning to the interrupted task. > > Use the existing kernel command line option "split_lock_detect=" to handle > #DB for bus lock: > > split_lock_detect= > #AC for split lock #DB for bus lock > > off Do nothing Do nothing > > warn Kernel OOPs Warn once per task and > Warn once per task and and continues to run. > disable future checking When both features are > supported, warn in #DB
Which means that we don't catch kernel split locks anymore with 'warn' if bus lock detection is supported. WHY? There is zero rationale for this change in the changelog.
> fatal Kernel OOPs Send SIGBUS to user > Send SIGBUS to user When both features are > supported, split lock > triggers #AC and bus lock > from non-WB triggers #DB.
> /* > - * 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. > + * Default to sld_off because most systems do not support split lock detection. > + * 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;
Why did you bother with 3 letters? bool s, b; along with comments explaining what it means would have been sufficient, right?
sld_enable/bld_enable would be too self explaining and this also lacks __ro_after_init
Aside of that it's beyond silly because bld and sld are just shadowing the corresponding CPU feature bits. So what are these variables gaining aside of confusion?
> +/* Bus lock detection is enabled if it's true. */ > +static bool bld; > > +static void __init sld_state_setup(void)
This is confusing as hell. sld_state_setup() is used for bus lock as well and split_lock_detect_state is not less confusing. It took me five reads to figure out how all of that works.
> +static void __init _split_lock_setup(void)
We generally use two underscores for readability sake.
> +{ > + if (!split_lock_verify_msr(false)) { > + pr_info("MSR access failed: Disabled\n");
> /* > @@ -1079,6 +1084,15 @@ static void sld_update_msr(bool on) > > static void split_lock_init(void) > { > + /* > + * If supported, #DB for bus lock will handle warn > + * and #AC for split lock is disabled.
Why does this disable the kernel detection? Just because?
> +void handle_bus_lock(struct pt_regs *regs) > +{ > + if (!bld) > + return;
How is #DB ever calling this function when the debug MSR bit is not set?
> -void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) > +static void __init split_lock_setup(struct cpuinfo_x86 *c) > { > const struct x86_cpu_id *m; > u64 ia32_core_caps; > @@ -1189,5 +1237,43 @@ void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) > } > > cpu_model_supports_sld = true; > - split_lock_setup(); > + _split_lock_setup(); > +} > + > +static void sld_state_show(void) > +{ > + if (!bld && !sld) > + return; > + > + switch (sld_state) { > + case sld_off: > + pr_info("disabled\n"); > + break; > + case sld_warn: > + if (bld) > + pr_info("#DB: warning about user-space bus_locks\n"); > + else > + pr_info("#AC: crashing the kernel about kernel split_locks and warning about user-space split_locks\n");
crashing about?
> + break; > + case sld_fatal: > + if (sld) > + pr_info("#AC: crashing the kernel on kernel split_locks and sending SIGBUS on user-space split_locks\n"); > + if (bld) > + pr_info("#DB: sending SIGBUS on user-space bus_locks%s\n", sld ? " from non-WB" : ""); > + break; > + }
Thanks,
tglx
| |