Re: [patch V4 part 4 15/24] x86/db: Split out dr6/7 handling

From: Thomas Gleixner <tglx@linutronix.de>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
	paulmck <paulmck@kernel.org>, Andy Lutomirski <luto@kernel.org>,
	Alexandre Chartre <alexandre.chartre@oracle.com>,
	Frederic Weisbecker <frederic@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Petr Mladek <pmladek@suse.com>, rostedt <rostedt@goodmis.org>,
	"Joel Fernandes\, Google" <joel@joelfernandes.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Juergen Gross <jgross@suse.com>, Brian Gerst <brgerst@gmail.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Will Deacon <will@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [patch V4 part 4 15/24] x86/db: Split out dr6/7 handling
Date: Fri, 15 May 2020 16:32:25 +0200	[thread overview]
Message-ID: <87h7whe0au.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <2107385142.21313.1589478370190.JavaMail.zimbra@efficios.com>

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> ----- On May 14, 2020, at 1:28 PM, Thomas Gleixner tglx@linutronix.de wrote:
>
>> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
>>> ----- On May 5, 2020, at 9:49 AM, Thomas Gleixner tglx@linutronix.de wrote:
>>>> +
>>>> +static __always_inline void debug_exit(unsigned long dr7)
>>>> +{
>>>> +	set_debugreg(dr7, 7);
>>>> +}
>>>
>
> * Question 1
>
>>> Out of curiosity, what prevents the compiler from moving instructions
>>> outside of the code regions surrounded by entry/exit ? This is an always
>>> inline, which invokes set_debugreg which is inline for CONFIG_PARAVIRT_XXL=n,
>>> which in turn uses an asm() (not volatile), without any memory
>>> clobber.

I misread 'surrounded by entry/exit'.

Reading it again I assume you mean nmi_enter/exit(). And yes, there is a
compiler barrier missing.

Thanks,

        tglx

8<----------------

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index e11ad0791dc3..ae1e61345225 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -718,6 +718,13 @@ static __always_inline void debug_enter(unsigned long *dr6, unsigned long *dr7)
 	get_debugreg(*dr7, 7);
 	set_debugreg(0, 7);
 
+	/*
+	 * Ensure the compiler doesn't lower the above statements into
+	 * the critical section; disabling breakpoints late would not
+	 * be good.
+	 */
+	barrier();
+
 	/*
 	 * The Intel SDM says:
 	 *
@@ -737,6 +744,12 @@ static __always_inline void debug_enter(unsigned long *dr6, unsigned long *dr7)
 
 static __always_inline void debug_exit(unsigned long dr7)
 {
+	/*
+	 * Ensure the compiler doesn't raise this statement into
+	 * the critical section; enabling breakpoints early would
+	 * not be good.
+	 */
+	barrier();
 	set_debugreg(dr7, 7);
 }
 
