[PATCH v3 0/3] arm64: sysreg: Fix sysreg field definitions and generation script

[PATCH v3 0/3] arm64: sysreg: Fix sysreg field definitions and generation script

[Fuad Tabba]

The sysreg definitions file has a few errors. In Enum Security,
NSACR_RFR is set to 0b0001 instead of the spec value of 0b0010.
Additionaly, the sign of the Enums DoubleLock and EIESB are wrong
(should be Signed and Unsigned, respectively).

Moreover, the file has some redundant definitions, e.g.,
RCWSMASK_EL1. While these redundant definitions are not wrong per se,
they add unnecessary code into the generated header file and could be a
source of future bugs.

Fix the Enums and remove the redundant Sysreg definitions.

To reduce the chance of this happening in the future, add validation to
the sysreg header generation script. I didn't want to go overboard with
validation, so I only added validation that would catch most of the
errors that I've seen, which also are errors that wouldn't generate a
build error later on.

Changes since v2 [1]:
- Added patch that fixes field sign definitions for DoubleLock and EIESB
- Removed whitespace fixes from first patch, since there are more
  inconsistent whitespace use in the file, and fixing it all would lead
  to too much churn.
- Collected Reviews/Acks
- Rebased on 6.17-rc3

Cheers,
/fuad

[1] https://lore.kernel.org/all/20250813164506.1203851-1-tabba@google.com/

Fuad Tabba (3):
  arm64: sysreg: Fix and tidy up sysreg field definitions
  arm64: sysreg: Correct sign definitions for EIESB and DoubleLock
  arm64: sysreg: Add validation checks to sysreg header generation
    script

 arch/arm64/tools/gen-sysreg.awk | 20 ++++++++++++++++++++
 arch/arm64/tools/sysreg         | 18 +++++-------------
 2 files changed, 25 insertions(+), 13 deletions(-)


base-commit: 1b237f190eb3d36f52dffe07a40b5eb210280e00
-- 
2.51.0.338.gd7d06c2dae-goog

[PATCH v3 1/3] arm64: sysreg: Fix and tidy up sysreg field definitions

[Fuad Tabba]

Fix the value of ID_PFR1_EL1.Security NSACR_RFR to be 0b0010, as per
DDI0601/2025-06, which wasn't correctly set when introduced in commit
1224308075f1 ("arm64/sysreg: Convert ID_PFR1_EL1 to automatic generation").

While at it, remove redundant definitions of CPACR_EL12 and
RCWSMASK_EL1 and fix some typos.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Fuad Tabba <tabba@google.com>
---
Note that NSACR_RFR isn't used in the kernel as far as I could tell, so
I didn't add a 'Fixes' tag.
---
 arch/arm64/tools/sysreg | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 696ab1f32a67..f1a012ee0db6 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -31,7 +31,7 @@
 # Mapping	<name_EL1>
 # EndSysreg
 
-# Where multiple system regsiters are not VHE aliases but share a
+# Where multiple system registers are not VHE aliases but share a
 # common layout, a SysregFields block can be used to describe the
 # shared layout:
 
@@ -54,7 +54,7 @@
 #
 # In general it is recommended that new enumeration items be named for the
 # feature that introduces them (eg, FEAT_LS64_ACCDATA introduces enumeration
-# item ACCDATA) though it may be more taseful to do something else.
+# item ACCDATA) though it may be more tasteful to do something else.
 
 Sysreg	OSDTRRX_EL1	2	0	0	0	2
 Res0	63:32
@@ -474,7 +474,7 @@ EndEnum
 Enum	7:4	Security
 	0b0000	NI
 	0b0001	EL3
-	0b0001	NSACR_RFR
+	0b0010	NSACR_RFR
 EndEnum
 UnsignedEnum	3:0	ProgMod
 	0b0000	NI
@@ -2528,10 +2528,6 @@ Field	17:16	ZEN
 Res0	15:0
 EndSysreg
 
-Sysreg	CPACR_EL12      3	5	1	0	2
-Mapping	CPACR_EL1
-EndSysreg
-
 Sysreg	CPACRALIAS_EL1  3	0	1	4	4
 Mapping	CPACR_EL1
 EndSysreg
@@ -2576,10 +2572,6 @@ Sysreg	PFAR_EL12	3	5	6	0	5
 Mapping	PFAR_EL1
 EndSysreg
 
-Sysreg	RCWSMASK_EL1	3	0	13	0	3
-Field	63:0	RCWSMASK
-EndSysreg
-
 Sysreg	SCTLR2_EL1      3	0	1	0	3
 Res0    63:13
 Field   12      CPTM0
-- 
2.51.0.338.gd7d06c2dae-goog

Re: [PATCH v3 1/3] arm64: sysreg: Fix and tidy up sysreg field definitions

