Dave Martin <Dave.Martin@xxxxxxx> writes: > On Mon, Aug 21, 2017 at 10:33:55AM +0100, Alex Bennée wrote: >> >> Dave Martin <Dave.Martin@xxxxxxx> 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 <Dave.Martin@xxxxxxx> >> > --- >> > 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 _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
