[PATCH 06/27] arm64/sve: System register and exception syndrome definitions

[alex.bennee@linaro.org (Alex Bennée)]

Dave Martin <Dave.Martin@arm.com> writes:

> On Mon, Aug 21, 2017 at 10:33:55AM +0100, Alex Benn?e wrote:
>>
>> Dave Martin <Dave.Martin@arm.com> 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@arm.com>
>> > ---
>> >  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