lkml.org 
[lkml]   [2020]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH v3 2/9] powerpc/watchpoint: Fix DAWR exception constraint
    On Wed, Jul 8, 2020 at 2:52 PM Ravi Bangoria
    <ravi.bangoria@linux.ibm.com> wrote:
    >
    > Pedro Miraglia Franco de Carvalho noticed that on p8, DAR value is
    > inconsistent with different type of load/store. Like for byte,word
    > etc. load/stores, DAR is set to the address of the first byte of
    > overlap between watch range and real access. But for quadword load/
    > store it's set to the address of the first byte of real access. This
    > issue has been fixed in p10. In p10(ISA 3.1), DAR is always set to
    > the address of the first byte of overlap. Commit 27985b2a640e
    > ("powerpc/watchpoint: Don't ignore extraneous exceptions blindly")
    > wrongly assumes that DAR is set to the address of the first byte of
    > overlap for all load/stores on p8 as well. Fix that. With the fix,
    > we now rely on 'ea' provided by analyse_instr(). If analyse_instr()
    > fails, generate event unconditionally on p8, and on p10 generate
    > event only if DAR is within a DAWR range.
    >
    > Note: 8xx is not affected.
    >
    > Fixes: 27985b2a640e ("powerpc/watchpoint: Don't ignore extraneous exceptions blindly")
    > Fixes: 74c6881019b7 ("powerpc/watchpoint: Prepare handler to handle more than one watchpoint")
    > Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@br.ibm.com>
    > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
    > ---
    > arch/powerpc/kernel/hw_breakpoint.c | 93 +++++++++++++++++++----------
    > 1 file changed, 63 insertions(+), 30 deletions(-)
    >
    > diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
    > index 031e6defc08e..7a66c370a105 100644
    > --- a/arch/powerpc/kernel/hw_breakpoint.c
    > +++ b/arch/powerpc/kernel/hw_breakpoint.c
    > @@ -498,11 +498,11 @@ static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info
    > return ((info->address <= dar) && (dar - info->address < info->len));
    > }
    >
    > -static bool dar_user_range_overlaps(unsigned long dar, int size,
    > - struct arch_hw_breakpoint *info)
    > +static bool ea_user_range_overlaps(unsigned long ea, int size,
    > + struct arch_hw_breakpoint *info)
    > {
    > - return ((dar < info->address + info->len) &&
    > - (dar + size > info->address));
    > + return ((ea < info->address + info->len) &&
    > + (ea + size > info->address));
    > }
    >
    > static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
    > @@ -515,20 +515,22 @@ static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
    > return ((hw_start_addr <= dar) && (hw_end_addr > dar));
    > }
    >
    > -static bool dar_hw_range_overlaps(unsigned long dar, int size,
    > - struct arch_hw_breakpoint *info)
    > +static bool ea_hw_range_overlaps(unsigned long ea, int size,
    > + struct arch_hw_breakpoint *info)
    > {
    > unsigned long hw_start_addr, hw_end_addr;
    >
    > hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
    > hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
    >
    > - return ((dar < hw_end_addr) && (dar + size > hw_start_addr));
    > + return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
    > }
    >
    > /*
    > * If hw has multiple DAWR registers, we also need to check all
    > * dawrx constraint bits to confirm this is _really_ a valid event.
    > + * If type is UNKNOWN, but privilege level matches, consider it as
    > + * a positive match.
    > */
    > static bool check_dawrx_constraints(struct pt_regs *regs, int type,
    > struct arch_hw_breakpoint *info)
    > @@ -536,7 +538,12 @@ static bool check_dawrx_constraints(struct pt_regs *regs, int type,
    > if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
    > return false;
    >
    > - if (OP_IS_STORE(type) && !(info->type & HW_BRK_TYPE_WRITE))
    > + /*
    > + * The Cache Management instructions other than dcbz never
    > + * cause a match. i.e. if type is CACHEOP, the instruction
    > + * is dcbz, and dcbz is treated as Store.
    > + */
    > + if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE))
    > return false;
    This change seems seperate to this commit?
    >
    > if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
    > @@ -553,7 +560,8 @@ static bool check_dawrx_constraints(struct pt_regs *regs, int type,
    > * including extraneous exception. Otherwise return false.
    > */
    > static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
    > - int type, int size, struct arch_hw_breakpoint *info)
    > + unsigned long ea, int type, int size,
    > + struct arch_hw_breakpoint *info)
    > {
    > bool in_user_range = dar_in_user_range(regs->dar, info);
    > bool dawrx_constraints;
    > @@ -569,11 +577,10 @@ static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
    > }
    >
    > if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) {
    > - if (in_user_range)
    > - return true;
    > -
    > - if (dar_in_hw_range(regs->dar, info)) {
    > - info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
    > + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
    > + if (dar_in_hw_range(regs->dar, info))
    > + return true;
    > + } else {
    > return true;
    I think this would be clearer as:
    if (cpu_has_feature(CPU_FTR_ARCH_31) &&
    !(dar_in_hw_range(regs->dar, info)))
    return false;
    else
    return true;

    > }
    > return false;
    > @@ -581,10 +588,20 @@ static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
    >
    > dawrx_constraints = check_dawrx_constraints(regs, type, info);
    >
    > - if (dar_user_range_overlaps(regs->dar, size, info))
    > + if (type == UNKNOWN) {
    > + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
    > + if (dar_in_hw_range(regs->dar, info))
    > + return dawrx_constraints;
    > + } else {
    > + return dawrx_constraints;
    > + }
    > + return false;
    > + }
    Similar thing here, it could be:
    if ((cpu_has_feature(CPU_FTR_ARCH_31)) &&
    !(dar_in_hw_range(regs->dar, info)))
    return false;
    else
    return dawrx_constraints;
    > +
    > + if (ea_user_range_overlaps(ea, size, info))
    > return dawrx_constraints;
    >
    > - if (dar_hw_range_overlaps(regs->dar, size, info)) {
    > + if (ea_hw_range_overlaps(ea, size, info)) {
    > if (dawrx_constraints) {
    > info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
    > return true;
    > @@ -593,8 +610,17 @@ static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
    > return false;
    > }
    >
    > +static int cache_op_size(void)
    > +{
    > +#ifdef __powerpc64__
    > + return ppc64_caches.l1d.block_size;
    > +#else
    > + return L1_CACHE_BYTES;
    > +#endif
    > +}
    > +
    > static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
    > - int *type, int *size, bool *larx_stcx)
    > + int *type, int *size, unsigned long *ea)
    > {
    > struct instruction_op op;
    >
    > @@ -602,16 +628,23 @@ static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
    > return;
    >
    > analyse_instr(&op, regs, *instr);
    > -
    > - /*
    > - * Set size = 8 if analyse_instr() fails. If it's a userspace
    > - * watchpoint(valid or extraneous), we can notify user about it.
    > - * If it's a kernel watchpoint, instruction emulation will fail
    > - * in stepping_handler() and watchpoint will be disabled.
    > - */
    > *type = GETTYPE(op.type);
    > - *size = !(*type == UNKNOWN) ? GETSIZE(op.type) : 8;
    > - *larx_stcx = (*type == LARX || *type == STCX);
    > + *ea = op.ea;
    > +#ifdef __powerpc64__
    > + if (!(regs->msr & MSR_64BIT))
    > + *ea &= 0xffffffffUL;
    > +#endif
    > +
    > + *size = GETSIZE(op.type);
    > + if (*type == CACHEOP) {
    > + *size = cache_op_size();
    > + *ea &= ~(*size - 1);
    > + }
    Again related to CACHEOP, should these changes be mentioned in the
    commit message?
    > +}
    > +
    > +static bool is_larx_stcx_instr(int type)
    > +{
    > + return type == LARX || type == STCX;
    > }
    >
    > /*
    > @@ -678,7 +711,7 @@ int hw_breakpoint_handler(struct die_args *args)
    > struct ppc_inst instr = ppc_inst(0);
    > int type = 0;
    > int size = 0;
    > - bool larx_stcx = false;
    > + unsigned long ea;
    >
    > /* Disable breakpoints during exception handling */
    > hw_breakpoint_disable();
    > @@ -692,7 +725,7 @@ int hw_breakpoint_handler(struct die_args *args)
    > rcu_read_lock();
    >
    > if (!IS_ENABLED(CONFIG_PPC_8xx))
    > - get_instr_detail(regs, &instr, &type, &size, &larx_stcx);
    > + get_instr_detail(regs, &instr, &type, &size, &ea);
    >
    > for (i = 0; i < nr_wp_slots(); i++) {
    > bp[i] = __this_cpu_read(bp_per_reg[i]);
    > @@ -702,7 +735,7 @@ int hw_breakpoint_handler(struct die_args *args)
    > info[i] = counter_arch_bp(bp[i]);
    > info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
    >
    > - if (check_constraints(regs, instr, type, size, info[i])) {
    > + if (check_constraints(regs, instr, ea, type, size, info[i])) {
    > if (!IS_ENABLED(CONFIG_PPC_8xx) &&
    > ppc_inst_equal(instr, ppc_inst(0))) {
    > handler_error(bp[i], info[i]);
    > @@ -744,7 +777,7 @@ int hw_breakpoint_handler(struct die_args *args)
    > }
    >
    > if (!IS_ENABLED(CONFIG_PPC_8xx)) {
    > - if (larx_stcx) {
    > + if (is_larx_stcx_instr(type)) {
    > for (i = 0; i < nr_wp_slots(); i++) {
    > if (!hit[i])
    > continue;
    > --
    > 2.26.2
    >

    \
     
     \ /
      Last update: 2020-07-15 04:20    [W:3.100 / U:0.364 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site