[Patch 0/6] [Patch 0/6] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver VIII

[Patch 0/6] [Patch 0/6] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver VIII

[K.Prasad]

Hi David,
	I'm back with a new version of patches after a brief hiatus!

After much deliberation about modifying the code to change the timing of signal
delivery to user-space, it has been decided to retain the existing behaviour
i.e. SIGTRAP delivered to user-space after execution of causative instruction
although exception is raised before execution of it.

One-shot behaviour will now be restricted only to ptrace requests. Kernel-space
and non-ptrace user-space requests will result in persistent breakpoints.

Reasons
--------
- Signal delivery before execution of instruction requires complex workarounds
- One of the plausible workarounds is a two-pass hw-breakpoint handler which
  delivers the signal after the first pass (with the breakpoints enabled).
  In the second pass, it follows the existing semantics of
  disable_hbp-->enable_ss-->single_step-->disable_ss-->enable_hbp.
- Possibility of nested exceptions is a problem here.
- Proper identification of a  second-pass of first exception and a new nested
  exception is difficult. Possibility of stray exceptions due to accesses in
  neighbouring memory regions of the breakpoint address further complicates it.
- Alternatives are i)use one-shot for all user-space requests ii)disable signal
  delivery for non-ptrace requests, allow the user-defined callback routine to
  generate signal.
- Using one-shot for all user-space requests will break the register/unregister
  interface semantics.
- Disabling signal delivery for non-ptrace requests is one of the options
  but will be a digression from x86 behaviour, or would require changes in x86
  code too. Even user-defined callback routines cannot deliver signal
  before instruction execution.

Considering all the above, we propose a behaviour that delivers the signal to
user-space after breakpoint execution. In due course, it will be good to have
ptrace on PPC64 follow the same behaviour.

Changelog - ver VIII
-------------------
- Reverting changes to allow one-shot breakpoints only for ptrace requests.
- Minor changes in sanity checking in arch_validate_hwbkpt_settings().
- put_cpu_no_resched() is no longer available. Converted to put_cpu().

Thanks,
K.Prasad

Previous changelogs
-------------------
Changelog - ver VII
-------------------
- Allow the one-shot behaviour for exception handlers to be defined by the user.
  A new 'is_one_shot' flag is added to 'struct arch_hw_breakpoint'.

Changelog - ver VI
------------------
The task of identifying 'genuine' breakpoint exceptions from those caused by
'out-of-range' accesses turned out to be more tricky than originally thought.
Some changes to this effect were made in version IV of this patchset, but they
were not sufficient for user-space. Basically the breakpoint address received
through ptrace is always aligned to 8-bytes since ptrace receives an encoded
'data' (consisting of address | translation_enable | bkpt_type), and the size of
the symbol is not known. However for kernel-space addresses, the symbol-size can
be determined using kallsyms_lookup_size_offset() and this is used to check if
DAR (in the exception context) is
'bkpt_address <= DAR <= (bkpt_address + symbol_size)', failing which we conclude
it as a stray exception.

