Re: [PATCH v5 09/18] x86/split_lock: Handle #AC exception for split lock

From: Dave Hansen <dave.hansen@intel.com>
To: Fenghua Yu <fenghua.yu@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, H Peter Anvin <hpa@zytor.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Ashok Raj <ashok.raj@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Xiaoyao Li <xiaoyao.li@intel.com>,
	Michael Chan <michael.chan@broadcom.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
	kvm@vger.kernel.org
Subject: Re: [PATCH v5 09/18] x86/split_lock: Handle #AC exception for split lock
Date: Wed, 13 Mar 2019 09:22:17 -0700	[thread overview]
Message-ID: <29ca715d-fe1c-59d6-3dc8-71e70b2427af@intel.com> (raw)
In-Reply-To: <20190313004955.GA31710@romley-ivt3.sc.intel.com>

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