On 15/01/16 20:07, Mark Rutland wrote: > [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 <geoff at infradead.org> >> --- >> 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 <asm/sysreg.h>), > 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 <asm/assembler.h> >> #include <asm/kvm_arm.h> >> #include <asm/kvm_mmu.h> >> -#include <asm/pgtable-hwdef.h> >> +#include <asm/sysreg.h> >> >> .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? Yes, that's one of the numerous instances of the same problem - I think Dave Martin also has some fixes in that area. I'll definitely take patches! M. -- Jazz is not dead. It just smells funny...