[Mark Rutland]

On Fri, Aug 29, 2025 at 10:51:41AM +0100, Fuad Tabba wrote:
> Fix the value of ID_PFR1_EL1.Security NSACR_RFR to be 0b0010, as per
> DDI0601/2025-06, which wasn't correctly set when introduced in commit
> 1224308075f1 ("arm64/sysreg: Convert ID_PFR1_EL1 to automatic generation").
> 
> While at it, remove redundant definitions of CPACR_EL12 and
> RCWSMASK_EL1 and fix some typos.
> 
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Acked-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
> Note that NSACR_RFR isn't used in the kernel as far as I could tell, so
> I didn't add a 'Fixes' tag.

Since this is all cleanup, I think that's fine, but were that necessary,
it would be nicer to have the NSACR_RFR fix separate from the other
cleanups.

FWIW, as this stands:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/tools/sysreg | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
> index 696ab1f32a67..f1a012ee0db6 100644
> --- a/arch/arm64/tools/sysreg
> +++ b/arch/arm64/tools/sysreg
> @@ -31,7 +31,7 @@
>  # Mapping	<name_EL1>
>  # EndSysreg
>  
> -# Where multiple system regsiters are not VHE aliases but share a
> +# Where multiple system registers are not VHE aliases but share a
>  # common layout, a SysregFields block can be used to describe the
>  # shared layout:
>  
> @@ -54,7 +54,7 @@
>  #
>  # In general it is recommended that new enumeration items be named for the
>  # feature that introduces them (eg, FEAT_LS64_ACCDATA introduces enumeration
> -# item ACCDATA) though it may be more taseful to do something else.
> +# item ACCDATA) though it may be more tasteful to do something else.
>  
>  Sysreg	OSDTRRX_EL1	2	0	0	0	2
>  Res0	63:32
> @@ -474,7 +474,7 @@ EndEnum
>  Enum	7:4	Security
>  	0b0000	NI
>  	0b0001	EL3
> -	0b0001	NSACR_RFR
> +	0b0010	NSACR_RFR
>  EndEnum
>  UnsignedEnum	3:0	ProgMod
>  	0b0000	NI
> @@ -2528,10 +2528,6 @@ Field	17:16	ZEN
>  Res0	15:0
>  EndSysreg
>  
> -Sysreg	CPACR_EL12      3	5	1	0	2
> -Mapping	CPACR_EL1
> -EndSysreg
> -
>  Sysreg	CPACRALIAS_EL1  3	0	1	4	4
>  Mapping	CPACR_EL1
>  EndSysreg
> @@ -2576,10 +2572,6 @@ Sysreg	PFAR_EL12	3	5	6	0	5
>  Mapping	PFAR_EL1
>  EndSysreg
>  
> -Sysreg	RCWSMASK_EL1	3	0	13	0	3
> -Field	63:0	RCWSMASK
> -EndSysreg
> -
>  Sysreg	SCTLR2_EL1      3	0	1	0	3
>  Res0    63:13
>  Field   12      CPTM0
> -- 
> 2.51.0.338.gd7d06c2dae-goog
> 
> 

[PATCH v3 2/3] arm64: sysreg: Correct sign definitions for EIESB and DoubleLock

[Fuad Tabba]

The `ID_AA64MMFR4_EL1.EIESB` field, is an unsigned enumeration, but was
incorrectly defined as a `SignedEnum` when introduced in commit
cfc680bb04c5 ("arm64: sysreg: Add layout for ID_AA64MMFR4_EL1"). This is
corrected to `UnsignedEnum`.

