Re: [RFC:PATCH 01/03] powerpc: Extended ptrace interface

David Gibson
2009-12-11T00:44:34Z

On Thu, Dec 10, 2009 at 01:57:15PM -0200, Dave Kleikamp wrote:
> powerpc: Extended ptrace interface
>
> From: Torez Smith <lnxtorez@linux.vnet.ibm.com>
>
> Add a new extended ptrace interface so that user-space has a single
> interface for powerpc, without having to know the specific layout
> of the debug registers.

> Implement:
> PPC_PTRACE_GETHWDEBUGINFO
> PPC_PTRACE_SETHWDEBUG
> PPC_PTRACE_DELHWDEBUG
>
> Signed-off-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> Signed-off-by: Torez Smith <lnxtorez@linux.vnet.ibm.com>

Apart from the data breakpoint alignment for 32-bit systems, all the
comments below are trivial nits, so:

Acked-by: David Gibson <dwg@au1.ibm.com>

[snip]
> +/*
> + * Trigger Type
> + */
> +#define PPC_BREAKPOINT_TRIGGER_EXECUTE 0x1
> +#define PPC_BREAKPOINT_TRIGGER_READ 0x2
> +#define PPC_BREAKPOINT_TRIGGER_WRITE 0x4
> +#define PPC_BREAKPOINT_TRIGGER_RW 0x6

For a little extra safety, I'd tend towards defining the RW constant
in terms of the READ and WRITE constants.

> +
> +/*
> + * Address Mode
> + */
> +#define PPC_BREAKPOINT_MODE_EXACT 0x0
> +#define PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE 0x1
> +#define PPC_BREAKPOINT_MODE_RANGE_EXCLUSIVE 0x2
> +#define PPC_BREAKPOINT_MODE_MASK 0x3
> +
> +/*
> + * Condition Mode
> + */
> +#define PPC_BREAKPOINT_CONDITION_NONE 0x0
> +#define PPC_BREAKPOINT_CONDITION_AND 0x1
> +#define PPC_BREAKPOINT_CONDITION_EXACT 0x1

And likewuse define EXACT in terms of AND.

> +#define PPC_BREAKPOINT_CONDITION_OR 0x2
> +#define PPC_BREAKPOINT_CONDITION_AND_OR 0x3
> +#define PPC_BREAKPOINT_CONDITION_BE_ALL 0x00ff0000
> +#define PPC_BREAKPOINT_CONDITION_BE_SHIFT 16
> +#define PPC_BREAKPOINT_CONDITION_BE(n) \
> + (1<<((n)+PPC_BREAKPOINT_CONDITION_BE_SHIFT))

[snip]
> + case PPC_PTRACE_GETHWDBGINFO: {
> + struct ppc_debug_info dbginfo;
> +
> + dbginfo.version = 1;
> + dbginfo.num_instruction_bps = 0;
> + dbginfo.num_data_bps = 1;
> + dbginfo.num_condition_regs = 0;
> +#ifdef CONFIG_PPC64
> + dbginfo.data_bp_alignment = 8;
> +#else
> + dbginfo.data_bp_alignment = 0;

Uh.. this looks wrong. Surely it should be 4.

--
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_!