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

[Mark Rutland <mark.rutland@arm.com>]

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
> 
>