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

[Thomas Gleixner]

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

> ----- On May 14, 2020, at 1:28 PM, Thomas Gleixner t...@linutronix.de wrote:
>
>> Mathieu Desnoyers <mathieu.desnoy...@efficios.com> writes:
>>> ----- On May 5, 2020, at 9:49 AM, Thomas Gleixner t...@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);
 }