From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.rutland@arm.com (Mark Rutland) Date: Fri, 15 Jan 2016 20:07:27 +0000 Subject: [PATCH 04/19] arm64: Cleanup SCTLR flags In-Reply-To: <701b44f00cdb8dbc7881c12508f55e993b9ce6dd.1452884156.git.geoff@infradead.org> References: <701b44f00cdb8dbc7881c12508f55e993b9ce6dd.1452884156.git.geoff@infradead.org> Message-ID: <20160115200727.GQ3262@leverpostej> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org [Adding Marc as this touches KVM code] On Fri, Jan 15, 2016 at 07:18:37PM +0000, Geoff Levand wrote: > We currently have macros defining flags for the arm64 sctlr registers in both > kvm_arm.h and sysreg.h. To clean things up and simplify move the definitions > of the SCTLR_EL2 flags from kvm_arm.h to sysreg.h, rename any SCTLR_EL1 or > SCTLR_EL2 flags that are common to both registers to be SCTLR_ELx, with 'x' > indicating a common flag, and fixup all files to include the proper header or > to use the new macro names. I am certainly in favour of having consistently named and located macros for register fields. > Signed-off-by: Geoff Levand > --- > arch/arm64/include/asm/kvm_arm.h | 11 ----------- > arch/arm64/include/asm/sysreg.h | 19 +++++++++++++++---- > arch/arm64/kvm/hyp-init.S | 6 +++--- > 3 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > index 5e6857b..92ef6f6 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -83,17 +83,6 @@ > #define HCR_INT_OVERRIDE (HCR_FMO | HCR_IMO) > > > -/* Hyp System Control Register (SCTLR_EL2) bits */ > -#define SCTLR_EL2_EE (1 << 25) > -#define SCTLR_EL2_WXN (1 << 19) > -#define SCTLR_EL2_I (1 << 12) > -#define SCTLR_EL2_SA (1 << 3) > -#define SCTLR_EL2_C (1 << 2) > -#define SCTLR_EL2_A (1 << 1) > -#define SCTLR_EL2_M 1 > -#define SCTLR_EL2_FLAGS (SCTLR_EL2_M | SCTLR_EL2_A | SCTLR_EL2_C | \ > - SCTLR_EL2_SA | SCTLR_EL2_I) SCTLR_EL2_FLAGS is a KVM-specific value (i.e. the SCTLR_EL2 flags which KVM wants to set), even if it consists solely of common fields. I believe it should stay here (with an include for ), perhaps with a KVM_ prefix to imply it's not as generic as one might assume it is. > - > /* TCR_EL2 Registers bits */ > #define TCR_EL2_RES1 ((1 << 31) | (1 << 23)) > #define TCR_EL2_TBI (1 << 20) > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index d48ab5b..109d46e 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -80,10 +80,21 @@ > #define SET_PSTATE_PAN(x) __inst_arm(0xd5000000 | REG_PSTATE_PAN_IMM |\ > (!!x)<<8 | 0x1f) > > -/* SCTLR_EL1 */ > -#define SCTLR_EL1_CP15BEN (0x1 << 5) > -#define SCTLR_EL1_SED (0x1 << 8) > -#define SCTLR_EL1_SPAN (0x1 << 23) > +/* Common SCTLR_ELx flags. */ > +#define SCTLR_ELx_EE (1 << 25) > +#define SCTLR_ELx_I (1 << 12) > +#define SCTLR_ELx_SA (1 << 3) > +#define SCTLR_ELx_C (1 << 2) > +#define SCTLR_ELx_A (1 << 1) > +#define SCTLR_ELx_M 1 For consistency, (1 << 0) would be preferable. > + > +#define SCTLR_ELx_FLAGS (SCTLR_ELx_M | SCTLR_ELx_A | SCTLR_ELx_C | \ > + SCTLR_ELx_SA | SCTLR_ELx_I) > + > +/* SCTLR_EL1 specific flags. */ > +#define SCTLR_EL1_SPAN (1 << 23) > +#define SCTLR_EL1_SED (1 << 8) > +#define SCTLR_EL1_CP15BEN (1 << 5) > > > /* id_aa64isar0 */ > diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S > index 178ba22..1d7e502 100644 > --- a/arch/arm64/kvm/hyp-init.S > +++ b/arch/arm64/kvm/hyp-init.S > @@ -20,7 +20,7 @@ > #include > #include > #include > -#include > +#include > > .text > .pushsection .hyp.idmap.text, "ax" > @@ -105,8 +105,8 @@ __do_hyp_init: > dsb sy > > mrs x4, sctlr_el2 > - and x4, x4, #SCTLR_EL2_EE // preserve endianness of EL2 > - ldr x5, =SCTLR_EL2_FLAGS > + and x4, x4, #SCTLR_ELx_EE // preserve endianness of EL2 > + ldr x5, =SCTLR_ELx_FLAGS Marc, Christoffer, I note that in SCTLR_EL2_FLAGS we don't set the RES1 bits of SCTLR_EL2 (not in head.S el2_setup). Should we perhaps be doing so so as to avoid any future surprises? Thanks, Mark. > orr x4, x4, x5 > msr sctlr_el2, x4 > isb > -- > 2.5.0 > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Date: Fri, 15 Jan 2016 20:07:27 +0000 From: Mark Rutland Subject: Re: [PATCH 04/19] arm64: Cleanup SCTLR flags Message-ID: <20160115200727.GQ3262@leverpostej> References: <701b44f00cdb8dbc7881c12508f55e993b9ce6dd.1452884156.git.geoff@infradead.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <701b44f00cdb8dbc7881c12508f55e993b9ce6dd.1452884156.git.geoff@infradead.org> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Geoff Levand , marc.zyngier@arm.com, christoffer.dall@linaro.org Cc: Catalin Marinas , Will Deacon , AKASHI Takahiro , James Morse , kexec@lists.infradead.org, linux-arm-kernel@lists.infradead.org [Adding Marc as this touches KVM code] On Fri, Jan 15, 2016 at 07:18:37PM +0000, Geoff Levand wrote: > We currently have macros defining flags for the arm64 sctlr registers in both > kvm_arm.h and sysreg.h. To clean things up and simplify move the definitions > of the SCTLR_EL2 flags from kvm_arm.h to sysreg.h, rename any SCTLR_EL1 or > SCTLR_EL2 flags that are common to both registers to be SCTLR_ELx, with 'x' > indicating a common flag, and fixup all files to include the proper header or > to use the new macro names. I am certainly in favour of having consistently named and located macros for register fields. > Signed-off-by: Geoff Levand > --- > arch/arm64/include/asm/kvm_arm.h | 11 ----------- > arch/arm64/include/asm/sysreg.h | 19 +++++++++++++++---- > arch/arm64/kvm/hyp-init.S | 6 +++--- > 3 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > index 5e6857b..92ef6f6 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -83,17 +83,6 @@ > #define HCR_INT_OVERRIDE (HCR_FMO | HCR_IMO) > > > -/* Hyp System Control Register (SCTLR_EL2) bits */ > -#define SCTLR_EL2_EE (1 << 25) > -#define SCTLR_EL2_WXN (1 << 19) > -#define SCTLR_EL2_I (1 << 12) > -#define SCTLR_EL2_SA (1 << 3) > -#define SCTLR_EL2_C (1 << 2) > -#define SCTLR_EL2_A (1 << 1) > -#define SCTLR_EL2_M 1 > -#define SCTLR_EL2_FLAGS (SCTLR_EL2_M | SCTLR_EL2_A | SCTLR_EL2_C | \ > - SCTLR_EL2_SA | SCTLR_EL2_I) SCTLR_EL2_FLAGS is a KVM-specific value (i.e. the SCTLR_EL2 flags which KVM wants to set), even if it consists solely of common fields. I believe it should stay here (with an include for ), perhaps with a KVM_ prefix to imply it's not as generic as one might assume it is. > - > /* TCR_EL2 Registers bits */ > #define TCR_EL2_RES1 ((1 << 31) | (1 << 23)) > #define TCR_EL2_TBI (1 << 20) > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index d48ab5b..109d46e 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -80,10 +80,21 @@ > #define SET_PSTATE_PAN(x) __inst_arm(0xd5000000 | REG_PSTATE_PAN_IMM |\ > (!!x)<<8 | 0x1f) > > -/* SCTLR_EL1 */ > -#define SCTLR_EL1_CP15BEN (0x1 << 5) > -#define SCTLR_EL1_SED (0x1 << 8) > -#define SCTLR_EL1_SPAN (0x1 << 23) > +/* Common SCTLR_ELx flags. */ > +#define SCTLR_ELx_EE (1 << 25) > +#define SCTLR_ELx_I (1 << 12) > +#define SCTLR_ELx_SA (1 << 3) > +#define SCTLR_ELx_C (1 << 2) > +#define SCTLR_ELx_A (1 << 1) > +#define SCTLR_ELx_M 1 For consistency, (1 << 0) would be preferable. > + > +#define SCTLR_ELx_FLAGS (SCTLR_ELx_M | SCTLR_ELx_A | SCTLR_ELx_C | \ > + SCTLR_ELx_SA | SCTLR_ELx_I) > + > +/* SCTLR_EL1 specific flags. */ > +#define SCTLR_EL1_SPAN (1 << 23) > +#define SCTLR_EL1_SED (1 << 8) > +#define SCTLR_EL1_CP15BEN (1 << 5) > > > /* id_aa64isar0 */ > diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S > index 178ba22..1d7e502 100644 > --- a/arch/arm64/kvm/hyp-init.S > +++ b/arch/arm64/kvm/hyp-init.S > @@ -20,7 +20,7 @@ > #include > #include > #include > -#include > +#include > > .text > .pushsection .hyp.idmap.text, "ax" > @@ -105,8 +105,8 @@ __do_hyp_init: > dsb sy > > mrs x4, sctlr_el2 > - and x4, x4, #SCTLR_EL2_EE // preserve endianness of EL2 > - ldr x5, =SCTLR_EL2_FLAGS > + and x4, x4, #SCTLR_ELx_EE // preserve endianness of EL2 > + ldr x5, =SCTLR_ELx_FLAGS Marc, Christoffer, I note that in SCTLR_EL2_FLAGS we don't set the RES1 bits of SCTLR_EL2 (not in head.S el2_setup). Should we perhaps be doing so so as to avoid any future surprises? Thanks, Mark. > orr x4, x4, x5 > msr sctlr_el2, x4 > isb > -- > 2.5.0 > > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec