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=-10.8 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 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 D8B07C4727C for ; Tue, 29 Sep 2020 13:47:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 81022208FE for ; Tue, 29 Sep 2020 13:47:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601387227; bh=g8XcAgcDdCLuUnEGsc4x5tUH7M2XpVkS8jU3ISBpim8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:List-ID:From; b=E2pbw8aCxDKr9B5pgJIZ48T6KSF9FKc51SgZYyVq3rXL7wW7VPFIjnZQ21I48ACPi kga5mfW38C+AwfuciQjY7FM6YL/v75VNC1AxV8oNcq7nLwFK+gVyzRIPhSEVL7sAsY pINM2mlvKvtmeTMWU64u2W45QtxKR2ljhIjgfryk= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730422AbgI2NrG (ORCPT ); Tue, 29 Sep 2020 09:47:06 -0400 Received: from mail.kernel.org ([198.145.29.99]:34894 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729858AbgI2Nq7 (ORCPT ); Tue, 29 Sep 2020 09:46:59 -0400 Received: from mail-oo1-f47.google.com (mail-oo1-f47.google.com [209.85.161.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 58C8C21924 for ; Tue, 29 Sep 2020 13:46:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601387218; bh=g8XcAgcDdCLuUnEGsc4x5tUH7M2XpVkS8jU3ISBpim8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=RTD2HHxYM7e/CJXp+98tgz6dnMbrFfMhcTmBottNdz3wL156TAmFU90+XU+XevIb+ fhVOtCeRJwwZ49CBogToraL7lVha3Lj8AhQf/hwcg1gqVFffKB/njPh8n3gEQtrYkL 8tAQ9AGASJ8/YdQBPQfi4syXGJExBvIpmiyjuWoM= Received: by mail-oo1-f47.google.com with SMTP id g26so1277517ooa.9 for ; Tue, 29 Sep 2020 06:46:58 -0700 (PDT) X-Gm-Message-State: AOAM533a1ABtou1b1AQxXy0DV+ChR0JpVWa9h3maHwAyAadsir74kBcd Rjj4wekP1En5KcJ5nH2ffBn2RElXXNCEmd7NrQ== X-Google-Smtp-Source: ABdhPJz6Pc0VWjIQlD0GF6Ki2VCDCnPNuQf2Vf7xpYiNkmwtih6mHNTieirGSSya3vQ0fufPg/uSnA+K5AJ7f40wHrw= X-Received: by 2002:a4a:d306:: with SMTP id g6mr4688913oos.25.1601387217467; Tue, 29 Sep 2020 06:46:57 -0700 (PDT) MIME-Version: 1.0 References: <20200911215118.2887710-1-robh@kernel.org> <20200911215118.2887710-2-robh@kernel.org> <20200928182601.GA11974@willie-the-truck> In-Reply-To: <20200928182601.GA11974@willie-the-truck> From: Rob Herring Date: Tue, 29 Sep 2020 08:46:46 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 01/10] arm64: pmu: Add hook to handle pmu-related undefined instructions To: Will Deacon Cc: Catalin Marinas , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Jiri Olsa , "linux-kernel@vger.kernel.org" , linux-arm-kernel , Alexander Shishkin , Namhyung Kim , Raphael Gault , Mark Rutland , Jonathan Cameron , Ian Rogers , Honnappa Nagarahalli Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 28, 2020 at 1:26 PM Will Deacon wrote: > > 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? So back to the RFC version[1]!? That would mean pulling librseq into the kernel based on the prior discussion. It doesn't look like that has happened yet. Why not just drop the undef hook? For heterogeneous systems, we require userspace to pin itself to cores for a specific PMU. See patch 9. If userspace fails to do that, then it gets to keep the pieces. Rob [1] https://lore.kernel.org/linux-arm-kernel/20190625134117.r3gysn7mvzzdrytj@willie-the-truck/ 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=-10.7 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 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 0925DC4727C for ; Tue, 29 Sep 2020 13:48:55 +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 73B112145D for ; Tue, 29 Sep 2020 13:48:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="iOK1bcd5"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="RTD2HHxY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 73B112145D 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:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=HRpP/BCGk9zpYnGWR6FrRPmiT43c1nx74+KuDM5zXlI=; b=iOK1bcd5BiZUrKuBACeJUFAjW NhdlL+j7ooxMRW4P8IN8C0NTop2d7UhgRcQQshWWKIYMXhsKf5UxSuOUtcooX0+RDrExAjD6OUgH3 7AXyssJpI+dSUdhJGuT2PKp6hq81yeL0c6B9Uv8rzTEz9U1RfT/5V4ttANReB8YJsPWMjQYoS/C3j /wwwW+Z8KvrgApp3kwoMRjD0bApd8ERzCbQTMZKZp9lEfimCeDJwNwUJnbY4gAl+ulzxpOVtVrS+m UMue5slrQ8u/VtBrioCz13QrbFKF4U1Gb/KW4n7MCNsaP41MHJG5J6b7cfZD8Ht1InuqWD8v4Er1J t0N0fUtjQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kNFyB-0005pd-DJ; Tue, 29 Sep 2020 13:47:03 +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 1kNFy7-0005nW-Bf for linux-arm-kernel@lists.infradead.org; Tue, 29 Sep 2020 13:47:00 +0000 Received: from mail-oo1-f54.google.com (mail-oo1-f54.google.com [209.85.161.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 62D0B21D43 for ; Tue, 29 Sep 2020 13:46:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601387218; bh=g8XcAgcDdCLuUnEGsc4x5tUH7M2XpVkS8jU3ISBpim8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=RTD2HHxYM7e/CJXp+98tgz6dnMbrFfMhcTmBottNdz3wL156TAmFU90+XU+XevIb+ fhVOtCeRJwwZ49CBogToraL7lVha3Lj8AhQf/hwcg1gqVFffKB/njPh8n3gEQtrYkL 8tAQ9AGASJ8/YdQBPQfi4syXGJExBvIpmiyjuWoM= Received: by mail-oo1-f54.google.com with SMTP id k13so1276455oor.2 for ; Tue, 29 Sep 2020 06:46:58 -0700 (PDT) X-Gm-Message-State: AOAM532S6Zb//p/eceVWpftXCMbmwo8ErajkC5GTYaNVO6iVP09mnxGg 5zYg/kVSn1zd8nCViDqeI61KmgUmeju7uDKZ6g== X-Google-Smtp-Source: ABdhPJz6Pc0VWjIQlD0GF6Ki2VCDCnPNuQf2Vf7xpYiNkmwtih6mHNTieirGSSya3vQ0fufPg/uSnA+K5AJ7f40wHrw= X-Received: by 2002:a4a:d306:: with SMTP id g6mr4688913oos.25.1601387217467; Tue, 29 Sep 2020 06:46:57 -0700 (PDT) MIME-Version: 1.0 References: <20200911215118.2887710-1-robh@kernel.org> <20200911215118.2887710-2-robh@kernel.org> <20200928182601.GA11974@willie-the-truck> In-Reply-To: <20200928182601.GA11974@willie-the-truck> From: Rob Herring Date: Tue, 29 Sep 2020 08:46:46 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 01/10] arm64: pmu: Add hook to handle pmu-related undefined instructions To: Will Deacon X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200929_094659_554969_F791C4D1 X-CRM114-Status: GOOD ( 42.42 ) 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 , Jonathan Cameron , Namhyung Kim , Jiri Olsa , linux-arm-kernel 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 Mon, Sep 28, 2020 at 1:26 PM Will Deacon wrote: > > 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? So back to the RFC version[1]!? That would mean pulling librseq into the kernel based on the prior discussion. It doesn't look like that has happened yet. Why not just drop the undef hook? For heterogeneous systems, we require userspace to pin itself to cores for a specific PMU. See patch 9. If userspace fails to do that, then it gets to keep the pieces. Rob [1] https://lore.kernel.org/linux-arm-kernel/20190625134117.r3gysn7mvzzdrytj@willie-the-truck/ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel