From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S946639AbcJaUco (ORCPT ); Mon, 31 Oct 2016 16:32:44 -0400 Received: from foss.arm.com ([217.140.101.70]:42836 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S946497AbcJaUcm (ORCPT ); Mon, 31 Oct 2016 16:32:42 -0400 Date: Mon, 31 Oct 2016 14:32:37 -0600 From: Catalin Marinas To: Pratyush Anand Cc: Russell King - ARM Linux , srikar@linux.vnet.ibm.com, vijaya.kumar@caviumnetworks.com, Will Deacon , Oleg Nesterov , open list , David Long , Steve Capper , William Cohen , linux-arm-kernel Subject: Re: [PATCH V2 6/6] arm64: Add uprobe support Message-ID: <20161031203237.lof7uzt4ye57y7h3@localhost> References: <41cc70043792e65cbc3b4cc4ad7fbf6379afa550.1474960629.git.panand@redhat.com> <20161030140901.h6mdxbb4pgqpqs7j@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Pratyush, On Mon, Oct 31, 2016 at 02:10:43PM +0530, Pratyush Anand wrote: > On Sun, Oct 30, 2016 at 7:39 PM, Catalin Marinas > 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 > >> + * > >> + * 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 > >> +#include > >> +#include > >> +#include > >> + > >> +#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 :(. Thinking some more about this, the default is_trap_insn() still seems better. It may return true occasionally for 32-bit tasks but we don't care anyway because the subsequent arch_uprobe_analyze_insn() would prevent the installation of the uprobe. However, always returning false in is_trap_insn() would confuse handle_swbp() if you install uprobes in an already debugged task. > 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. As I said above, even if it matches, we don't support uprobes for 32-bit (caught by the subsequent test). > 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. I think I have one minor comment on arch_uprobe_analyze_insn() and v3 should look ok. -- Catalin