The following changes are made to enable check:
- Addition of a symbolsize field in 'struct arch_hw_breakpoint' field.
- Store the size of the 'watched' kernel symbol into 'symbolsize' field in
  arch_store_info(0 routine.
- Verify if the above described condition is true when is_one_shot is FALSE in
  hw_breakpoint_handler().

Changelog - ver V
------------------
- Breakpoint requests from ptrace (for user-space) are designed to be one-shot
in PPC64. The patch contains changes to retain this behaviour by returning early
in hw_breakpoint_handler() [without re-initialising DABR] and unregistering the
user-space request in ptrace_triggered(). It is safe to make a
unregister_user_hw_breakpoint() call from the breakpoint exception context
[through ptrace_triggered()] without giving rise to circular locking-dependancy.
This is because there can be no kernel code running on the CPU (which received
the exception) with the same spinlock held.

- Minor change in 'type' member of 'struct arch_hw_breakpoint' from u8 to 'int'.

Changelog - ver IV
------------------
- While DABR register requires double-word (8 bytes) aligned addresses, i.e.
the breakpoint is active over a range of 8 bytes, PPC64 allows byte-level
addressability. This may lead to stray exceptions which have to be ignored in
hw_breakpoint_handler(), when DAR != (Breakpoint request address). However DABR
will be populated with the requested breakpoint address aligned to the previous
double-word address. The code is now modified to store user-requested address
in 'bp->info.address' but update the DABR with a double-word aligned address.
- Please note that the Data Breakpoint facility in Xmon is broken as of 2.6.29
and the same has not been integrated into this facility as described in Ver I.

Changelog - ver III
------------------
- Patches are based on commit 08f16e060bf54bdc34f800ed8b5362cdeda75d8b of -tip
tree.
- The declarations in arch/powerpc/include/asm/hw_breakpoint.h are done only if
CONFIG_PPC64 is defined. This eliminates the need to conditionally include this
header file.
- load_debug_registers() is done in start_secondary() i.e. during CPU
initialisation.
- arch_check_va_<> routines in hw_breakpoint.c are now replaced with a much
simpler is_kernel_addr() check in arch_validate_hwbkpt_settings()
- Return code of hw_breakpoint_handler() when triggered due to Lazy debug
register switching is now changed to NOTIFY_STOP.
- The ptrace code no longer sets the TIF_DEBUG task flag as it is proposed to
be done in register_user_hw_breakpoint() routine.
- hw_breakpoint_handler() is now modified to use hbp_kernel_pos value to
  determine if the trigger was a user/kernel space address. The DAR register
  value is checked with the address stored in 'struct hw_breakpoint' to avoid
  handling of exceptions that belong to kprobe/Xmon.


Changelog - ver II
------------------
- Split the monolithic patch into six logical patches
- Changed the signature of arch_check_va_in_<user><kernel>space functions. They
  are now marked static.
- HB_NUM is now called as HBP_NUM (to preserve a consistent short-name
  convention)
- Introduced hw_breakpoint_disable() and changes to kexec code to disable
  breakpoints before a reboot.
- Minor changes in ptrace code to use macro-defined constants instead of
  numbers.
- Introduced a new constant definition INSTRUCTION_LEN in reg.h

Re: [Patch 0/6] [Patch 0/6] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver VIII

[David Gibson]

On Mon, Jul 27, 2009 at 05:41:52AM +0530, K.Prasad wrote:
> Hi David,
> 	I'm back with a new version of patches after a brief hiatus!
> 
> After much deliberation about modifying the code to change the timing of signal
> delivery to user-space, it has been decided to retain the existing behaviour
> i.e. SIGTRAP delivered to user-space after execution of causative instruction
> although exception is raised before execution of it.

Ok.  Except, presumably for ptrace, since changing that would break
gdb.

> One-shot behaviour will now be restricted only to ptrace
> requests. Kernel-space and non-ptrace user-space requests will
> result in persistent breakpoints.

Ok.

> Reasons
> --------
> - Signal delivery before execution of instruction requires complex workarounds
> - One of the plausible workarounds is a two-pass hw-breakpoint handler which
>   delivers the signal after the first pass (with the breakpoints enabled).
>   In the second pass, it follows the existing semantics of
>   disable_hbp-->enable_ss-->single_step-->disable_ss-->enable_hbp.

Yes, that's the only way I can see to do it.

> - Possibility of nested exceptions is a problem here.

Ok, why?

> - Proper identification of a  second-pass of first exception and a new nested
>   exception is difficult. Possibility of stray exceptions due to accesses in
>   neighbouring memory regions of the breakpoint address further complicates it.
> - Alternatives are i)use one-shot for all user-space requests ii)disable signal
>   delivery for non-ptrace requests, allow the user-defined callback routine to
>   generate signal.
> - Using one-shot for all user-space requests will break the register/unregister
>   interface semantics.
> - Disabling signal delivery for non-ptrace requests is one of the options
>   but will be a digression from x86 behaviour, or would require changes in x86
>   code too. Even user-defined callback routines cannot deliver signal
>   before instruction execution.
> 
> Considering all the above, we propose a behaviour that delivers the signal to
> user-space after breakpoint execution. In due course, it will be good to have
> ptrace on PPC64 follow the same behaviour.

Um.. except we can't change ptrace semantics in this way.  It could
break existing users.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Patch 0/6] [Patch 0/6] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver VIII

[Luis Machado]

Hi,

