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.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham 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 4F82CC2D0A8 for ; Mon, 28 Sep 2020 18:26:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B278B20789 for ; Mon, 28 Sep 2020 18:26:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601317570; bh=u3Ru8Fp5ZqhFBwNIXZAOccSRXU2X+Qr4X2uqL+hJr/U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=hYdBh4MrfBDabOMmqFAgN0wK38oKpuZHT5ky4TZX4Ks+9xLGBBFjPme5+W2Rgk6m/ L6Nt9plW1avdRcYAonxI9vEXsNZrFWwncaOB0GTiR1fdG0uDrf1SBjnZkWEUhhhEyA /j8D/UQYVQq8o46nFD0zTBGmDQYb7cVjfyNGKoxE= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726686AbgI1S0K (ORCPT ); Mon, 28 Sep 2020 14:26:10 -0400 Received: from mail.kernel.org ([198.145.29.99]:36308 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726500AbgI1S0J (ORCPT ); Mon, 28 Sep 2020 14:26:09 -0400 Received: from willie-the-truck (236.31.169.217.in-addr.arpa [217.169.31.236]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 88E23206C3; Mon, 28 Sep 2020 18:26:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601317568; bh=u3Ru8Fp5ZqhFBwNIXZAOccSRXU2X+Qr4X2uqL+hJr/U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=B03u8dyNCfhxAIlvYxgJmoafLkMChsrV98Aao3vYaGNFG8KHfTnE6sNTcZbE3ym/b Er4NWOpq1K4ZY4i7huD+aYeJ61Zy3Z6SLZPCkrjwcrWH0aT6pIP5ZDIaBtM1ErEr+Q +t/obcQz90G7jfpNwCksmWsI4HkHME2Vv6dKDlZw= Date: Mon, 28 Sep 2020 19:26:02 +0100 From: Will Deacon To: Rob Herring Cc: Catalin Marinas , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Jiri Olsa , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Alexander Shishkin , Namhyung Kim , Raphael Gault , Mark Rutland , Jonathan Cameron , Ian Rogers , honnappa.nagarahalli@arm.com Subject: Re: [PATCH v3 01/10] arm64: pmu: Add hook to handle pmu-related undefined instructions Message-ID: <20200928182601.GA11974@willie-the-truck> References: <20200911215118.2887710-1-robh@kernel.org> <20200911215118.2887710-2-robh@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200911215118.2887710-2-robh@kernel.org> User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 11, 2020 at 03:51:09PM -0600, Rob Herring wrote: > From: Raphael Gault > > This patch introduces a protection for the userspace processes which are > trying to access the registers from the pmu registers on a big.LITTLE > environment. It introduces a hook to handle undefined instructions. > > The goal here is to prevent the process to be interrupted by a signal > when the error is caused by the task being scheduled while accessing > a counter, causing the counter access to be invalid. As we are not able > to know efficiently the number of counters available physically on both > pmu in that context we consider that any faulting access to a counter > which is architecturally correct should not cause a SIGILL signal if > the permissions are set accordingly. > > This commit also modifies the mask of the mrs_hook declared in > arch/arm64/kernel/cpufeatures.c which emulates only feature register > access. This is necessary because this hook's mask was too large and > thus masking any mrs instruction, even if not related to the emulated > registers which made the pmu emulation inefficient. > > Signed-off-by: Raphael Gault > Signed-off-by: Rob Herring > --- > v2: > - Fix warning for set but unused sys_reg > --- > arch/arm64/kernel/cpufeature.c | 4 +-- > arch/arm64/kernel/perf_event.c | 54 ++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index a389b999482e..00bf53ffd9b0 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -2811,8 +2811,8 @@ static int emulate_mrs(struct pt_regs *regs, u32 insn) > } > > static struct undef_hook mrs_hook = { > - .instr_mask = 0xfff00000, > - .instr_val = 0xd5300000, > + .instr_mask = 0xffff0000, > + .instr_val = 0xd5380000, > .pstate_mask = PSR_AA32_MODE_MASK, > .pstate_val = PSR_MODE_EL0t, > .fn = emulate_mrs, > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > index 462f9a9cc44b..70538ae684da 100644 > --- a/arch/arm64/kernel/perf_event.c > +++ b/arch/arm64/kernel/perf_event.c > @@ -8,9 +8,11 @@ > * This code is based heavily on the ARMv7 perf event code. > */ > > +#include > #include > #include > #include > +#include > #include > > #include > @@ -1016,6 +1018,58 @@ static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu) > return probe.present ? 0 : -ENODEV; > } > > +static int emulate_pmu(struct pt_regs *regs, u32 insn) > +{ > + u32 rt; > + u32 pmuserenr; > + > + rt = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RT, insn); > + pmuserenr = read_sysreg(pmuserenr_el0); > + > + if ((pmuserenr & (ARMV8_PMU_USERENR_ER|ARMV8_PMU_USERENR_CR)) != > + (ARMV8_PMU_USERENR_ER|ARMV8_PMU_USERENR_CR)) > + return -EINVAL; > + > + > + /* > + * Userspace is expected to only use this in the context of the scheme > + * described in the struct perf_event_mmap_page comments. > + * > + * Given that context, we can only get here if we got migrated between > + * getting the register index and doing the MSR read. This in turn > + * implies we'll fail the sequence and retry, so any value returned is > + * 'good', all we need is to be non-fatal. > + * > + * The choice of the value 0 is comming from the fact that when > + * accessing a register which is not counting events but is accessible, > + * we get 0. > + */ > + pt_regs_write_reg(regs, rt, 0); Hmm... this feels pretty fragile since, although we may expect userspace only to trigger this in the context of the specific perf use-case, we don't have a way to detect that, so the ABI we're exposing is that EL0 accesses to non-existent counters will return 0. I don't really think that's something we want to commit to. When restartable sequences were added to the kernel, one of the proposed use-cases was to allow PMU access on big/little systems, because the sequence will abort on preemption. Taking that approach removes the need for this emulation hook entirely. Is that something we can rely on instead of this emulation hook? Cheers, Will 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,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, 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 C5C1EC2D0A8 for ; Mon, 28 Sep 2020 18:28:02 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 5A2DA206C3 for ; Mon, 28 Sep 2020 18:28:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="jWXYgz/O"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="B03u8dyN" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5A2DA206C3 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org 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=merlin.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=04QeznpBO28I3/cERwczqTLik5Qzyb8ibS1Yhct8qXU=; b=jWXYgz/O3iPaAA9kC4j8+K05X dZSUDniO+uDUpokBO++cZJECcA3k6viBL31Imwv5F0y3Ay0P/xIVLMu9eI8gRefz55t3srsrlfqK5 RET4Ak9Z3Poccf/m6QQx8KIxnQQxbjgnagA3RBY1xFxuz9HNIkZjoau2NVuahr/r8DJuuRgYHKKgt 7VYVYsnUbM4oQng96cJH1d4J6SqWJOKUNpPP0NPVkYJ9xja/qiVqtNI0TZ+/gtYd7Nl4HA6hJ/DVh Cu4pe27L9EUjEw+nXWDjQLEIa1pbHTvttLkQoyB3IOOPBl5GTEH5/nRy8zaJxJJJDLyJ+tcKT84fp 3gB7vY85w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kMxqm-0006fw-Bl; Mon, 28 Sep 2020 18:26:12 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kMxqj-0006ev-6S for linux-arm-kernel@lists.infradead.org; Mon, 28 Sep 2020 18:26:10 +0000 Received: from willie-the-truck (236.31.169.217.in-addr.arpa [217.169.31.236]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 88E23206C3; Mon, 28 Sep 2020 18:26:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601317568; bh=u3Ru8Fp5ZqhFBwNIXZAOccSRXU2X+Qr4X2uqL+hJr/U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=B03u8dyNCfhxAIlvYxgJmoafLkMChsrV98Aao3vYaGNFG8KHfTnE6sNTcZbE3ym/b Er4NWOpq1K4ZY4i7huD+aYeJ61Zy3Z6SLZPCkrjwcrWH0aT6pIP5ZDIaBtM1ErEr+Q +t/obcQz90G7jfpNwCksmWsI4HkHME2Vv6dKDlZw= Date: Mon, 28 Sep 2020 19:26:02 +0100 From: Will Deacon To: Rob Herring Subject: Re: [PATCH v3 01/10] arm64: pmu: Add hook to handle pmu-related undefined instructions Message-ID: <20200928182601.GA11974@willie-the-truck> References: <20200911215118.2887710-1-robh@kernel.org> <20200911215118.2887710-2-robh@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200911215118.2887710-2-robh@kernel.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200928_142609_399929_AF3AF042 X-CRM114-Status: GOOD ( 34.73 ) 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: Mark Rutland , Ian Rogers , Peter Zijlstra , Catalin Marinas , linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo , Alexander Shishkin , Raphael Gault , Ingo Molnar , honnappa.nagarahalli@arm.com, Jonathan Cameron , Namhyung Kim , Jiri Olsa , linux-arm-kernel@lists.infradead.org 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 On Fri, Sep 11, 2020 at 03:51:09PM -0600, Rob Herring wrote: > From: Raphael Gault > > This patch introduces a protection for the userspace processes which are > trying to access the registers from the pmu registers on a big.LITTLE > environment. It introduces a hook to handle undefined instructions. > > The goal here is to prevent the process to be interrupted by a signal > when the error is caused by the task being scheduled while accessing > a counter, causing the counter access to be invalid. As we are not able > to know efficiently the number of counters available physically on both > pmu in that context we consider that any faulting access to a counter > which is architecturally correct should not cause a SIGILL signal if > the permissions are set accordingly. > > This commit also modifies the mask of the mrs_hook declared in > arch/arm64/kernel/cpufeatures.c which emulates only feature register > access. This is necessary because this hook's mask was too large and > thus masking any mrs instruction, even if not related to the emulated > registers which made the pmu emulation inefficient. > > Signed-off-by: Raphael Gault > Signed-off-by: Rob Herring > --- > v2: > - Fix warning for set but unused sys_reg > --- > arch/arm64/kernel/cpufeature.c | 4 +-- > arch/arm64/kernel/perf_event.c | 54 ++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index a389b999482e..00bf53ffd9b0 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -2811,8 +2811,8 @@ static int emulate_mrs(struct pt_regs *regs, u32 insn) > } > > static struct undef_hook mrs_hook = { > - .instr_mask = 0xfff00000, > - .instr_val = 0xd5300000, > + .instr_mask = 0xffff0000, > + .instr_val = 0xd5380000, > .pstate_mask = PSR_AA32_MODE_MASK, > .pstate_val = PSR_MODE_EL0t, > .fn = emulate_mrs, > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > index 462f9a9cc44b..70538ae684da 100644 > --- a/arch/arm64/kernel/perf_event.c > +++ b/arch/arm64/kernel/perf_event.c > @@ -8,9 +8,11 @@ > * This code is based heavily on the ARMv7 perf event code. > */ > > +#include > #include > #include > #include > +#include > #include > > #include > @@ -1016,6 +1018,58 @@ static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu) > return probe.present ? 0 : -ENODEV; > } > > +static int emulate_pmu(struct pt_regs *regs, u32 insn) > +{ > + u32 rt; > + u32 pmuserenr; > + > + rt = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RT, insn); > + pmuserenr = read_sysreg(pmuserenr_el0); > + > + if ((pmuserenr & (ARMV8_PMU_USERENR_ER|ARMV8_PMU_USERENR_CR)) != > + (ARMV8_PMU_USERENR_ER|ARMV8_PMU_USERENR_CR)) > + return -EINVAL; > + > + > + /* > + * Userspace is expected to only use this in the context of the scheme > + * described in the struct perf_event_mmap_page comments. > + * > + * Given that context, we can only get here if we got migrated between > + * getting the register index and doing the MSR read. This in turn > + * implies we'll fail the sequence and retry, so any value returned is > + * 'good', all we need is to be non-fatal. > + * > + * The choice of the value 0 is comming from the fact that when > + * accessing a register which is not counting events but is accessible, > + * we get 0. > + */ > + pt_regs_write_reg(regs, rt, 0); Hmm... this feels pretty fragile since, although we may expect userspace only to trigger this in the context of the specific perf use-case, we don't have a way to detect that, so the ABI we're exposing is that EL0 accesses to non-existent counters will return 0. I don't really think that's something we want to commit to. When restartable sequences were added to the kernel, one of the proposed use-cases was to allow PMU access on big/little systems, because the sequence will abort on preemption. Taking that approach removes the need for this emulation hook entirely. Is that something we can rely on instead of this emulation hook? Cheers, Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel