Frederic Weisbecker <frede...@kernel.org> writes:
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c
> b/arch/powerpc/kernel/hw_breakpoint.c
> index 348cac9..fba6527 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -139,30 +139,31 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
> /*
> * Validate the arch-specific HW Breakpoint register settings
> */
> -int arch_validate_hwbkpt_settings(struct perf_event *bp)
> +int hw_breakpoint_arch_parse(struct perf_event *bp,
> + struct perf_event_attr *attr,
> + struct arch_hw_breakpoint *hw)
I think the semantics here are that we are reading from bp/attr and
writing to hw?
If so would some sprinkling of const on the first two parameters help
make that clearer?
> {
> int ret = -EINVAL, length_max;
> - struct arch_hw_breakpoint *info = counter_arch_bp(bp);
>
> if (!bp)
> return ret;
>
> - info->type = HW_BRK_TYPE_TRANSLATE;
> - if (bp->attr.bp_type & HW_BREAKPOINT_R)
> - info->type |= HW_BRK_TYPE_READ;
> - if (bp->attr.bp_type & HW_BREAKPOINT_W)
> - info->type |= HW_BRK_TYPE_WRITE;
> - if (info->type == HW_BRK_TYPE_TRANSLATE)
> + hw->type = HW_BRK_TYPE_TRANSLATE;
> + if (attr->bp_type & HW_BREAKPOINT_R)
> + hw->type |= HW_BRK_TYPE_READ;
> + if (attr->bp_type & HW_BREAKPOINT_W)
> + hw->type |= HW_BRK_TYPE_WRITE;
> + if (hw->type == HW_BRK_TYPE_TRANSLATE)
> /* must set alteast read or write */
> return ret;
> - if (!(bp->attr.exclude_user))
> - info->type |= HW_BRK_TYPE_USER;
> - if (!(bp->attr.exclude_kernel))
> - info->type |= HW_BRK_TYPE_KERNEL;
> - if (!(bp->attr.exclude_hv))
> - info->type |= HW_BRK_TYPE_HYP;
> - info->address = bp->attr.bp_addr;
> - info->len = bp->attr.bp_len;
> + if (!attr->exclude_user)
> + hw->type |= HW_BRK_TYPE_USER;
> + if (!attr->exclude_kernel)
> + hw->type |= HW_BRK_TYPE_KERNEL;
> + if (!attr->exclude_hv)
> + hw->type |= HW_BRK_TYPE_HYP;
> + hw->address = attr->bp_addr;
> + hw->len = attr->bp_len;
All looks right to me.
> /*
> * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8)
> @@ -176,12 +177,12 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
> if (cpu_has_feature(CPU_FTR_DAWR)) {
> length_max = 512 ; /* 64 doublewords */
> /* DAWR region can't cross 512 boundary */
> - if ((bp->attr.bp_addr >> 10) !=
> - ((bp->attr.bp_addr + bp->attr.bp_len - 1) >> 10))
> + if ((attr->bp_addr >> 10) !=
> + ((attr->bp_addr + attr->bp_len - 1) >> 10))
> return -EINVAL;
This will conflict with my next branch, but it should be easy enough to
resolve.
> }
> - if (info->len >
> - (length_max - (info->address & HW_BREAKPOINT_ALIGN)))
> + if (hw->len >
> + (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
> return -EINVAL;
> return 0;
> }
Acked-by: Michael Ellerman <m...@ellerman.id.au>
cheers