On Fri, 2009-07-31 at 16:10 +1000, David Gibson wrote:
> On Mon, Jul 27, 2009 at 05:41:52AM +0530, K.Prasad wrote:
> > Hi David,
> > 	I'm back with a new version of patches after a brief hiatus!
> > 
> > After much deliberation about modifying the code to change the timing of signal
> > delivery to user-space, it has been decided to retain the existing behaviour
> > i.e. SIGTRAP delivered to user-space after execution of causative instruction
> > although exception is raised before execution of it.
> 
> Ok.  Except, presumably for ptrace, since changing that would break
> gdb.

Yes. GDB works with some assumptions in mind. Hardware breakpoints
(they're not supported right now) trigger before execution, then GDB
just removes them and stepi's until we're past the breakpoint.

As for HW watchpoints, we always stop before execution (by ppc design
presumably). GDB will check the value before the trigger, remove the HW
watch, stepi it, and will check the value again to see if it has
changed.

If we're to trigger after the data load/store has happened, we could
have breakage in GDB.

Regards,
Luis

Re: [Patch 0/6] [Patch 0/6] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver VIII

[K.Prasad]

On Fri, Jul 31, 2009 at 04:10:13PM +1000, David Gibson wrote:
> On Mon, Jul 27, 2009 at 05:41:52AM +0530, K.Prasad wrote:

<edited>

> > Reasons
> > --------
> > - Signal delivery before execution of instruction requires complex workarounds
> > - One of the plausible workarounds is a two-pass hw-breakpoint handler which
> >   delivers the signal after the first pass (with the breakpoints enabled).
> >   In the second pass, it follows the existing semantics of
> >   disable_hbp-->enable_ss-->single_step-->disable_ss-->enable_hbp.
> 
> Yes, that's the only way I can see to do it.
> 
> > - Possibility of nested exceptions is a problem here.
> 
> Ok, why?
> 

Reason as described in the para below.

> > - Proper identification of a  second-pass of first exception and a new nested
> >   exception is difficult. Possibility of stray exceptions due to accesses in
> >   neighbouring memory regions of the breakpoint address further complicates it.

To elaborate, consider a case where a user-space address 'x' is
monitored for read or write, and the following happens (assume the
existence of the two-pass method for signal delivery).

- Instruction 'i' attempts to read/write in address 'x'
- hw-bkpt exception generated (pass I)
- Signal generated and hw-bkpt exception returns to user-space
- Signal is handled before 'i' is executed. Handler code reads/writes
  data in 'x' again. Generates nested exception.
- hw-breakpoint handler code is unable to distinguish if the new
  exception is from signal handler (nested) or due to second-pass (as
  per design above).

Thanks,
K.Prasad

Re: [Patch 0/6] [Patch 0/6] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver VIII

[David Gibson]

On Tue, Aug 04, 2009 at 02:23:16AM +0530, K.Prasad wrote:
> On Fri, Jul 31, 2009 at 04:10:13PM +1000, David Gibson wrote:
> > On Mon, Jul 27, 2009 at 05:41:52AM +0530, K.Prasad wrote:
> 
> <edited>
> 
> > > Reasons
> > > --------
> > > - Signal delivery before execution of instruction requires complex workarounds
> > > - One of the plausible workarounds is a two-pass hw-breakpoint handler which
> > >   delivers the signal after the first pass (with the breakpoints enabled).
> > >   In the second pass, it follows the existing semantics of
> > >   disable_hbp-->enable_ss-->single_step-->disable_ss-->enable_hbp.
> > 
> > Yes, that's the only way I can see to do it.
> > 
> > > - Possibility of nested exceptions is a problem here.
> > 
> > Ok, why?
> > 
> 
> Reason as described in the para below.
> 
> > > - Proper identification of a  second-pass of first exception and a new nested
> > >   exception is difficult. Possibility of stray exceptions due to accesses in
> > >   neighbouring memory regions of the breakpoint address further complicates it.
> 
> To elaborate, consider a case where a user-space address 'x' is
> monitored for read or write, and the following happens (assume the
> existence of the two-pass method for signal delivery).
> 
> - Instruction 'i' attempts to read/write in address 'x'
> - hw-bkpt exception generated (pass I)
> - Signal generated and hw-bkpt exception returns to user-space
> - Signal is handled before 'i' is executed. Handler code reads/writes
>   data in 'x' again. Generates nested exception.
> - hw-breakpoint handler code is unable to distinguish if the new
>   exception is from signal handler (nested) or due to second-pass (as
>   per design above).

Ah, ok, I understand now.  Hrm.  I'll have to think about this.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson