linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] arm64: sysreg: Fix sysreg field definitions and generation script
@ 2025-08-13 16:45 Fuad Tabba
  2025-08-13 16:45 ` [PATCH v2 1/2] arm64: sysreg: Fix and tidyup sysreg field definitions Fuad Tabba
  2025-08-13 16:45 ` [PATCH v2 2/2] arm64: sysreg: Add validation checks to sysreg header generation script Fuad Tabba
  0 siblings, 2 replies; 5+ messages in thread
From: Fuad Tabba @ 2025-08-13 16:45 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: maz, oliver.upton, will, joey.gouly, suzuki.poulose, yuzenghui,
	catalin.marinas, vdonnefort, qperret, sebastianene, keirf,
	smostafa, tabba

The sysreg file has an erroronous value in the Security Enum, where
NSACR_RFR is set to 0b0001 instead of the spec value of 0b0010.
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 header file and could be a source of future
bugs.

Fix the Enum value, remove the redundant Sysreg definitions, and while
at it, tidy up the sysreg file a bit.

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 the errors that
I've seen, which also are errors that wouldn't generate a build error
later on.

Changes since v1 [1]:
- Remove redundant definition of CPACR_EL12
- New patch that adds validation to the sysreg header generation script
- Rebase on 6.17-rc1

Cheers,
/fuad

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

Fuad Tabba (2):
  arm64: sysreg: Fix and tidyup sysreg field definitions
  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: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.51.0.rc0.215.g125493bb4a-goog



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/2] arm64: sysreg: Fix and tidyup sysreg field definitions
  2025-08-13 16:45 [PATCH v2 0/2] arm64: sysreg: Fix sysreg field definitions and generation script Fuad Tabba
@ 2025-08-13 16:45 ` Fuad Tabba
  2025-08-14  7:13   ` Marc Zyngier
  2025-08-13 16:45 ` [PATCH v2 2/2] arm64: sysreg: Add validation checks to sysreg header generation script Fuad Tabba
  1 sibling, 1 reply; 5+ messages in thread
From: Fuad Tabba @ 2025-08-13 16:45 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: maz, oliver.upton, will, joey.gouly, suzuki.poulose, yuzenghui,
	catalin.marinas, vdonnefort, qperret, sebastianene, keirf,
	smostafa, 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, fix typos, and convert the only instance of spacing with
spaces to tabs (in SPMZR_EL0).

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 | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 696ab1f32a67..975dcfacbfe6 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
@@ -377,8 +377,8 @@ Sysreg	SPMOVSCLR_EL0	2	3	9	12	3
 Field	63:0	P
 EndSysreg
 
-Sysreg	SPMZR_EL0       2	3	9	12	4
-Field   63:0      P
+Sysreg	SPMZR_EL0	2	3	9	12	4
+Field   63:0	P
 EndSysreg
 
 Sysreg	SPMSELR_EL0	2	3	9	12	5
@@ -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.rc0.215.g125493bb4a-goog



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] arm64: sysreg: Add validation checks to sysreg header generation script
  2025-08-13 16:45 [PATCH v2 0/2] arm64: sysreg: Fix sysreg field definitions and generation script Fuad Tabba
  2025-08-13 16:45 ` [PATCH v2 1/2] arm64: sysreg: Fix and tidyup sysreg field definitions Fuad Tabba
@ 2025-08-13 16:45 ` Fuad Tabba
  2025-08-14  7:16   ` Marc Zyngier
  1 sibling, 1 reply; 5+ messages in thread
From: Fuad Tabba @ 2025-08-13 16:45 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: maz, oliver.upton, will, joey.gouly, suzuki.poulose, yuzenghui,
	catalin.marinas, vdonnefort, qperret, sebastianene, keirf,
	smostafa, 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. Or 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.

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.rc0.215.g125493bb4a-goog



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/2] arm64: sysreg: Fix and tidyup sysreg field definitions
  2025-08-13 16:45 ` [PATCH v2 1/2] arm64: sysreg: Fix and tidyup sysreg field definitions Fuad Tabba
@ 2025-08-14  7:13   ` Marc Zyngier
  0 siblings, 0 replies; 5+ messages in thread
From: Marc Zyngier @ 2025-08-14  7:13 UTC (permalink / raw)
  To: Fuad Tabba
  Cc: kvmarm, linux-arm-kernel, oliver.upton, will, joey.gouly,
	suzuki.poulose, yuzenghui, catalin.marinas, vdonnefort, qperret,
	sebastianene, keirf, smostafa

On Wed, 13 Aug 2025 17:45:05 +0100,
Fuad Tabba <tabba@google.com> 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, fix typos, and convert the only instance of spacing with
> spaces to tabs (in SPMZR_EL0).
> 
> 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.

Seems reasonable.

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] arm64: sysreg: Add validation checks to sysreg header generation script
  2025-08-13 16:45 ` [PATCH v2 2/2] arm64: sysreg: Add validation checks to sysreg header generation script Fuad Tabba
@ 2025-08-14  7:16   ` Marc Zyngier
  0 siblings, 0 replies; 5+ messages in thread
From: Marc Zyngier @ 2025-08-14  7:16 UTC (permalink / raw)
  To: Fuad Tabba
  Cc: kvmarm, linux-arm-kernel, oliver.upton, will, joey.gouly,
	suzuki.poulose, yuzenghui, catalin.marinas, vdonnefort, qperret,
	sebastianene, keirf, smostafa

On Wed, 13 Aug 2025 17:45:06 +0100,
Fuad Tabba <tabba@google.com> 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. Or 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.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>

Thanks for adding these checks, most useful.

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-08-14  9:46 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-13 16:45 [PATCH v2 0/2] arm64: sysreg: Fix sysreg field definitions and generation script Fuad Tabba
2025-08-13 16:45 ` [PATCH v2 1/2] arm64: sysreg: Fix and tidyup sysreg field definitions Fuad Tabba
2025-08-14  7:13   ` Marc Zyngier
2025-08-13 16:45 ` [PATCH v2 2/2] arm64: sysreg: Add validation checks to sysreg header generation script Fuad Tabba
2025-08-14  7:16   ` Marc Zyngier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).