Re: [PATCH v2 3/4] x86/split_lock: Handle #AC exception for split lock

[Dave Hansen]

> +/*
> + * #AC handler for split lock is called by generic #AC handler.
> + *
> + * On split lock in kernel, warn and disable #AC for split lock on current CPU.
> + *
> + * On split lock in user process, send SIGBUS in the generic #AC handler.
> + */

Don't comment the function, comment the code, please.  The thing that
needs to be here that is missing is what the return values mean.

> +bool do_ac_split_lock(struct pt_regs *regs)
> +{
> +	/* Generic #AC handler will handle split lock in user. */
> +	if (user_mode(regs))
> +		return false;
> +
> +	/* Clear the split lock bit to disable the feature on local CPU. */
> +	msr_clear_bit(MSR_TEST_CTL, MSR_TEST_CTL_ENABLE_AC_SPLIT_LOCK_SHIFT);
> +
> +	WARN_ONCE(1, "A split lock issue is detected. Please FIX it\n");

I think folks understand that warnings need to get fixed.  They don't
need to be urged to FIX IT IN CAPS, or asked nicely.  This can simply be:

	"lock split across cacheline boundary"

But, warning here is also not super useful.  Shouldn't we be dumping out
the info in 'regs' instead of the current context?  We don't care about
the state in the #AC handler, we care about 'regs'.


> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 18de4e35a4e5..ca4ef8325dfe 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -954,6 +954,8 @@ void __init setup_arch(char **cmdline_p)
>  	parse_early_param();
>  
>  	detect_ac_split_lock();

This ^ needs:

	/* Do detection only on the boot cpu. */

> +	/* Set up #AC for split lock at the earliest phase. */
> +	setup_ac_split_lock();
>  
>  	if (efi_enabled(EFI_BOOT))
>  		efi_memblock_x86_reserve_range();
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index c2f7d1d2a5c3..d6b224e6284f 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -225,6 +225,9 @@ static void notrace start_secondary(void *unused)
>  #endif
>  	load_current_idt();
>  	cpu_init();
> +
> +	setup_ac_split_lock();

and:

	/* Feature detection was done on the boot cpu, only do setup */

> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index e6db475164ed..dd309a7b46bd 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -61,6 +61,7 @@
>  #include <asm/mpx.h>
>  #include <asm/vm86.h>
>  #include <asm/umip.h>
> +#include <asm/cpu.h>
>  
>  #ifdef CONFIG_X86_64
>  #include <asm/x86_init.h>
> @@ -318,7 +319,36 @@ DO_ERROR(X86_TRAP_OLD_MF, SIGFPE,  "coprocessor segment overrun",coprocessor_seg
>  DO_ERROR(X86_TRAP_TS,     SIGSEGV, "invalid TSS",		invalid_TSS)
>  DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present",	segment_not_present)
>  DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
> -DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
> +
> +dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)
> +{
> +	unsigned int trapnr = X86_TRAP_AC;
> +	char str[] = "alignment check";
> +	int signr = SIGBUS;
> +	siginfo_t info;
> +	int ret;
> +
> +	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) {

There does not seem to be a _lot_ of consistency for whether
notify_die() gets called before or after kernel exceptions are handled.
Why did you choose to do it this way?

Also, please unindent this block and just return if notify_die() returns
false.

> +		/* #AC exception could be handled by split lock handler. */
> +		ret = do_ac_split_lock(regs);
> +		if (ret) {
> +			cond_local_irq_enable(regs);
> +
> +			return;
> +		}
> +
> +		cond_local_irq_enable(regs);

FWIW, you can consolidate the cond_local_irq_enable() calls:

	ret = do_ac_split_lock(regs);
	cond_local_irq_enable(regs);
	if (ret)
		return;

	...