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

[Thomas Gleixner]

Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> writes:

> ----- On May 14, 2020, at 1:28 PM, Thomas Gleixner tglx@xxxxxxxxxxxxx wrote:
>
>> Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> writes:
>>> ----- On May 5, 2020, at 9:49 AM, Thomas Gleixner tglx@xxxxxxxxxxxxx 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);
 }