Conversely, the `ID_AA64DFR0_EL1.DoubleLock` field, is a signed
enumeration, but was incorrectly defined as an `UnsignedEnum`. This is
corrected to `SignedEnum`, which wasn't correctly set when annotated as
such in commit ad16d4cf0b4f ("arm64/sysreg: Initial unsigned annotations
for ID registers").

Signed-off-by: Fuad Tabba <tabba@google.com>
---
Note: No active users in the kernel for these two fields yet. pKVM
checks DoubleLock, but only for protected VMs, which aren't enabled
upstream yet. Hence no Fixes tag.
---
 arch/arm64/tools/sysreg | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index f1a012ee0db6..d396fa587ec1 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -1693,7 +1693,7 @@ UnsignedEnum	43:40	TraceFilt
 	0b0000	NI
 	0b0001	IMP
 EndEnum
-UnsignedEnum	39:36	DoubleLock
+SignedEnum	39:36	DoubleLock
 	0b0000	IMP
 	0b1111	NI
 EndEnum
@@ -2409,7 +2409,7 @@ UnsignedEnum	11:8	ASID2
 	0b0000	NI
 	0b0001	IMP
 EndEnum
-SignedEnum	7:4	EIESB
+UnsignedEnum	7:4	EIESB
 	0b0000	NI
 	0b0001	ToEL3
 	0b0010	ToELx
-- 
2.51.0.338.gd7d06c2dae-goog

Re: [PATCH v3 2/3] arm64: sysreg: Correct sign definitions for EIESB and DoubleLock

[Mark Rutland]

On Fri, Aug 29, 2025 at 10:51:42AM +0100, Fuad Tabba wrote:
> The `ID_AA64MMFR4_EL1.EIESB` field, is an unsigned enumeration, but was
> incorrectly defined as a `SignedEnum` when introduced in commit
> cfc680bb04c5 ("arm64: sysreg: Add layout for ID_AA64MMFR4_EL1"). This is
> corrected to `UnsignedEnum`.
> 
> Conversely, the `ID_AA64DFR0_EL1.DoubleLock` field, is a signed
> enumeration, but was incorrectly defined as an `UnsignedEnum`. This is
> corrected to `SignedEnum`, which wasn't correctly set when annotated as
> such in commit ad16d4cf0b4f ("arm64/sysreg: Initial unsigned annotations
> for ID registers").
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
> Note: No active users in the kernel for these two fields yet. pKVM
> checks DoubleLock, but only for protected VMs, which aren't enabled
> upstream yet. Hence no Fixes tag.

I have no strong feelings either way on that, so FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/tools/sysreg | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
> index f1a012ee0db6..d396fa587ec1 100644
> --- a/arch/arm64/tools/sysreg
> +++ b/arch/arm64/tools/sysreg
> @@ -1693,7 +1693,7 @@ UnsignedEnum	43:40	TraceFilt
>  	0b0000	NI
>  	0b0001	IMP
>  EndEnum
> -UnsignedEnum	39:36	DoubleLock
> +SignedEnum	39:36	DoubleLock
>  	0b0000	IMP
>  	0b1111	NI
>  EndEnum
> @@ -2409,7 +2409,7 @@ UnsignedEnum	11:8	ASID2
>  	0b0000	NI
>  	0b0001	IMP
>  EndEnum
> -SignedEnum	7:4	EIESB
> +UnsignedEnum	7:4	EIESB
>  	0b0000	NI
>  	0b0001	ToEL3
>  	0b0010	ToELx
> -- 
> 2.51.0.338.gd7d06c2dae-goog
> 
> 

[PATCH v3 3/3] arm64: sysreg: Add validation checks to sysreg header generation script

[Fuad Tabba]

The gen_sysreg.awk script processes the system register specification in
the sysreg text file to generate C macro definitions. The current script
will silently accept certain errors in the specification file, leading
to incorrect header generation.

For example, a Sysreg or SysregFields can be accidentally duplicated,
causing its macros to be emitted twice. An Enum can contain duplicate
values for different items, which is architecturally incorrect.

Add checks to catch these errors at build time. The script now tracks
all seen Sysreg and SysregFields definitions and checks for duplicates.
It also tracks values within each Enum block to ensure entries are
unique.

Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/tools/gen-sysreg.awk | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk
index f2a1732cb1f6..bbbb812603e8 100755
--- a/arch/arm64/tools/gen-sysreg.awk
+++ b/arch/arm64/tools/gen-sysreg.awk
@@ -122,6 +122,10 @@ $1 == "SysregFields" && block_current() == "Root" {
 	res1 = "UL(0)"
 	unkn = "UL(0)"
 
+	if (reg in defined_fields)
+		fatal("Duplicate SysregFields definition for " reg)
+	defined_fields[reg] = 1
+
 	next_bit = 63
 
 	next
@@ -162,6 +166,10 @@ $1 == "Sysreg" && block_current() == "Root" {
 	res1 = "UL(0)"
 	unkn = "UL(0)"
 
+	if (reg in defined_regs)
+		fatal("Duplicate Sysreg definition for " reg)
+	defined_regs[reg] = 1
+
 	define("REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_" op2)
 	define("SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", " crm ", " op2 ")")
 
@@ -284,6 +292,8 @@ $1 == "SignedEnum" && (block_current() == "Sysreg" || block_current() == "Sysreg
 	define_field(reg, field, msb, lsb)
 	define_field_sign(reg, field, "true")
 
+	delete seen_enum_vals
+
 	next
 }
 
@@ -297,6 +307,8 @@ $1 == "UnsignedEnum" && (block_current() == "Sysreg" || block_current() == "Sysr
 	define_field(reg, field, msb, lsb)
 	define_field_sign(reg, field, "false")
 
+	delete seen_enum_vals
+
 	next
 }
 
@@ -309,6 +321,8 @@ $1 == "Enum" && (block_current() == "Sysreg" || block_current() == "SysregFields
 
 	define_field(reg, field, msb, lsb)
 
+	delete seen_enum_vals
+
 	next
 }
 
@@ -320,6 +334,8 @@ $1 == "EndEnum" && block_current() == "Enum" {
 	lsb = null
 	print ""
 
+	delete seen_enum_vals
+
 	block_pop()
 	next
 }
@@ -329,6 +345,10 @@ $1 == "EndEnum" && block_current() == "Enum" {
 	val = $1
 	name = $2
 
+	if (val in seen_enum_vals)
+		fatal("Duplicate Enum value " val " for " name)
+	seen_enum_vals[val] = 1
+
 	define(reg "_" field "_" name, "UL(" val ")")
 	next
 }
-- 
2.51.0.338.gd7d06c2dae-goog

Re: [PATCH v3 3/3] arm64: sysreg: Add validation checks to sysreg header generation script

[Mark Rutland]

On Fri, Aug 29, 2025 at 10:51:43AM +0100, Fuad Tabba wrote:
> The gen_sysreg.awk script processes the system register specification in
> the sysreg text file to generate C macro definitions. The current script
> will silently accept certain errors in the specification file, leading
> to incorrect header generation.
> 
> For example, a Sysreg or SysregFields can be accidentally duplicated,
> causing its macros to be emitted twice. An Enum can contain duplicate
> values for different items, which is architecturally incorrect.
> 
> Add checks to catch these errors at build time. The script now tracks
> all seen Sysreg and SysregFields definitions and checks for duplicates.
> It also tracks values within each Enum block to ensure entries are
> unique.
> 
> Acked-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Fuad Tabba <tabba@google.com>

I wonder if it would make sense to treat SysregFields and Sysregs as one
group (both using defined_regs), so that we can catch:

	Sysreg	FOO_EL1	...
		...
	EndSysreg

	SysregFields	FOO_EL1
		...
	EndSysregFields

I have no strong feelings either way, so for this as-is OR for using
`defined_regs` for both:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/tools/gen-sysreg.awk | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk
> index f2a1732cb1f6..bbbb812603e8 100755
> --- a/arch/arm64/tools/gen-sysreg.awk
> +++ b/arch/arm64/tools/gen-sysreg.awk
> @@ -122,6 +122,10 @@ $1 == "SysregFields" && block_current() == "Root" {
>  	res1 = "UL(0)"
>  	unkn = "UL(0)"
>  
> +	if (reg in defined_fields)
> +		fatal("Duplicate SysregFields definition for " reg)
> +	defined_fields[reg] = 1
> +
>  	next_bit = 63
>  
>  	next
> @@ -162,6 +166,10 @@ $1 == "Sysreg" && block_current() == "Root" {
>  	res1 = "UL(0)"
>  	unkn = "UL(0)"
>  
> +	if (reg in defined_regs)
> +		fatal("Duplicate Sysreg definition for " reg)
> +	defined_regs[reg] = 1
> +
>  	define("REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_" op2)
>  	define("SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", " crm ", " op2 ")")
>  
> @@ -284,6 +292,8 @@ $1 == "SignedEnum" && (block_current() == "Sysreg" || block_current() == "Sysreg
>  	define_field(reg, field, msb, lsb)
>  	define_field_sign(reg, field, "true")
>  
> +	delete seen_enum_vals
> +
>  	next
>  }
>  
> @@ -297,6 +307,8 @@ $1 == "UnsignedEnum" && (block_current() == "Sysreg" || block_current() == "Sysr
>  	define_field(reg, field, msb, lsb)
>  	define_field_sign(reg, field, "false")
>  
> +	delete seen_enum_vals
> +
>  	next
>  }
>  
> @@ -309,6 +321,8 @@ $1 == "Enum" && (block_current() == "Sysreg" || block_current() == "SysregFields
>  
>  	define_field(reg, field, msb, lsb)
>  
> +	delete seen_enum_vals
> +
>  	next
>  }
>  
> @@ -320,6 +334,8 @@ $1 == "EndEnum" && block_current() == "Enum" {
>  	lsb = null
>  	print ""
>  
> +	delete seen_enum_vals
> +
>  	block_pop()
>  	next
>  }
> @@ -329,6 +345,10 @@ $1 == "EndEnum" && block_current() == "Enum" {
>  	val = $1
>  	name = $2
>  
> +	if (val in seen_enum_vals)
> +		fatal("Duplicate Enum value " val " for " name)
> +	seen_enum_vals[val] = 1
> +
>  	define(reg "_" field "_" name, "UL(" val ")")
>  	next
>  }
> -- 
> 2.51.0.338.gd7d06c2dae-goog
> 
>