Hi Catalin, On Sun, Oct 30, 2016 at 7:39 PM, Catalin Marinas <catalin.mari...@arm.com> wrote: > On Tue, Sep 27, 2016 at 01:18:00PM +0530, Pratyush Anand wrote: >> --- /dev/null >> +++ b/arch/arm64/kernel/probes/uprobes.c >> @@ -0,0 +1,221 @@ >> +/* >> + * Copyright (C) 2014-2016 Pratyush Anand <pan...@redhat.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> +#include <linux/highmem.h> >> +#include <linux/ptrace.h> >> +#include <linux/uprobes.h> >> +#include <asm/cacheflush.h> >> + >> +#include "decode-insn.h" >> + >> +#define UPROBE_INV_FAULT_CODE UINT_MAX >> + >> +bool is_trap_insn(uprobe_opcode_t *insn) >> +{ >> + return false; >> +} > > On the previous series, I had a comment left unanswered with regards to > always returning false in is_trap_insn(): > > Looking at handle_swbp(), if we hit a breakpoint for which we don't have > a valid uprobe, this function currently sends a SIGTRAP. But if > is_trap_insn() returns false always, is_trap_at_addr() would return 0 in > this case so the SIGTRAP is never issued.
A agreed on this that the older implementation i.e. the default one of is_trap_insn() is better for the time being. probably 'strtle r0, [r0], #160' would have the closest matching aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too seems a bad instruction. So, there might not be any aarch32 instruction which will match to uprobe BRK instruction. Therefore, if I send a V3 by removing aacrh64 Hi Catalin, On Sun, Oct 30, 2016 at 7:39 PM, Catalin Marinas <catalin.mari...@arm.com> wrote: > On Tue, Sep 27, 2016 at 01:18:00PM +0530, Pratyush Anand wrote: >> --- /dev/null >> +++ b/arch/arm64/kernel/probes/uprobes.c >> @@ -0,0 +1,221 @@ >> +/* >> + * Copyright (C) 2014-2016 Pratyush Anand <pan...@redhat.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> +#include <linux/highmem.h> >> +#include <linux/ptrace.h> >> +#include <linux/uprobes.h> >> +#include <asm/cacheflush.h> >> + >> +#include "decode-insn.h" >> + >> +#define UPROBE_INV_FAULT_CODE UINT_MAX >> + >> +bool is_trap_insn(uprobe_opcode_t *insn) >> +{ >> + return false; >> +} > > On the previous series, I had a comment left unanswered with regards to > always returning false in is_trap_insn(): > > Looking at handle_swbp(), if we hit a breakpoint for which we don't have > a valid uprobe, this function currently sends a SIGTRAP. But if > is_trap_insn() returns false always, is_trap_at_addr() would return 0 in > this case so the SIGTRAP is never issued. A agreed on this that the older implementation i.e. the default one of is_trap_insn() is better for the time being. probably 'strtle r0, [r0], #160' would have the closest matching aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too seems a bad instruction. So, there might not be any aarch32 instruction which will match to uprobe BRK instruction. Therefore, if I send a V3 by removing aacrh64 Hi Catalin, On Sun, Oct 30, 2016 at 7:39 PM, Catalin Marinas <catalin.mari...@arm.com> wrote: > On Tue, Sep 27, 2016 at 01:18:00PM +0530, Pratyush Anand wrote: >> --- /dev/null >> +++ b/arch/arm64/kernel/probes/uprobes.c >> @@ -0,0 +1,221 @@ >> +/* >> + * Copyright (C) 2014-2016 Pratyush Anand <pan...@redhat.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> +#include <linux/highmem.h> >> +#include <linux/ptrace.h> >> +#include <linux/uprobes.h> >> +#include <asm/cacheflush.h> >> + >> +#include "decode-insn.h" >> + >> +#define UPROBE_INV_FAULT_CODE UINT_MAX >> + >> +bool is_trap_insn(uprobe_opcode_t *insn) >> +{ >> + return false; >> +} > > On the previous series, I had a comment left unanswered with regards to > always returning false in is_trap_insn(): > > Looking at handle_swbp(), if we hit a breakpoint for which we don't have > a valid uprobe, this function currently sends a SIGTRAP. But if > is_trap_insn() returns false always, is_trap_at_addr() would return 0 in > this case so the SIGTRAP is never issued. A agreed on this that the older implementation i.e. the default one of is_trap_insn() is better for the time being. I sent out V2 before your last comment on it in V1 :(. probably 'strtle r0, [r0], #160' would have the closest matching aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too seems a bad aarch32 instruction. So, there might not be any aarch32 instruction which will match to uprobe BRK instruction. Therefore, if I send a V3 by removing aacrh64 is_trap_insn(), would that be acceptable, or do you see any other issue with this patch series? If there is anything else, I would address that in V3 as well. Thanks for your review. ~Pratyush