Re: [PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()

[Andy Lutomirski]

> On May 28, 2020, at 2:34 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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);
> }
> 
> /*
>