Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Andy Lutomirski <luto@amacapital.net>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()
Date:   Fri, 29 May 2020 10:28:33 -0700
Message-Id: <AC94E789-9EC7-4156-955C-841FF7114176@amacapital.net>
References: <20200528213352.GC4496@worktop.programming.kicks-ass.net>
Cc:     Andrew Cooper <andrew.cooper3@citrix.com>, tglx@linutronix.de,
        linux-kernel@vger.kernel.org, x86@kernel.org,
        Lai Jiangshan <laijs@linux.alibaba.com>,
        sean.j.christopherson@intel.com, daniel.thompson@linaro.org
In-Reply-To: <20200528213352.GC4496@worktop.programming.kicks-ass.net>
To:     Peter Zijlstra <peterz@infradead.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On May 28, 2020, at 2:34 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>=20
> =EF=BB=BFOn 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 */
>>>>=20
>>>> +static __always_inline void local_db_save(unsigned long *dr7)
>>>> +{
>>>> +    get_debugreg(*dr7, 7);
>>>> +    if (*dr7)
>>>> +        set_debugreg(0, 7);
>>>=20
>>> %dr7 has an architecturally stuck bit in it.
>>>=20
>>> You want *dr7 !=3D 0x400 to avoid writing 0 unconditionally.
>>=20
>> Do we have to have that bit set when writing it? Otherwise I might
>> actually prefer masking it out.
>=20
> I'm an idiot, we write a plain 9..
>=20
>>> Also, API wise, wouldn't it be nicer to write "dr7 =3D local_db_save()"
>>> rather than having a void function returning a single long via pointer?
>>=20
>> Probably.. I started with local_irq_save() and .. well, n/m. I'll change
>> it ;-)
>=20
> How's this?
>=20
> ---
> --- 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 */
>=20
> +static __always_inline unsigned long local_db_save(void)
> +{
> +    unsigned long dr7;
> +
> +    get_debugreg(&dr7, 7);
> +    dr7 ^=3D 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 =3D local_db_save();
>=20
>    /*
>     * The Intel SDM says:
> @@ -756,13 +748,7 @@ static __always_inline void debug_enter(
>=20
> 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);
> }
>=20
> /*
>=20