Re: [PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()
On Fri, May 29, 2020 at 10:28:33AM -0700, Andy Lutomirski wrote:
> > +static __always_inline unsigned long local_db_save(void)
> > +{
> > +unsigned long dr7;
> > +
> > +get_debugreg(, 7);
> > +dr7 ^= 0x400;
>
> Why xor? This seems extra confusing.
I'll do the normal mask thing ..
Re: [PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()
> On May 28, 2020, at 2:34 PM, Peter Zijlstra 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(, 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);
> }
>
> /*
>
Re: [PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()
On 28/05/2020 22:15, 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.
Not currently. I guess it depends on how likely %dr7 is to gain an
inverted polarity bit like %dr6 did.
~Andrew
Re: [PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()
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(, 7);
+ dr7 ^= 0x400;
+ 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);
}
/*
Re: [PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()
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.
> 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 ;-)
Re: [PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()
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.
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?
~Andrew
[PATCH 1/6] x86/entry: Introduce local_db_{save,restore}()
In order to allow other exceptions than #DB to disable breakpoints,
provide a common helper.
Signed-off-by: Peter Zijlstra (Intel)
---
arch/x86/include/asm/debugreg.h | 25 +
arch/x86/kernel/traps.c | 18 ++
2 files changed, 27 insertions(+), 16 deletions(-)
--- 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);
+ /*
+* Ensure the compiler doesn't lower the above statements into
+* the critical section; disabling breakpoints late would not
+* be good.
+*/
+ barrier();
+}
+
+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();
+ local_db_save(dr7);
/*
* 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);
}
/*
