From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex =?utf-8?Q?Benn=C3=A9e?= Subject: Re: [PATCH 06/27] arm64/sve: System register and exception syndrome definitions Date: Mon, 21 Aug 2017 15:36:37 +0100 Message-ID: <87378l56oa.fsf@linaro.org> References: <1502280338-23002-1-git-send-email-Dave.Martin@arm.com> <1502280338-23002-7-git-send-email-Dave.Martin@arm.com> <87a82t5kos.fsf@linaro.org> <20170821135654.GP6321@e103592.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from mail-wr0-f174.google.com ([209.85.128.174]:37664 "EHLO mail-wr0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751738AbdHUOgr (ORCPT ); Mon, 21 Aug 2017 10:36:47 -0400 Received: by mail-wr0-f174.google.com with SMTP id z91so99422724wrc.4 for ; Mon, 21 Aug 2017 07:36:47 -0700 (PDT) In-reply-to: <20170821135654.GP6321@e103592.cambridge.arm.com> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dave Martin Cc: linux-arch@vger.kernel.org, libc-alpha@sourceware.org, Ard Biesheuvel , Szabolcs Nagy , Catalin Marinas , Will Deacon , Richard Sandiford , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Dave Martin writes: > On Mon, Aug 21, 2017 at 10:33:55AM +0100, Alex Bennée wrote: >> >> Dave Martin writes: >> >> > The SVE architecture adds some system registers, ID register fields >> > and a dedicated ESR exception class. >> > >> > This patch adds the appropriate definitions that will be needed by >> > the kernel. >> > >> > Signed-off-by: Dave Martin >> > --- >> > arch/arm64/include/asm/esr.h | 3 ++- >> > arch/arm64/include/asm/kvm_arm.h | 1 + >> > arch/arm64/include/asm/sysreg.h | 16 ++++++++++++++++ >> > arch/arm64/kernel/traps.c | 1 + >> > 4 files changed, 20 insertions(+), 1 deletion(-) > > [...] > >> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h >> > index 248339e..2d259e8 100644 >> > --- a/arch/arm64/include/asm/sysreg.h >> > +++ b/arch/arm64/include/asm/sysreg.h > > [...] > >> > +#define SYS_ZCR_EL1 sys_reg(3, 0, 1, 2, 0) >> > + >> >> I'll have to take these on trust. They are mentioned in both the ARM ARM >> and the SVE supplement but I can't see any actual definitions of them. > > [I note from subsequent replies you've now found this in the > accompanying HTML] > > [...] > >> > +#define CPACR_EL1_ZEN_EL1EN (1 << 16) >> > +#define CPACR_EL1_ZEN_EL0EN (1 << 17) >> > +#define CPACR_EL1_ZEN (CPACR_EL1_ZEN_EL1EN | >> > CPACR_EL1_ZEN_EL0EN) >> >> This is a little weird as it is a 2 bit field in which 00 and 11 are not >> simply the sum of their bits. If the code wrote CPACR_EL1_ZEN_EL0EN | >> CPACR_EL1_ZEN_EL1EN to the CPACR_EL1 you wouldn't get the expected behaviour. > > This seemed natural-ish if you believe that disabling at EL1 implies > disabling at EL0. This is consistent with the way the traps at EL2/3 > work, and lack of this property would be a sort of privilege inversion. > > The meanings of the bits are not orthogonal, but it's still useful to be > able to twiddle EL0EN by itself when EL1EN is set (since we wan't to > control access for EL0 but leave EL1 access enabled). > > Maybe comments would be sufficient: > > #define CPACR_EL1_ZEN_EL1EN ... /* enable EL1 access */ > #define CPACR_EL1_ZEN_EL0EN ... /* enable EL0 access, if EL1EN is also > set */ Certainly it would help. I'll see as I go through the rest of the code but it looks like a potential bear trap we should at least mitigate. > > I'm not sure how to make the names both reasonably terse and self- > escribing, but I'm open to ideas. Sorry no decent ideas. Naming things is hard as they say. BTW see the follow-up mail for the other things I found.... From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f174.google.com ([209.85.128.174]:37664 "EHLO mail-wr0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751738AbdHUOgr (ORCPT ); Mon, 21 Aug 2017 10:36:47 -0400 Received: by mail-wr0-f174.google.com with SMTP id z91so99422724wrc.4 for ; Mon, 21 Aug 2017 07:36:47 -0700 (PDT) References: <1502280338-23002-1-git-send-email-Dave.Martin@arm.com> <1502280338-23002-7-git-send-email-Dave.Martin@arm.com> <87a82t5kos.fsf@linaro.org> <20170821135654.GP6321@e103592.cambridge.arm.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= Subject: Re: [PATCH 06/27] arm64/sve: System register and exception syndrome definitions In-reply-to: <20170821135654.GP6321@e103592.cambridge.arm.com> Date: Mon, 21 Aug 2017 15:36:37 +0100 Message-ID: <87378l56oa.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dave Martin Cc: linux-arch@vger.kernel.org, libc-alpha@sourceware.org, Ard Biesheuvel , Szabolcs Nagy , Catalin Marinas , Will Deacon , Richard Sandiford , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Message-ID: <20170821143637.khJzOpchwXKbTno-bZAUvQmlx2_eECPPULZ6VmP5RmI@z> Dave Martin writes: > On Mon, Aug 21, 2017 at 10:33:55AM +0100, Alex Bennée wrote: >> >> Dave Martin writes: >> >> > The SVE architecture adds some system registers, ID register fields >> > and a dedicated ESR exception class. >> > >> > This patch adds the appropriate definitions that will be needed by >> > the kernel. >> > >> > Signed-off-by: Dave Martin >> > --- >> > arch/arm64/include/asm/esr.h | 3 ++- >> > arch/arm64/include/asm/kvm_arm.h | 1 + >> > arch/arm64/include/asm/sysreg.h | 16 ++++++++++++++++ >> > arch/arm64/kernel/traps.c | 1 + >> > 4 files changed, 20 insertions(+), 1 deletion(-) > > [...] > >> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h >> > index 248339e..2d259e8 100644 >> > --- a/arch/arm64/include/asm/sysreg.h >> > +++ b/arch/arm64/include/asm/sysreg.h > > [...] > >> > +#define SYS_ZCR_EL1 sys_reg(3, 0, 1, 2, 0) >> > + >> >> I'll have to take these on trust. They are mentioned in both the ARM ARM >> and the SVE supplement but I can't see any actual definitions of them. > > [I note from subsequent replies you've now found this in the > accompanying HTML] > > [...] > >> > +#define CPACR_EL1_ZEN_EL1EN (1 << 16) >> > +#define CPACR_EL1_ZEN_EL0EN (1 << 17) >> > +#define CPACR_EL1_ZEN (CPACR_EL1_ZEN_EL1EN | >> > CPACR_EL1_ZEN_EL0EN) >> >> This is a little weird as it is a 2 bit field in which 00 and 11 are not >> simply the sum of their bits. If the code wrote CPACR_EL1_ZEN_EL0EN | >> CPACR_EL1_ZEN_EL1EN to the CPACR_EL1 you wouldn't get the expected behaviour. > > This seemed natural-ish if you believe that disabling at EL1 implies > disabling at EL0. This is consistent with the way the traps at EL2/3 > work, and lack of this property would be a sort of privilege inversion. > > The meanings of the bits are not orthogonal, but it's still useful to be > able to twiddle EL0EN by itself when EL1EN is set (since we wan't to > control access for EL0 but leave EL1 access enabled). > > Maybe comments would be sufficient: > > #define CPACR_EL1_ZEN_EL1EN ... /* enable EL1 access */ > #define CPACR_EL1_ZEN_EL0EN ... /* enable EL0 access, if EL1EN is also > set */ Certainly it would help. I'll see as I go through the rest of the code but it looks like a potential bear trap we should at least mitigate. > > I'm not sure how to make the names both reasonably terse and self- > escribing, but I'm open to ideas. Sorry no decent ideas. Naming things is hard as they say. BTW see the follow-up mail for the other things I found.... -- Alex Bennée