Re: [PATCH v5 00/12] arm64: Automatic system register definition generation

[Catalin Marinas <catalin.marinas@arm.com>]

On Tue, Apr 26, 2022 at 07:16:52PM +0100, Mark Brown wrote:
> This patch series introduces a
> script which describes registers and the fields within them in a format
> that is easy to cross reference with the architecture reference manual
> and uses them to generate the constants we use in a standard format:
> 
> | #define REG_ID_AA64ISAR0_EL1                    S3_0_C0_C6_0
> | #define SYS_ID_AA64ISAR0_EL1                    sys_reg(3, 0, 0, 6, 0)
> | #define SYS_ID_AA64ISAR0_EL1_Op0                3
> | #define SYS_ID_AA64ISAR0_EL1_Op1                0
> | #define SYS_ID_AA64ISAR0_EL1_CRn                0
> | #define SYS_ID_AA64ISAR0_EL1_CRm                6
> | #define SYS_ID_AA64ISAR0_EL1_Op2                0
> 
> | #define ID_AA64ISAR0_EL1_RNDR                   GENMASK(63, 60)
> | #define ID_AA64ISAR0_EL1_RNDR_MASK              GENMASK(63, 60)
> | #define ID_AA64ISAR0_EL1_RNDR_SHIFT             60
> | #define ID_AA64ISAR0_EL1_RNDR_WIDTH             4
> | #define ID_AA64ISAR0_EL1_RNDR_NI                UL(0b0000)
> | #define ID_AA64ISAR0_EL1_RNDR_IMP               UL(0b0001)

I like the automatic generation of the register fields, I think it's
less prone to errors and it gives us consistent notation, so it would be
nice to have this in mainline.

I'm not convinced that we need the RES[01]_msb_lsb definitions, I'd
rather have a RES[01] with all the UL(x) or'ed in. We want to avoid
anyone making use of the RES1_x explicitly, though most likely we'd
notice this during review. But these are some 'constants' that will keep
changing even their macro name, not just the value (when one bit from
the field gets used for something else).

> At the moment this is only intended to express metadata from the
> architecture, and does not handle policy imposed by the kernel, such as
> values exposed to userspace or VMs. In future this could be extended to
> express such information. This could also be extended to cover more
> information such as the FTR_SIGNED/FTR_UNSIGNED distinction.

We could indeed add signed info for the enum fields at some point. As
for kernel possible like FTR_VISIBLE, not sure whether we should add
them to the same sysreg file. My instinct is to keep it strictly about
the architecture definitions, not kernel policy.

> Rather than attempting to convert every register at once the current
> series converts a few sample registers to provide some concrete examples
> but allow for easier updating during review of the file format and the
> script.  Handling a register at a time should also make review less
> taxing so it seems like a sensible approach in general.

So apart from RES definitions (but happy to hear an argument), I'm fine
with the patches. If you think they are ready, we could queue the
infrastructure for 5.19 and add some of the register definitions at -rc1
to avoid conflicts (the more conversions the better).

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel