> On May 28, 2020, at 2:34 PM, Peter Zijlstra <pet...@infradead.org> wrote:
>
> On Thu, May 28, 2020 at 11:15:50PM +0200, Peter Zijlstra wrote:
>>> On Thu, May 28, 2020 at 09:52:30PM +0100, Andrew Cooper wrote:
>>> On 28/05/2020 21:19, Peter Zijlstra wrote:
>>>> --- a/arch/x86/include/asm/debugreg.h
>>>> +++ b/arch/x86/include/asm/debugreg.h
>>>> @@ -113,6 +113,31 @@ static inline void debug_stack_usage_inc
>>>> static inline void debug_stack_usage_dec(void) { }
>>>> #endif /* X86_64 */
>>>>
>>>> +static __always_inline void local_db_save(unsigned long *dr7)
>>>> +{
>>>> + get_debugreg(*dr7, 7);
>>>> + if (*dr7)
>>>> + set_debugreg(0, 7);
>>>
>>> %dr7 has an architecturally stuck bit in it.
>>>
>>> You want *dr7 != 0x400 to avoid writing 0 unconditionally.
>>
>> Do we have to have that bit set when writing it? Otherwise I might
>> actually prefer masking it out.
>
> I'm an idiot, we write a plain 9..
>
>>> Also, API wise, wouldn't it be nicer to write "dr7 = local_db_save()"
>>> rather than having a void function returning a single long via pointer?
>>
>> Probably.. I started with local_irq_save() and .. well, n/m. I'll change
>> it ;-)
>
> How's this?
>
> ---
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
> @@ -113,6 +113,36 @@ static inline void debug_stack_usage_inc
> static inline void debug_stack_usage_dec(void) { }
> #endif /* X86_64 */
>
> +static __always_inline unsigned long local_db_save(void)
> +{
> + unsigned long dr7;
> +
> + get_debugreg(&dr7, 7);
> + dr7 ^= 0x400;
Why xor? This seems extra confusing.
> + if (dr7)
> + 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();
> +
> + return dr7;
> +}
> +
> +static __always_inline void local_db_restore(unsigned long dr7)
> +{
> + /*
> + * Ensure the compiler doesn't raise this statement into
> + * the critical section; enabling breakpoints early would
> + * not be good.
> + */
> + barrier();
> + if (dr7)
> + set_debugreg(dr7, 7);
> +}
> +
> #ifdef CONFIG_CPU_SUP_AMD
> extern void set_dr_addr_mask(unsigned long mask, int dr);
> #else
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -727,15 +727,7 @@ static __always_inline void debug_enter(
> * Entry text is excluded for HW_BP_X and cpu_entry_area, which
> * includes the entry stack is excluded for everything.
> */
> - 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();
> + *dr7 = local_db_save();
>
> /*
> * The Intel SDM says:
> @@ -756,13 +748,7 @@ static __always_inline void debug_enter(
>
> 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);
> + local_db_restore(dr7);
> }
>
> /*
>
