From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.2 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DB882C433C1 for ; Wed, 24 Mar 2021 03:39:42 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 42B3A619D3 for ; Wed, 24 Mar 2021 03:39:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 42B3A619D3 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:Cc:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=+bMAH7YorMwfI8XeEeo8cVjH3s55oCszDjuIr3PWUag=; b=kw/CzzMNtwFPbvXrjGItXmMf3 m/sg97Q2iszQIuRwmHajtb4Shl/uCYM/Ueoi6N36ZsLH+clqcgpx6XiLJfRH4sAySIzrOteYam3s9 Zk8vO70w4rVdcF5ETafYSSyDq9GW+0r6Es1FH5oY+skrohstYhxG6GQslIiUB8LjtjTtqideWgneU boXFm6Le5ZpwvzAYdbx+g1tWxCn4dxBXtI3yZB7v44aNakc+wZVEvVVsaMrWaBNXGIg5ohAZRGmD+ 7FeiqIO8yA98bRJE9o58GDVw7lPisphHfLCcKjR+HdLzF1MF+w6IxkPIgmwHDHQiC+ckByZtfU4EC SlLfKpNuw==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lOuLC-00GGI3-6n; Wed, 24 Mar 2021 03:37:54 +0000 Received: from mail-pf1-x42f.google.com ([2607:f8b0:4864:20::42f]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lOuL6-00GGHU-Uj for linux-arm-kernel@lists.infradead.org; Wed, 24 Mar 2021 03:37:50 +0000 Received: by mail-pf1-x42f.google.com with SMTP id y5so16363589pfn.1 for ; Tue, 23 Mar 2021 20:37:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Scpkc+99pBAZyf4X9FUTLeY4be6wnkguK+kKt2gu3AU=; b=R0oAaGGB2zuALYZoBV42JTPvZSumUY94V8pqfE3xmU6UE3La3aXsWT4W9EZEEgjSNn 65ZFNW/QJeu/1FqXikFDhluL2l3MPSwULNWhlKU68GGQToh1mtZD2teHmXzXCAAuD25k Sa2Mk9n4ur1sw9Gxrmgy7DbT49rImrV0kbNkb9RYtahzQiiU0tOGCo9S99gbKa1gj8yN 9ubRxjAyAaJpL6lsV7DXWij/KuT3J5zApZ326Ev474x1NlRsjGEtEc7oIHeqNE16mFqe Tdyq5Om0vzjQClzJ/DiGw5clksoarIDXc/15lFGztVXUvseyqjSljyMhPFWWeXYPhxfU 0T4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Scpkc+99pBAZyf4X9FUTLeY4be6wnkguK+kKt2gu3AU=; b=PvEYo36tLWl9CRivwh1gP3wqES49YxWdOSo7jTdOnnwBE5Lje+1HvFwpzwmfA+gwpy im3hXsygsjM4NaoFfqTmzdCBCkd4P6zeUE/YbxR44c+YjcP/SsJIKTllbZLHboeImx4d e9IsUs6NEOrAtFYiwQ54b2x9hrbNpw6v6FVIIrAOOhh7EUXzy77vs8gtnqwy7fYTNOfZ eEXYSdhr8lGjYDB4bBKhAoeYunFuv/1iPSgpR84aV8BQreNss+FgQTsO3joXXd0m9AbK RJkTM5dvSkV5pcXAZnyELCa1EPGhz0soImRW4IkUeiD1vrgQGL/uchbtO+o04s9e7J+8 D7gw== X-Gm-Message-State: AOAM533t0xm2ZmQM9+rFAVA6LxGuGhA0MS3/dTghJpN1pouIsDaPz/Og REZIA8JjtaHlddf4nZQBrHs= X-Google-Smtp-Source: ABdhPJzURV/2ocdWj0YF97qRlk1QKMnQiKE4Qa4uq7KwlHJzXdZghRkhzBSK24VKlPA36ZaGkmNYGg== X-Received: by 2002:a63:4d0a:: with SMTP id a10mr1248047pgb.177.1616557066856; Tue, 23 Mar 2021 20:37:46 -0700 (PDT) Received: from [10.230.29.202] ([192.19.223.252]) by smtp.gmail.com with ESMTPSA id z2sm569936pfq.198.2021.03.23.20.37.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 23 Mar 2021 20:37:46 -0700 (PDT) Subject: Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support To: Qais Yousef , Alexander Sverdlin Cc: Steven Rostedt , Ingo Molnar , Russell King , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Ard Biesheuvel , Linus Walleij References: <3eecf51d-b189-9e8b-f19d-a49d0764aae5@nokia.com> <05608bc8-f44d-5f91-15ab-af00c59b53e6@gmail.com> <20210312172401.36awjh4hmj4cs6ot@e107158-lin.cambridge.arm.com> <134e1a2c-daac-7b00-c170-bcca434d08df@gmail.com> <20210314220217.4mexdide7sqjfved@e107158-lin> <20210321190611.d6a3hbqabts3qq5v@e107158-lin> <20210322110106.2bed3d50@gandalf.local.home> <20210322163248.id7qplbk6och6kuw@e107158-lin> <504d72ec-70a6-7e50-dbbb-16d693ce6150@nokia.com> <20210323222230.2d63hdcxq6strbug@e107158-lin> From: Florian Fainelli Message-ID: <2404ff10-7acc-3946-6592-31508f257f33@gmail.com> Date: Tue, 23 Mar 2021 20:37:44 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <20210323222230.2d63hdcxq6strbug@e107158-lin> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210324_033749_121168_8D63C1DF X-CRM114-Status: GOOD ( 32.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Qais, On 3/23/2021 3:22 PM, Qais Yousef wrote: > Hi Alexander > > On 03/22/21 18:02, Alexander Sverdlin wrote: >> Hi Qais, >> >> On 22/03/2021 17:32, Qais Yousef wrote: >>> Yes you're right. I was a bit optimistic on CONFIG_DYNAMIC_FTRACE will imply >>> CONFIG_ARM_MODULE_PLTS is enabled too. >>> >>> It only has an impact on reducing ifdefery when calling >>> >>> ftrace_call_replace_mod(rec->arch.mod, ...) >>> >>> Should be easy to wrap rec->arch.mod with its own accessor that will return >>> NULL if !CONFIG_ARM_MODULE_PLTS or just ifdef the functions. >>> >>> Up to Alexander to pick what he prefers :-) >> >> well, I of course prefer v7 as-is, because this review is running longer than two >> years and I actually hope these patches to be finally merged at some point. >> But you are welcome to optimize them with follow up patches :) > > I appreciate that and thanks a lot for your effort. My attempt to review and > test here is to help in getting this merged. > > FWIW my main concern is about duplicating the range check in > ftrace_call_replace() and using magic values that already exist in > __arm_gen_branch_{arm, thumb2}() and better remain encapsulated there. Your patch in addition to Alexander's patch work for me as well, so feel free to add a: Tested-by: Florian Fainelli FWIW, what is nice about Alexander's original patch is that it applies relatively cleanly to older kernels as well where this is equally needed. There is not currently any Fixes: tag being provided but maybe we should amend the second patch with one? Thanks! > > Thanks > > -- > Qais Yousef > > ----->8------ > > > diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h > index a4dbac07e4ef..8545b3ff8317 100644 > --- a/arch/arm/include/asm/ftrace.h > +++ b/arch/arm/include/asm/ftrace.h > @@ -25,6 +25,27 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) > /* With Thumb-2, the recorded addresses have the lsb set */ > return addr & ~1; > } > + > +#ifdef CONFIG_ARM_MODULE_PLTS > +static inline void ftrace_set_mod(struct dyn_arch_ftrace *arch, struct module *mod) > +{ > + arch->mod = mod; > +} > + > +static inline struct module *ftrace_get_mod(struct dyn_arch_ftrace *arch) > +{ > + return arch->mod; > +} > +#else > +static inline void ftrace_set_mod(struct dyn_arch_ftrace *arch, struct module *mod) > +{ > +} > + > +static inline struct module *ftrace_get_mod(struct dyn_arch_ftrace *arch) > +{ > + return NULL; > +} > +#endif > #endif > > #endif > diff --git a/arch/arm/include/asm/insn.h b/arch/arm/include/asm/insn.h > index f20e08ac85ae..71c3edefe629 100644 > --- a/arch/arm/include/asm/insn.h > +++ b/arch/arm/include/asm/insn.h > @@ -13,18 +13,24 @@ arm_gen_nop(void) > } > > unsigned long > -__arm_gen_branch(unsigned long pc, unsigned long addr, bool link); > +__arm_gen_branch(unsigned long pc, unsigned long addr, bool link, bool check); > > static inline unsigned long > arm_gen_branch(unsigned long pc, unsigned long addr) > { > - return __arm_gen_branch(pc, addr, false); > + return __arm_gen_branch(pc, addr, false, true); > } > > static inline unsigned long > arm_gen_branch_link(unsigned long pc, unsigned long addr) > { > - return __arm_gen_branch(pc, addr, true); > + return __arm_gen_branch(pc, addr, true, true); > +} > + > +static inline unsigned long > +arm_gen_branch_link_nocheck(unsigned long pc, unsigned long addr) > +{ > + return __arm_gen_branch(pc, addr, true, false); > } > > #endif > diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c > index fa867a57100f..63ea34edd222 100644 > --- a/arch/arm/kernel/ftrace.c > +++ b/arch/arm/kernel/ftrace.c > @@ -70,20 +70,28 @@ int ftrace_arch_code_modify_post_process(void) > > static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr) > { > - s32 offset = addr - pc; > - s32 blim = 0xfe000008; > - s32 flim = 0x02000004; > + return arm_gen_branch_link(pc, addr); > +} > > - if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) { > - blim = 0xff000004; > - flim = 0x01000002; > - } > +static unsigned long > +ftrace_call_replace_mod(struct module *mod, unsigned long pc, unsigned long addr) > +{ > +#ifdef CONFIG_ARM_MODULE_PLTS > + unsigned long new; > > - if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS) && > - (offset < blim || offset > flim)) > - return 0; > + if (likely(!mod)) > + return arm_gen_branch_link(pc, addr); > > + new = arm_gen_branch_link_nocheck(pc, addr); > + if (!new) { > + addr = get_module_plt(mod, pc, addr); > + new = arm_gen_branch_link(pc, addr); > + } > + > + return new; > +#else > return arm_gen_branch_link(pc, addr); > +#endif > } > > static int ftrace_modify_code(unsigned long pc, unsigned long old, > @@ -141,18 +149,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) > > old = ftrace_nop_replace(rec); > > - new = ftrace_call_replace(ip, aaddr); > - > -#ifdef CONFIG_ARM_MODULE_PLTS > - if (!new) { > - struct module *mod = rec->arch.mod; > - > - if (mod) { > - aaddr = get_module_plt(mod, ip, aaddr); > - new = ftrace_call_replace(ip, aaddr); > - } > - } > -#endif > + new = ftrace_call_replace_mod(ftrace_get_mod(&rec->arch), ip, aaddr); > > return ftrace_modify_code(rec->ip, old, new, true); > } > @@ -183,23 +180,11 @@ int ftrace_make_nop(struct module *mod, > unsigned long new; > int ret; > > -#ifdef CONFIG_ARM_MODULE_PLTS > /* mod is only supplied during module loading */ > - if (!mod) > - mod = rec->arch.mod; > - else > - rec->arch.mod = mod; > -#endif > - > - old = ftrace_call_replace(ip, aaddr); > - > -#ifdef CONFIG_ARM_MODULE_PLTS > - if (!old && mod) { > - aaddr = get_module_plt(mod, ip, aaddr); > - old = ftrace_call_replace(ip, aaddr); > - } > -#endif > + if (mod) > + ftrace_set_mod(&rec->arch, mod); > > + old = ftrace_call_replace_mod(ftrace_get_mod(&rec->arch), ip, aaddr); > new = ftrace_nop_replace(rec); > ret = ftrace_modify_code(ip, old, new, true); > > diff --git a/arch/arm/kernel/insn.c b/arch/arm/kernel/insn.c > index 2e844b70386b..37ec5734309e 100644 > --- a/arch/arm/kernel/insn.c > +++ b/arch/arm/kernel/insn.c > @@ -4,7 +4,7 @@ > #include > > static unsigned long > -__arm_gen_branch_thumb2(unsigned long pc, unsigned long addr, bool link) > +__arm_gen_branch_thumb2(unsigned long pc, unsigned long addr, bool link, bool check) > { > unsigned long s, j1, j2, i1, i2, imm10, imm11; > unsigned long first, second; > @@ -12,7 +12,7 @@ __arm_gen_branch_thumb2(unsigned long pc, unsigned long addr, bool link) > > offset = (long)addr - (long)(pc + 4); > if (offset < -16777216 || offset > 16777214) { > - WARN_ON_ONCE(1); > + WARN_ON_ONCE(check); > return 0; > } > > @@ -34,7 +34,7 @@ __arm_gen_branch_thumb2(unsigned long pc, unsigned long addr, bool link) > } > > static unsigned long > -__arm_gen_branch_arm(unsigned long pc, unsigned long addr, bool link) > +__arm_gen_branch_arm(unsigned long pc, unsigned long addr, bool link, bool check) > { > unsigned long opcode = 0xea000000; > long offset; > @@ -44,7 +44,7 @@ __arm_gen_branch_arm(unsigned long pc, unsigned long addr, bool link) > > offset = (long)addr - (long)(pc + 8); > if (unlikely(offset < -33554432 || offset > 33554428)) { > - WARN_ON_ONCE(1); > + WARN_ON_ONCE(check); > return 0; > } > > @@ -54,10 +54,10 @@ __arm_gen_branch_arm(unsigned long pc, unsigned long addr, bool link) > } > > unsigned long > -__arm_gen_branch(unsigned long pc, unsigned long addr, bool link) > +__arm_gen_branch(unsigned long pc, unsigned long addr, bool link, bool check) > { > if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) > - return __arm_gen_branch_thumb2(pc, addr, link); > + return __arm_gen_branch_thumb2(pc, addr, link, check); > else > - return __arm_gen_branch_arm(pc, addr, link); > + return __arm_gen_branch_arm(pc, addr, link, check); > } > -- Florian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel