lkml.org 
[lkml]   [2019]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 09/18] x86/split_lock: Handle #AC exception for split lock
From
Date
On 3/12/19 5:49 PM, Fenghua Yu wrote:
> On Tue, Mar 12, 2019 at 04:51:22PM -0700, Dave Hansen wrote:
>> On 3/12/19 4:00 PM, Fenghua Yu wrote:
>> I don't see any feature checking here. Don't we need to see if this MSR
>> is supported?
>>
>> Shouldn't the code here on systems that don't support split lock
>> disabling be the same as on CONFIG_CPU_SUP_INTEL=n systems?
>
> You are right. Is the following #AC handler code better?

Fenghua, I'd really appreciate if you could take a deep breath and slow
down. The most important thing is getting the right patch out and being
as respectful as possible with reviewer bandwidth.

> @@ -293,7 +294,37 @@ DO_ERROR(X86_TRAP_OLD_MF, SIGFPE, 0, NULL, "coprocessor segment overru
> DO_ERROR(X86_TRAP_TS, SIGSEGV, 0, NULL, "invalid TSS", invalid_TSS)
> DO_ERROR(X86_TRAP_NP, SIGBUS, 0, NULL, "segment not present", segment_not_present)
> DO_ERROR(X86_TRAP_SS, SIGBUS, 0, NULL, "stack segment", stack_segment)
> -DO_ERROR(X86_TRAP_AC, SIGBUS, BUS_ADRALN, NULL, "alignment check", alignment_check)
> +dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)

Is this really an appropriate place to stick this function? Without any
whitespace, and even pushing out the "#undef" that was here before?

> +{
> + unsigned int trapnr = X86_TRAP_AC;
> + char str[] = "alignment check";
> + int signr = SIGBUS;
> +
> + RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> +
> + if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
> + NOTIFY_STOP) {

Please unindent this code block.

> + cond_local_irq_enable(regs);
> + if (!user_mode(regs)) {

Comments please.

The comment about #AC being impossible in the kernel without the split
lock detection feature belongs here, not below.

> + if (!this_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> + return;

Is this consistent with the code that was here before? Basically, if we
are in the kernel, get an #AC and end up here, we just return from this
function? Is that what DO_ERROR() did?

> + /*
> + * Only split lock can generate #AC from kernel. Warn
> + * and disable #AC for split lock on current CPU.
> + */
> + msr_clear_bit(MSR_TEST_CTL,
> + TEST_CTL_ENABLE_SPLIT_LOCK_DETECT_SHIFT);
> + WARN_ONCE(1, "A split lock issue is detected.\n");

Is it an issue? I'd probably say: "split lock operation detected"

> +
> +
> + return;

Extra whitespace.

> + }
> + /* Handle #AC generated from user code. */
> + do_trap(X86_TRAP_AC, SIGBUS, "alignment check", regs,
> + error_code, BUS_ADRALN, NULL);
> + }
> +}
> #undef IP

\
 
 \ /
  Last update: 2019-03-13 17:23    [W:0.064 / U:1.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site