From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Martin Subject: Re: [PATCH v2 11/12] arm64: BTI: Reset BTYPE when skipping emulated instructions Date: Fri, 18 Oct 2019 15:49:11 +0100 Message-ID: <20191018144910.GF27757@arm.com> References: <1570733080-21015-1-git-send-email-Dave.Martin@arm.com> <1570733080-21015-12-git-send-email-Dave.Martin@arm.com> <20191011142157.GC33537@lakrids.cambridge.arm.com> <20191011144743.GJ27757@arm.com> <20191018110428.GA27759@lakrids.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20191018110428.GA27759@lakrids.cambridge.arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Mark Rutland Cc: Paul Elliott , Peter Zijlstra , Catalin Marinas , Will Deacon , Yu-cheng Yu , Amit Kachhap , Vincenzo Frascino , linux-arch@vger.kernel.org, Eugene Syromiatnikov , Szabolcs Nagy , "H.J. Lu" , Andrew Jones , Kees Cook , Arnd Bergmann , Jann Horn , Richard Henderson , Kristina =?utf-8?Q?Mart=C5=A1enko?= , Mark Brown , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Florian Weimer , linux-kernel@vger.kernel.org, Sudakshina Das List-Id: linux-arch.vger.kernel.org On Fri, Oct 18, 2019 at 12:04:29PM +0100, Mark Rutland wrote: > On Fri, Oct 11, 2019 at 03:47:43PM +0100, Dave Martin wrote: > > On Fri, Oct 11, 2019 at 03:21:58PM +0100, Mark Rutland wrote: > > > On Thu, Oct 10, 2019 at 07:44:39PM +0100, Dave Martin wrote: > > > > Since normal execution of any non-branch instruction resets the > > > > PSTATE BTYPE field to 0, so do the same thing when emulating a > > > > trapped instruction. > > > > > > > > Branches don't trap directly, so we should never need to assign a > > > > non-zero value to BTYPE here. > > > > > > > > Signed-off-by: Dave Martin > > > > --- > > > > arch/arm64/kernel/traps.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > > > index 3af2768..4d8ce50 100644 > > > > --- a/arch/arm64/kernel/traps.c > > > > +++ b/arch/arm64/kernel/traps.c > > > > @@ -331,6 +331,8 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) > > > > > > > > if (regs->pstate & PSR_MODE32_BIT) > > > > advance_itstate(regs); > > > > + else > > > > + regs->pstate &= ~(u64)PSR_BTYPE_MASK; > > > > > > This looks good to me, with one nit below. > > > > > > We don't (currently) need the u64 cast here, and it's inconsistent with > > > what we do elsewhere. If the upper 32-bit of pstate get allocated, we'll > > > need to fix up all the other masking we do: > > > > Huh, looks like I missed that. Dang. Will fix. > > > > > [mark@lakrids:~/src/linux]% git grep 'pstate &= ~' > > > arch/arm64/kernel/armv8_deprecated.c: regs->pstate &= ~PSR_AA32_E_BIT; > > > arch/arm64/kernel/cpufeature.c: regs->pstate &= ~PSR_SSBS_BIT; > > > arch/arm64/kernel/debug-monitors.c: regs->pstate &= ~DBG_SPSR_SS; > > > arch/arm64/kernel/insn.c: pstate &= ~(pstate >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */ > > > arch/arm64/kernel/insn.c: pstate &= ~(pstate >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */ > > > arch/arm64/kernel/probes/kprobes.c: regs->pstate &= ~PSR_D_BIT; > > > arch/arm64/kernel/probes/kprobes.c: regs->pstate &= ~DAIF_MASK; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~SPSR_EL1_AARCH32_RES0_BITS; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~PSR_AA32_E_BIT; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~SPSR_EL1_AARCH64_RES0_BITS; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~DBG_SPSR_SS; > > > arch/arm64/kernel/ssbd.c: task_pt_regs(task)->pstate &= ~val; > > > arch/arm64/kernel/traps.c: regs->pstate &= ~PSR_AA32_IT_MASK; > > > > > > ... and at that point I'd suggest we should just ensure the bit > > > definitions are all defined as unsigned long in the first place since > > > adding casts to each use is error-prone. > > > > Are we concerned about changing the types of UAPI #defines? That can > > cause subtle and unexpected breakage, especially when the signedness > > of a #define changes. > > > > Ideally, we'd just change all these to 1UL << n. > > I agree that's the ideal -- I don't know how concerned we are w.r.t. the > UAPI headers, I'm afraid. OK, I'll following the existing convention for now, keep the #define as (implicitly) signed, and drop the u64 casts. At some point in the future we may want to refactor the headers so that the kernel uses shadow register bit definitions that are always u64. The new HWCAP definitions provide a reasonable template for doing that kind of thing. It's probably best not to do anything to alter the types of the UAPI definitions. I will shamelessly duck this for now :| Cheers ---Dave From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [217.140.110.172] ([217.140.110.172]:41518 "EHLO foss.arm.com" rhost-flags-FAIL-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1729257AbfJROth (ORCPT ); Fri, 18 Oct 2019 10:49:37 -0400 Date: Fri, 18 Oct 2019 15:49:11 +0100 From: Dave Martin Subject: Re: [PATCH v2 11/12] arm64: BTI: Reset BTYPE when skipping emulated instructions Message-ID: <20191018144910.GF27757@arm.com> References: <1570733080-21015-1-git-send-email-Dave.Martin@arm.com> <1570733080-21015-12-git-send-email-Dave.Martin@arm.com> <20191011142157.GC33537@lakrids.cambridge.arm.com> <20191011144743.GJ27757@arm.com> <20191018110428.GA27759@lakrids.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191018110428.GA27759@lakrids.cambridge.arm.com> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Mark Rutland Cc: Paul Elliott , Peter Zijlstra , Catalin Marinas , Will Deacon , Andrew Jones , Amit Kachhap , Vincenzo Frascino , linux-arch@vger.kernel.org, Eugene Syromiatnikov , Szabolcs Nagy , "H.J. Lu" , Yu-cheng Yu , Kees Cook , Arnd Bergmann , Jann Horn , Richard Henderson , Kristina =?utf-8?Q?Mart=C5=A1enko?= , Mark Brown , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Florian Weimer , linux-kernel@vger.kernel.org, Sudakshina Das Message-ID: <20191018144911.18nB154JFWgYZkqF3LyM-58Ij3DThGvulELzzbUjTt8@z> On Fri, Oct 18, 2019 at 12:04:29PM +0100, Mark Rutland wrote: > On Fri, Oct 11, 2019 at 03:47:43PM +0100, Dave Martin wrote: > > On Fri, Oct 11, 2019 at 03:21:58PM +0100, Mark Rutland wrote: > > > On Thu, Oct 10, 2019 at 07:44:39PM +0100, Dave Martin wrote: > > > > Since normal execution of any non-branch instruction resets the > > > > PSTATE BTYPE field to 0, so do the same thing when emulating a > > > > trapped instruction. > > > > > > > > Branches don't trap directly, so we should never need to assign a > > > > non-zero value to BTYPE here. > > > > > > > > Signed-off-by: Dave Martin > > > > --- > > > > arch/arm64/kernel/traps.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > > > index 3af2768..4d8ce50 100644 > > > > --- a/arch/arm64/kernel/traps.c > > > > +++ b/arch/arm64/kernel/traps.c > > > > @@ -331,6 +331,8 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) > > > > > > > > if (regs->pstate & PSR_MODE32_BIT) > > > > advance_itstate(regs); > > > > + else > > > > + regs->pstate &= ~(u64)PSR_BTYPE_MASK; > > > > > > This looks good to me, with one nit below. > > > > > > We don't (currently) need the u64 cast here, and it's inconsistent with > > > what we do elsewhere. If the upper 32-bit of pstate get allocated, we'll > > > need to fix up all the other masking we do: > > > > Huh, looks like I missed that. Dang. Will fix. > > > > > [mark@lakrids:~/src/linux]% git grep 'pstate &= ~' > > > arch/arm64/kernel/armv8_deprecated.c: regs->pstate &= ~PSR_AA32_E_BIT; > > > arch/arm64/kernel/cpufeature.c: regs->pstate &= ~PSR_SSBS_BIT; > > > arch/arm64/kernel/debug-monitors.c: regs->pstate &= ~DBG_SPSR_SS; > > > arch/arm64/kernel/insn.c: pstate &= ~(pstate >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */ > > > arch/arm64/kernel/insn.c: pstate &= ~(pstate >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */ > > > arch/arm64/kernel/probes/kprobes.c: regs->pstate &= ~PSR_D_BIT; > > > arch/arm64/kernel/probes/kprobes.c: regs->pstate &= ~DAIF_MASK; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~SPSR_EL1_AARCH32_RES0_BITS; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~PSR_AA32_E_BIT; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~SPSR_EL1_AARCH64_RES0_BITS; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~DBG_SPSR_SS; > > > arch/arm64/kernel/ssbd.c: task_pt_regs(task)->pstate &= ~val; > > > arch/arm64/kernel/traps.c: regs->pstate &= ~PSR_AA32_IT_MASK; > > > > > > ... and at that point I'd suggest we should just ensure the bit > > > definitions are all defined as unsigned long in the first place since > > > adding casts to each use is error-prone. > > > > Are we concerned about changing the types of UAPI #defines? That can > > cause subtle and unexpected breakage, especially when the signedness > > of a #define changes. > > > > Ideally, we'd just change all these to 1UL << n. > > I agree that's the ideal -- I don't know how concerned we are w.r.t. the > UAPI headers, I'm afraid. OK, I'll following the existing convention for now, keep the #define as (implicitly) signed, and drop the u64 casts. At some point in the future we may want to refactor the headers so that the kernel uses shadow register bit definitions that are always u64. The new HWCAP definitions provide a reasonable template for doing that kind of thing. It's probably best not to do anything to alter the types of the UAPI definitions. I will shamelessly duck this for now :| Cheers ---Dave 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=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,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 33B74CA9EAB for ; Fri, 18 Oct 2019 14:49:42 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 E29AC21D7C for ; Fri, 18 Oct 2019 14:49:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="t1wReMvN" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E29AC21D7C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-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=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=u7b2iFn+YTafGdMoUUjLUjmyoku7gGEZYqwxS5koCfs=; b=t1wReMvNxI3U/D vwnrPn/Z/6S1xe28dgrV+Bp2UPbeWdxjLDwenHeWi5uYd6Zrn05KqU0WcHcVnoXi2E/9cJzOr7QH+ FkcdFUYa9/AdWu7QoL3yVjCyqy/zdlWUmZivkdkJx1Ftuc26Pdpc5mA9Y7z0Rw0U7Jas4Ix5Aucih ma7lhah+tbKXDoJlrmk0QazvJQix1TKJrJtKw2LSeYx2lcxc+WdXcyMgKOKf4sYEJXgHhh0MHYz1z PioSzCIWBkZAfvviPk5C0cscMDnaY1nWAeECYjsXbsugvZxkLCE6Adw27q1UFxfeoF7WtHmtCNX2r Ll38SPAGcOJ28fm2bWRw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1iLTZK-0002pA-Fe; Fri, 18 Oct 2019 14:49:30 +0000 Received: from [217.140.110.172] (helo=foss.arm.com) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1iLTZH-0002no-5S for linux-arm-kernel@lists.infradead.org; Fri, 18 Oct 2019 14:49:28 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 14ED1B57; Fri, 18 Oct 2019 07:49:16 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2E1803F718; Fri, 18 Oct 2019 07:49:13 -0700 (PDT) Date: Fri, 18 Oct 2019 15:49:11 +0100 From: Dave Martin To: Mark Rutland Subject: Re: [PATCH v2 11/12] arm64: BTI: Reset BTYPE when skipping emulated instructions Message-ID: <20191018144910.GF27757@arm.com> References: <1570733080-21015-1-git-send-email-Dave.Martin@arm.com> <1570733080-21015-12-git-send-email-Dave.Martin@arm.com> <20191011142157.GC33537@lakrids.cambridge.arm.com> <20191011144743.GJ27757@arm.com> <20191018110428.GA27759@lakrids.cambridge.arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20191018110428.GA27759@lakrids.cambridge.arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191018_074927_294475_D68D8BB3 X-CRM114-Status: GOOD ( 24.67 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Paul Elliott , Peter Zijlstra , Catalin Marinas , Will Deacon , Yu-cheng Yu , Amit Kachhap , Vincenzo Frascino , linux-arch@vger.kernel.org, Eugene Syromiatnikov , Szabolcs Nagy , "H.J. Lu" , Andrew Jones , Kees Cook , Arnd Bergmann , Jann Horn , Richard Henderson , Kristina =?utf-8?Q?Mart=C5=A1enko?= , Mark Brown , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Florian Weimer , linux-kernel@vger.kernel.org, Sudakshina Das Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Oct 18, 2019 at 12:04:29PM +0100, Mark Rutland wrote: > On Fri, Oct 11, 2019 at 03:47:43PM +0100, Dave Martin wrote: > > On Fri, Oct 11, 2019 at 03:21:58PM +0100, Mark Rutland wrote: > > > On Thu, Oct 10, 2019 at 07:44:39PM +0100, Dave Martin wrote: > > > > Since normal execution of any non-branch instruction resets the > > > > PSTATE BTYPE field to 0, so do the same thing when emulating a > > > > trapped instruction. > > > > > > > > Branches don't trap directly, so we should never need to assign a > > > > non-zero value to BTYPE here. > > > > > > > > Signed-off-by: Dave Martin > > > > --- > > > > arch/arm64/kernel/traps.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > > > index 3af2768..4d8ce50 100644 > > > > --- a/arch/arm64/kernel/traps.c > > > > +++ b/arch/arm64/kernel/traps.c > > > > @@ -331,6 +331,8 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) > > > > > > > > if (regs->pstate & PSR_MODE32_BIT) > > > > advance_itstate(regs); > > > > + else > > > > + regs->pstate &= ~(u64)PSR_BTYPE_MASK; > > > > > > This looks good to me, with one nit below. > > > > > > We don't (currently) need the u64 cast here, and it's inconsistent with > > > what we do elsewhere. If the upper 32-bit of pstate get allocated, we'll > > > need to fix up all the other masking we do: > > > > Huh, looks like I missed that. Dang. Will fix. > > > > > [mark@lakrids:~/src/linux]% git grep 'pstate &= ~' > > > arch/arm64/kernel/armv8_deprecated.c: regs->pstate &= ~PSR_AA32_E_BIT; > > > arch/arm64/kernel/cpufeature.c: regs->pstate &= ~PSR_SSBS_BIT; > > > arch/arm64/kernel/debug-monitors.c: regs->pstate &= ~DBG_SPSR_SS; > > > arch/arm64/kernel/insn.c: pstate &= ~(pstate >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */ > > > arch/arm64/kernel/insn.c: pstate &= ~(pstate >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */ > > > arch/arm64/kernel/probes/kprobes.c: regs->pstate &= ~PSR_D_BIT; > > > arch/arm64/kernel/probes/kprobes.c: regs->pstate &= ~DAIF_MASK; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~SPSR_EL1_AARCH32_RES0_BITS; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~PSR_AA32_E_BIT; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~SPSR_EL1_AARCH64_RES0_BITS; > > > arch/arm64/kernel/ptrace.c: regs->pstate &= ~DBG_SPSR_SS; > > > arch/arm64/kernel/ssbd.c: task_pt_regs(task)->pstate &= ~val; > > > arch/arm64/kernel/traps.c: regs->pstate &= ~PSR_AA32_IT_MASK; > > > > > > ... and at that point I'd suggest we should just ensure the bit > > > definitions are all defined as unsigned long in the first place since > > > adding casts to each use is error-prone. > > > > Are we concerned about changing the types of UAPI #defines? That can > > cause subtle and unexpected breakage, especially when the signedness > > of a #define changes. > > > > Ideally, we'd just change all these to 1UL << n. > > I agree that's the ideal -- I don't know how concerned we are w.r.t. the > UAPI headers, I'm afraid. OK, I'll following the existing convention for now, keep the #define as (implicitly) signed, and drop the u64 casts. At some point in the future we may want to refactor the headers so that the kernel uses shadow register bit definitions that are always u64. The new HWCAP definitions provide a reasonable template for doing that kind of thing. It's probably best not to do anything to alter the types of the UAPI definitions. I will shamelessly duck this for now :| Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel