[PATCH 0/3] arm64/sysreg: Introduce Feat descriptor and generated ICH_VMCR_EL2 support

[PATCH 0/3] arm64/sysreg: Introduce Feat descriptor and generated ICH_VMCR_EL2 support

[Sascha Bischoff]

This series introduces support for feature-specific field encodings in
the sysreg description framework and migrates the vGIC-v3 code to use
generated definitions for ICH_VMCR_EL2, in part as an example of how
the Feat descriptor can be used.

The first patch adds the Feat descriptor, allowing sysreg definitions
to vary based on architectural features (e.g., GICv3 vs GICv5). The
Feat/ElseFeat/EndFeat construct enables generation of feature-prefixed
field encodings without affecting legacy definitions. This forms the
basis for supporting feature-dependent register layouts.

The second patch adds the generated description for ICH_VMCR_EL2,
including both its GICv3 and GICv5 variants. This register was
previously defined manually in the KVM GICv3 code; moving it into the
sysreg framework ensures consistency and reduces duplication.

Finally, the third patch updates the KVM vGIC-v3 implementation to use
the generated ICH_VMCR_EL2 definitions. This replaces and removes the
hand-written definitions, with no functional change to behaviour.

Together, these patches complete the migration of ICH_VMCR_EL2 to the
sysreg framework and establish the infrastructure needed to describe
registers whose field layouts depend on architectural features.

Thanks,
Sascha

Sascha Bischoff (3):
  arm64/sysreg: Support feature-specific fields with 'Feat' descriptor
  arm64/sysreg: Add ICH_VMCR_EL2
  KVM: arm64: gic-v3: Switch vGIC-v3 to use generated ICH_VMCR_EL2

 arch/arm64/include/asm/sysreg.h      |  21 ----
 arch/arm64/kvm/hyp/vgic-v3-sr.c      |  64 +++++-------
 arch/arm64/kvm/vgic/vgic-v3-nested.c |   8 +-
 arch/arm64/kvm/vgic/vgic-v3.c        |  42 ++++----
 arch/arm64/tools/gen-sysreg.awk      | 148 +++++++++++++++++++--------
 arch/arm64/tools/sysreg              |  22 ++++
 6 files changed, 177 insertions(+), 128 deletions(-)

-- 
2.34.1

[PATCH 1/3] arm64/sysreg: Support feature-specific fields with 'Feat' descriptor

[Sascha Bischoff]

Some system register field encodings change based on the available and
in-use architecture features. In order to support these different
field encodings, introduce the Feat descriptor (Feat, ElseFeat,
EndFeat) for describing such sysregs.

The Feat descriptor can be used in the following way (Feat acts as
both an if and an else-if):

        Sysreg  EXAMPLE 0    1    2    3    4
        Feat    FEAT_A
	Field   63:0    Foo
	Feat    FEAT_B
	Field   63:1    Bar
 	Res0    0
        ElseFeat
        Field   63:0    Baz
        EndFeat
        EndSysreg

This will generate a single set of system register encodings (REG_,
SYS_, ...), and then generate three sets of field definitions for the
system register called EXAMPLE. The first set is prefixed by FEAT_A,
e.g. FEAT_A_EXAMPLE_Foo. The second set is prefixed by FEAT_B, e.g.,
FEAT_B_EXAMPLE_Bar. The third set is not given a prefix at all,
e.g. EXAMPLE_BAZ. For each set, a corresponding set of defines for
Res0, Res1, and Unkn is generated.

The intent for the final prefix-less ElseFeat is for describing
default or legacy field encodings. This ensure that new
feature-conditional encodings can be added to already-present sysregs
without affecting existing legacy code.

The Feat descriptor can be used within Sysreg or SysregFields
blocks. Field, Res0, Res1, Unkn, Rax, SignedEnum, Enum can all be used
within a Feat block. Fields and Mapping can not. Fields that vary with
features must be described as part of a SysregFields block,
instead. Mappings, which are just a code comment, make little sense in
this context, and have hence not been included.

There are no changes to the generated system register definitions as
part of this change.

Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
---
 arch/arm64/tools/gen-sysreg.awk | 148 ++++++++++++++++++++++----------
 1 file changed, 104 insertions(+), 44 deletions(-)

diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk
index f2a1732cb1f63..c1bb6d5087a99 100755
--- a/arch/arm64/tools/gen-sysreg.awk
+++ b/arch/arm64/tools/gen-sysreg.awk
@@ -44,23 +44,35 @@ function expect_fields(nf) {
 
 # Print a CPP macro definition, padded with spaces so that the macro bodies
 # line up in a column
-function define(name, val) {
-	printf "%-56s%s\n", "#define " name, val
+function define(feat, name, val) {
+	printf "%-56s%s\n", "#define " feat name, val
 }
 
 # Print standard BITMASK/SHIFT/WIDTH CPP definitions for a field
-function define_field(reg, field, msb, lsb) {
-	define(reg "_" field, "GENMASK(" msb ", " lsb ")")
-	define(reg "_" field "_MASK", "GENMASK(" msb ", " lsb ")")
-	define(reg "_" field "_SHIFT", lsb)
-	define(reg "_" field "_WIDTH", msb - lsb + 1)
+function define_field(feat, reg, field, msb, lsb) {
+	define(feat, reg "_" field, "GENMASK(" msb ", " lsb ")")
+	define(feat, reg "_" field "_MASK", "GENMASK(" msb ", " lsb ")")
+	define(feat, reg "_" field "_SHIFT", lsb)
+	define(feat, reg "_" field "_WIDTH", msb - lsb + 1)
 }
 
 # Print a field _SIGNED definition for a field
-function define_field_sign(reg, field, sign) {
-	define(reg "_" field "_SIGNED", sign)
+function define_field_sign(feat, reg, field, sign) {
+	define(feat, reg "_" field "_SIGNED", sign)
 }
 
+# Print the Res0, Res1, Unkn masks
+function define_resx_unkn(feat, reg, res0, res1, unkn) {
+	if (res0 != null)
+		define(feat, reg "_RES0", "(" res0 ")")
+	if (res1 != null)
+		define(feat, reg "_RES1", "(" res1 ")")
+	if (unkn != null)
+		define(feat, reg "_UNKN", "(" unkn ")")
+	if (res0 != null || res1 != null || unkn != null)
+		print ""
+	}
+
 # Parse a "<msb>[:<lsb>]" string into the global variables @msb and @lsb
 function parse_bitdef(reg, field, bitdef, _bits)
 {
@@ -132,10 +144,7 @@ $1 == "EndSysregFields" && block_current() == "SysregFields" {
 	if (next_bit > 0)
 		fatal("Unspecified bits in " reg)
 
-	define(reg "_RES0", "(" res0 ")")
-	define(reg "_RES1", "(" res1 ")")
-	define(reg "_UNKN", "(" unkn ")")
-	print ""
+	define_resx_unkn(feat, reg, res0, res1, unkn)
 
 	reg = null
 	res0 = null
@@ -162,14 +171,16 @@ $1 == "Sysreg" && block_current() == "Root" {
 	res1 = "UL(0)"
 	unkn = "UL(0)"
 
-	define("REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_" op2)
-	define("SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", " crm ", " op2 ")")
+	feat = null
+
+	define(feat, "REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_" op2)
+	define(feat, "SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", " crm ", " op2 ")")
 
-	define("SYS_" reg "_Op0", op0)
-	define("SYS_" reg "_Op1", op1)
-	define("SYS_" reg "_CRn", crn)
-	define("SYS_" reg "_CRm", crm)
-	define("SYS_" reg "_Op2", op2)
+	define(feat, "SYS_" reg "_Op0", op0)
+	define(feat, "SYS_" reg "_Op1", op1)
+	define(feat, "SYS_" reg "_CRn", crn)
+	define(feat, "SYS_" reg "_CRm", crm)
+	define(feat, "SYS_" reg "_Op2", op2)
 
 	print ""
 
@@ -183,14 +194,7 @@ $1 == "EndSysreg" && block_current() == "Sysreg" {
 	if (next_bit > 0)
 		fatal("Unspecified bits in " reg)
 
-	if (res0 != null)
-		define(reg "_RES0", "(" res0 ")")
-	if (res1 != null)
-		define(reg "_RES1", "(" res1 ")")
-	if (unkn != null)
-		define(reg "_UNKN", "(" unkn ")")
-	if (res0 != null || res1 != null || unkn != null)
-		print ""
+	define_resx_unkn(feat, reg, res0, res1, unkn)
 
 	reg = null
 	op0 = null
@@ -201,6 +205,7 @@ $1 == "EndSysreg" && block_current() == "Sysreg" {
 	res0 = null
 	res1 = null
 	unkn = null
+	feat = null
 
 	block_pop()
 	next
@@ -225,8 +230,7 @@ $1 == "EndSysreg" && block_current() == "Sysreg" {
 	next
 }
 
-
-$1 == "Res0" && (block_current() == "Sysreg" || block_current() == "SysregFields") {
+$1 == "Res0" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
 	expect_fields(2)
 	parse_bitdef(reg, "RES0", $2)
 	field = "RES0_" msb "_" lsb
@@ -236,7 +240,7 @@ $1 == "Res0" && (block_current() == "Sysreg" || block_current() == "SysregFields
 	next
 }
 
-$1 == "Res1" && (block_current() == "Sysreg" || block_current() == "SysregFields") {
+$1 == "Res1" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
 	expect_fields(2)
 	parse_bitdef(reg, "RES1", $2)
 	field = "RES1_" msb "_" lsb
@@ -246,7 +250,7 @@ $1 == "Res1" && (block_current() == "Sysreg" || block_current() == "SysregFields
 	next
 }
 
-$1 == "Unkn" && (block_current() == "Sysreg" || block_current() == "SysregFields") {
+$1 == "Unkn" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
 	expect_fields(2)
 	parse_bitdef(reg, "UNKN", $2)
 	field = "UNKN_" msb "_" lsb
@@ -256,58 +260,58 @@ $1 == "Unkn" && (block_current() == "Sysreg" || block_current() == "SysregFields
 	next
 }
 
-$1 == "Field" && (block_current() == "Sysreg" || block_current() == "SysregFields") {
+$1 == "Field" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
 	expect_fields(3)
 	field = $3
 	parse_bitdef(reg, field, $2)
 
-	define_field(reg, field, msb, lsb)
+	define_field(feat, reg, field, msb, lsb)
 	print ""
 
 	next
 }
 
-$1 == "Raz" && (block_current() == "Sysreg" || block_current() == "SysregFields") {
+$1 == "Raz" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
 	expect_fields(2)
 	parse_bitdef(reg, field, $2)
 
 	next
 }
 
-$1 == "SignedEnum" && (block_current() == "Sysreg" || block_current() == "SysregFields") {
+$1 == "SignedEnum" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
 	block_push("Enum")
 
 	expect_fields(3)
 	field = $3
 	parse_bitdef(reg, field, $2)
 
-	define_field(reg, field, msb, lsb)
-	define_field_sign(reg, field, "true")
+	define_field(feat, reg, field, msb, lsb)
+	define_field_sign(feat, reg, field, "true")
 
 	next
 }
 
-$1 == "UnsignedEnum" && (block_current() == "Sysreg" || block_current() == "SysregFields") {
+$1 == "UnsignedEnum" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
 	block_push("Enum")
 
 	expect_fields(3)
 	field = $3
 	parse_bitdef(reg, field, $2)
 
-	define_field(reg, field, msb, lsb)
-	define_field_sign(reg, field, "false")
+	define_field(feat, reg, field, msb, lsb)
+	define_field_sign(feat, reg, field, "false")
 
 	next
 }
 
-$1 == "Enum" && (block_current() == "Sysreg" || block_current() == "SysregFields") {
+$1 == "Enum" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
 	block_push("Enum")
 
 	expect_fields(3)
 	field = $3
 	parse_bitdef(reg, field, $2)
 
-	define_field(reg, field, msb, lsb)
+	define_field(feat, reg, field, msb, lsb)
 
 	next
 }
@@ -329,7 +333,63 @@ $1 == "EndEnum" && block_current() == "Enum" {
 	val = $1
 	name = $2
 
-	define(reg "_" field "_" name, "UL(" val ")")
+	define(feat, reg "_" field "_" name, "UL(" val ")")
+	next
+}
+
+$1 == "Feat" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {
+	# Don't push a new block if we're already in a Feat
+	# block. This is to support constructs such as:
+	#	Feat FEAT_A
+	#	...
+	#	Feat FEAT_B
+	#	...
+	#	ElseFeat
+	#	...
+	#	EndFeat
+	if (block_current() != "Feat")
+		block_push("Feat")
+	else
+		define_resx_unkn(feat, reg, res0, res1, unkn)
+
+	expect_fields(2)
+	feat = $2 "_"
+
+	next_bit = 63
+
+	res0 = "UL(0)"
+	res1 = "UL(0)"
+	unkn = "UL(0)"
+
+	next
+}
+
+$1 == "ElseFeat" && block_current() == "Feat" {
+	expect_fields(1)
+
+	define_resx_unkn(feat, reg, res0, res1, unkn)
+
+	res0 = "UL(0)"
+	res1 = "UL(0)"
+	unkn = "UL(0)"
+	feat = null
+	next_bit = 63
+
+	next
+}
+
+$1 == "EndFeat" && block_current() == "Feat" {
+	expect_fields(1)
+
+	define_resx_unkn(feat, reg, res0, res1, unkn)
+
+	res0 = null
+	res1 = null
+	unkn = null
+	feat = null
+
+	block_pop()
+
 	next
 }
 
-- 
2.34.1

[PATCH 2/3] arm64/sysreg: Add ICH_VMCR_EL2

[Sascha Bischoff]

Add the ICH_VMCR_EL2 register, which is required for the upcoming
GICv5 KVM support. This register has two different field encodings,
based on if it is used for GICv3 or GICv5-based VMs. The
GICv5-specific field encodings are generated with a FEAT_GCIE prefix.

This register is already described in the GICv3 KVM code
directly. This will be ported across to use the generated encodings as
part of an upcoming change.

Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
---
 arch/arm64/tools/sysreg | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 696ab1f32a674..a2bf8a689f968 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -4668,6 +4668,28 @@ Field	1	V3
 Field	0	En
 EndSysreg
 
+Sysreg	ICH_VMCR_EL2	3	4	12	11	7
+Feat	FEAT_GCIE
+Res0	63:32
+Field	31:27	VPMR
+Res0	26:1
+Field	0	EN
+ElseFeat
+Res0	63:32
+Field	31:24	VPMR
+Field	23:21	VBPR0
+Field	20:18	VBPR1
+Res0	17:10
+Field	9	VEOIM
+Res0	8:5
+Field	4	VCBPR
+Field	3	VFIQEn
+Field	2	VAckCtl
+Field	1	VENG1
+Field	0	VENG0
+EndFeat
+EndSysreg
+
 Sysreg	CONTEXTIDR_EL2	3	4	13	0	1
 Fields	CONTEXTIDR_ELx
 EndSysreg
-- 
2.34.1

[PATCH 3/3] KVM: arm64: gic-v3: Switch vGIC-v3 to use generated ICH_VMCR_EL2

[Sascha Bischoff]

The VGIC-v3 code relied on hand-written definitions for the
ICH_VMCR_EL2 register. This register, and the associated fields, is
now generated as part of the sysreg framework. Move to using the
generated definitions instead of the hand-written ones.

There are no functional changes as part of this change.

Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
---
 arch/arm64/include/asm/sysreg.h      | 21 ---------
 arch/arm64/kvm/hyp/vgic-v3-sr.c      | 64 ++++++++++++----------------
 arch/arm64/kvm/vgic/vgic-v3-nested.c |  8 ++--
 arch/arm64/kvm/vgic/vgic-v3.c        | 42 +++++++++---------
 4 files changed, 51 insertions(+), 84 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 6604fd6f33f45..06bc0e628b03e 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -571,7 +571,6 @@
 #define SYS_ICC_SRE_EL2			sys_reg(3, 4, 12, 9, 5)
 #define SYS_ICH_EISR_EL2		sys_reg(3, 4, 12, 11, 3)
 #define SYS_ICH_ELRSR_EL2		sys_reg(3, 4, 12, 11, 5)
-#define SYS_ICH_VMCR_EL2		sys_reg(3, 4, 12, 11, 7)
 
 #define __SYS__LR0_EL2(x)		sys_reg(3, 4, 12, 12, x)
 #define SYS_ICH_LR0_EL2			__SYS__LR0_EL2(0)
@@ -999,26 +998,6 @@
 #define ICH_LR_PRIORITY_SHIFT	48
 #define ICH_LR_PRIORITY_MASK	(0xffULL << ICH_LR_PRIORITY_SHIFT)
 
-/* ICH_VMCR_EL2 bit definitions */
-#define ICH_VMCR_ACK_CTL_SHIFT	2
-#define ICH_VMCR_ACK_CTL_MASK	(1 << ICH_VMCR_ACK_CTL_SHIFT)
-#define ICH_VMCR_FIQ_EN_SHIFT	3
-#define ICH_VMCR_FIQ_EN_MASK	(1 << ICH_VMCR_FIQ_EN_SHIFT)
-#define ICH_VMCR_CBPR_SHIFT	4
-#define ICH_VMCR_CBPR_MASK	(1 << ICH_VMCR_CBPR_SHIFT)
-#define ICH_VMCR_EOIM_SHIFT	9
-#define ICH_VMCR_EOIM_MASK	(1 << ICH_VMCR_EOIM_SHIFT)
-#define ICH_VMCR_BPR1_SHIFT	18
-#define ICH_VMCR_BPR1_MASK	(7 << ICH_VMCR_BPR1_SHIFT)
-#define ICH_VMCR_BPR0_SHIFT	21
-#define ICH_VMCR_BPR0_MASK	(7 << ICH_VMCR_BPR0_SHIFT)
-#define ICH_VMCR_PMR_SHIFT	24
-#define ICH_VMCR_PMR_MASK	(0xffUL << ICH_VMCR_PMR_SHIFT)
-#define ICH_VMCR_ENG0_SHIFT	0
-#define ICH_VMCR_ENG0_MASK	(1 << ICH_VMCR_ENG0_SHIFT)
-#define ICH_VMCR_ENG1_SHIFT	1
-#define ICH_VMCR_ENG1_MASK	(1 << ICH_VMCR_ENG1_SHIFT)
-
 /*
  * Permission Indirection Extension (PIE) permission encodings.
  * Encodings with the _O suffix, have overlays applied (Permission Overlay Extension).
diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index d81275790e69b..c90608daa5b2b 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -567,11 +567,11 @@ static int __vgic_v3_highest_priority_lr(struct kvm_vcpu *vcpu, u32 vmcr,
 			continue;
 
 		/* Group-0 interrupt, but Group-0 disabled? */
-		if (!(val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_ENG0_MASK))
+		if (!(val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_EL2_VENG0_MASK))
 			continue;
 
 		/* Group-1 interrupt, but Group-1 disabled? */
-		if ((val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_ENG1_MASK))
+		if ((val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_EL2_VENG1_MASK))
 			continue;
 
 		/* Not the highest priority? */
@@ -644,19 +644,19 @@ static int __vgic_v3_get_highest_active_priority(void)
 
 static unsigned int __vgic_v3_get_bpr0(u32 vmcr)
 {
-	return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+	return FIELD_GET(ICH_VMCR_EL2_VBPR0, vmcr);
 }
 
 static unsigned int __vgic_v3_get_bpr1(u32 vmcr)
 {
 	unsigned int bpr;
 
-	if (vmcr & ICH_VMCR_CBPR_MASK) {
+	if (vmcr & ICH_VMCR_EL2_VCBPR_MASK) {
 		bpr = __vgic_v3_get_bpr0(vmcr);
 		if (bpr < 7)
 			bpr++;
 	} else {
-		bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+		bpr = FIELD_GET(ICH_VMCR_EL2_VBPR1, vmcr);
 	}
 
 	return bpr;
@@ -756,7 +756,7 @@ static void __vgic_v3_read_iar(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	if (grp != !!(lr_val & ICH_LR_GROUP))
 		goto spurious;
 
-	pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT;
+	pmr = FIELD_GET(ICH_VMCR_EL2_VPMR, vmcr);
 	lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
 	if (pmr <= lr_prio)
 		goto spurious;
@@ -804,7 +804,7 @@ static void __vgic_v3_write_dir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	int lr;
 
 	/* EOImode == 0, nothing to be done here */
-	if (!(vmcr & ICH_VMCR_EOIM_MASK))
+	if (!FIELD_GET(ICH_VMCR_EL2_VEOIM_MASK, vmcr))
 		return;
 
 	/* No deactivate to be performed on an LPI */
@@ -841,7 +841,7 @@ static void __vgic_v3_write_eoir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	}
 
 	/* EOImode == 1 and not an LPI, nothing to be done here */
-	if ((vmcr & ICH_VMCR_EOIM_MASK) && !(vid >= VGIC_MIN_LPI))
+	if (FIELD_GET(ICH_VMCR_EL2_VEOIM_MASK, vmcr) && !(vid >= VGIC_MIN_LPI))
 		return;
 
 	lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
@@ -857,12 +857,12 @@ static void __vgic_v3_write_eoir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 
 static void __vgic_v3_read_igrpen0(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 {
-	vcpu_set_reg(vcpu, rt, !!(vmcr & ICH_VMCR_ENG0_MASK));
+	vcpu_set_reg(vcpu, rt, !!FIELD_GET(ICH_VMCR_EL2_VENG0_MASK, vmcr));
 }
 
 static void __vgic_v3_read_igrpen1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 {
-	vcpu_set_reg(vcpu, rt, !!(vmcr & ICH_VMCR_ENG1_MASK));
+	vcpu_set_reg(vcpu, rt, !!FIELD_GET(ICH_VMCR_EL2_VENG1_MASK, vmcr));
 }
 
 static void __vgic_v3_write_igrpen0(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
@@ -870,9 +870,9 @@ static void __vgic_v3_write_igrpen0(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	u64 val = vcpu_get_reg(vcpu, rt);
 
 	if (val & 1)
-		vmcr |= ICH_VMCR_ENG0_MASK;
+		vmcr |= ICH_VMCR_EL2_VENG0_MASK;
 	else
-		vmcr &= ~ICH_VMCR_ENG0_MASK;
+		vmcr &= ~ICH_VMCR_EL2_VENG0_MASK;
 
 	__vgic_v3_write_vmcr(vmcr);
 }
@@ -882,9 +882,9 @@ static void __vgic_v3_write_igrpen1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	u64 val = vcpu_get_reg(vcpu, rt);
 
 	if (val & 1)
-		vmcr |= ICH_VMCR_ENG1_MASK;
+		vmcr |= ICH_VMCR_EL2_VENG1_MASK;
 	else
-		vmcr &= ~ICH_VMCR_ENG1_MASK;
+		vmcr &= ~ICH_VMCR_EL2_VENG1_MASK;
 
 	__vgic_v3_write_vmcr(vmcr);
 }
@@ -908,10 +908,8 @@ static void __vgic_v3_write_bpr0(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	if (val < bpr_min)
 		val = bpr_min;
 
-	val <<= ICH_VMCR_BPR0_SHIFT;
-	val &= ICH_VMCR_BPR0_MASK;
-	vmcr &= ~ICH_VMCR_BPR0_MASK;
-	vmcr |= val;
+	vmcr &= ~ICH_VMCR_EL2_VBPR0_MASK;
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VBPR0, val);
 
 	__vgic_v3_write_vmcr(vmcr);
 }
@@ -921,17 +919,15 @@ static void __vgic_v3_write_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	u64 val = vcpu_get_reg(vcpu, rt);
 	u8 bpr_min = __vgic_v3_bpr_min();
 
-	if (vmcr & ICH_VMCR_CBPR_MASK)
+	if (FIELD_GET(ICH_VMCR_EL2_VCBPR_MASK, val))
 		return;
 
 	/* Enforce BPR limiting */
 	if (val < bpr_min)
 		val = bpr_min;
 
-	val <<= ICH_VMCR_BPR1_SHIFT;
-	val &= ICH_VMCR_BPR1_MASK;
-	vmcr &= ~ICH_VMCR_BPR1_MASK;
-	vmcr |= val;
+	vmcr &= ~ICH_VMCR_EL2_VBPR1_MASK;
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VBPR1, val);
 
 	__vgic_v3_write_vmcr(vmcr);
 }
@@ -1021,19 +1017,15 @@ static void __vgic_v3_read_hppir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 
 static void __vgic_v3_read_pmr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 {
-	vmcr &= ICH_VMCR_PMR_MASK;
-	vmcr >>= ICH_VMCR_PMR_SHIFT;
-	vcpu_set_reg(vcpu, rt, vmcr);
+	vcpu_set_reg(vcpu, rt, FIELD_GET(ICH_VMCR_EL2_VPMR, vmcr));
 }
 
 static void __vgic_v3_write_pmr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 {
 	u32 val = vcpu_get_reg(vcpu, rt);
 
-	val <<= ICH_VMCR_PMR_SHIFT;
-	val &= ICH_VMCR_PMR_MASK;
-	vmcr &= ~ICH_VMCR_PMR_MASK;
-	vmcr |= val;
+	vmcr &= ~ICH_VMCR_EL2_VPMR_MASK;
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VPMR, val);
 
 	write_gicreg(vmcr, ICH_VMCR_EL2);
 }
@@ -1056,9 +1048,9 @@ static void __vgic_v3_read_ctlr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	/* A3V */
 	val |= ((vtr >> 21) & 1) << ICC_CTLR_EL1_A3V_SHIFT;
 	/* EOImode */
-	val |= ((vmcr & ICH_VMCR_EOIM_MASK) >> ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
+	val |= FIELD_GET(ICH_VMCR_EL2_VEOIM, vmcr) << ICC_CTLR_EL1_EOImode_SHIFT;
 	/* CBPR */
-	val |= (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT;
+	val |= FIELD_GET(ICH_VMCR_EL2_VCBPR, vmcr);
 
 	vcpu_set_reg(vcpu, rt, val);
 }
@@ -1068,14 +1060,14 @@ static void __vgic_v3_write_ctlr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	u32 val = vcpu_get_reg(vcpu, rt);
 
 	if (val & ICC_CTLR_EL1_CBPR_MASK)
-		vmcr |= ICH_VMCR_CBPR_MASK;
+		vmcr |= ICH_VMCR_EL2_VCBPR_MASK;
 	else
-		vmcr &= ~ICH_VMCR_CBPR_MASK;
+		vmcr &= ~ICH_VMCR_EL2_VCBPR_MASK;
 
 	if (val & ICC_CTLR_EL1_EOImode_MASK)
-		vmcr |= ICH_VMCR_EOIM_MASK;
+		vmcr |= ICH_VMCR_EL2_VEOIM_MASK;
 	else
-		vmcr &= ~ICH_VMCR_EOIM_MASK;
+		vmcr &= ~ICH_VMCR_EL2_VEOIM_MASK;
 
 	write_gicreg(vmcr, ICH_VMCR_EL2);
 }
diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
index 7f1259b49c505..cf9c14e0edd48 100644
--- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
+++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
@@ -199,16 +199,16 @@ u64 vgic_v3_get_misr(struct kvm_vcpu *vcpu)
 	if ((hcr & ICH_HCR_EL2_NPIE) && !mi_state.pend)
 		reg |= ICH_MISR_EL2_NP;
 
-	if ((hcr & ICH_HCR_EL2_VGrp0EIE) && (vmcr & ICH_VMCR_ENG0_MASK))
+	if ((hcr & ICH_HCR_EL2_VGrp0EIE) && (vmcr & ICH_VMCR_EL2_VENG0_MASK))
 		reg |= ICH_MISR_EL2_VGrp0E;
 
-	if ((hcr & ICH_HCR_EL2_VGrp0DIE) && !(vmcr & ICH_VMCR_ENG0_MASK))
+	if ((hcr & ICH_HCR_EL2_VGrp0DIE) && !(vmcr & ICH_VMCR_EL2_VENG0_MASK))
 		reg |= ICH_MISR_EL2_VGrp0D;
 
-	if ((hcr & ICH_HCR_EL2_VGrp1EIE) && (vmcr & ICH_VMCR_ENG1_MASK))
+	if ((hcr & ICH_HCR_EL2_VGrp1EIE) && (vmcr & ICH_VMCR_EL2_VENG1_MASK))
 		reg |= ICH_MISR_EL2_VGrp1E;
 
-	if ((hcr & ICH_HCR_EL2_VGrp1DIE) && !(vmcr & ICH_VMCR_ENG1_MASK))
+	if ((hcr & ICH_HCR_EL2_VGrp1DIE) && !(vmcr & ICH_VMCR_EL2_VENG1_MASK))
 		reg |= ICH_MISR_EL2_VGrp1D;
 
 	return reg;
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index b9ad7c42c5b01..f9dd5b6c57294 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -199,25 +199,23 @@ void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
 	u32 vmcr;
 
 	if (model == KVM_DEV_TYPE_ARM_VGIC_V2) {
-		vmcr = (vmcrp->ackctl << ICH_VMCR_ACK_CTL_SHIFT) &
-			ICH_VMCR_ACK_CTL_MASK;
-		vmcr |= (vmcrp->fiqen << ICH_VMCR_FIQ_EN_SHIFT) &
-			ICH_VMCR_FIQ_EN_MASK;
+		vmcr = FIELD_PREP(ICH_VMCR_EL2_VAckCtl, vmcrp->ackctl);
+		vmcr |= FIELD_PREP(ICH_VMCR_EL2_VFIQEn, vmcrp->fiqen);
 	} else {
 		/*
 		 * When emulating GICv3 on GICv3 with SRE=1 on the
 		 * VFIQEn bit is RES1 and the VAckCtl bit is RES0.
 		 */
-		vmcr = ICH_VMCR_FIQ_EN_MASK;
+		vmcr = ICH_VMCR_EL2_VFIQEn_MASK;
 	}
 
-	vmcr |= (vmcrp->cbpr << ICH_VMCR_CBPR_SHIFT) & ICH_VMCR_CBPR_MASK;
-	vmcr |= (vmcrp->eoim << ICH_VMCR_EOIM_SHIFT) & ICH_VMCR_EOIM_MASK;
-	vmcr |= (vmcrp->abpr << ICH_VMCR_BPR1_SHIFT) & ICH_VMCR_BPR1_MASK;
-	vmcr |= (vmcrp->bpr << ICH_VMCR_BPR0_SHIFT) & ICH_VMCR_BPR0_MASK;
-	vmcr |= (vmcrp->pmr << ICH_VMCR_PMR_SHIFT) & ICH_VMCR_PMR_MASK;
-	vmcr |= (vmcrp->grpen0 << ICH_VMCR_ENG0_SHIFT) & ICH_VMCR_ENG0_MASK;
-	vmcr |= (vmcrp->grpen1 << ICH_VMCR_ENG1_SHIFT) & ICH_VMCR_ENG1_MASK;
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VCBPR, vmcrp->cbpr);
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VEOIM, vmcrp->eoim);
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VBPR1, vmcrp->abpr);
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VBPR0, vmcrp->bpr);
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VPMR, vmcrp->pmr);
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VENG0, vmcrp->grpen0);
+	vmcr |= FIELD_PREP(ICH_VMCR_EL2_VENG1, vmcrp->grpen1);
 
 	cpu_if->vgic_vmcr = vmcr;
 }
@@ -231,10 +229,8 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
 	vmcr = cpu_if->vgic_vmcr;
 
 	if (model == KVM_DEV_TYPE_ARM_VGIC_V2) {
-		vmcrp->ackctl = (vmcr & ICH_VMCR_ACK_CTL_MASK) >>
-			ICH_VMCR_ACK_CTL_SHIFT;
-		vmcrp->fiqen = (vmcr & ICH_VMCR_FIQ_EN_MASK) >>
-			ICH_VMCR_FIQ_EN_SHIFT;
+		vmcrp->ackctl = FIELD_GET(ICH_VMCR_EL2_VAckCtl, vmcr);
+		vmcrp->fiqen = FIELD_GET(ICH_VMCR_EL2_VFIQEn, vmcr);
 	} else {
 		/*
 		 * When emulating GICv3 on GICv3 with SRE=1 on the
@@ -244,13 +240,13 @@ void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
 		vmcrp->ackctl = 0;
 	}
 
-	vmcrp->cbpr = (vmcr & ICH_VMCR_CBPR_MASK) >> ICH_VMCR_CBPR_SHIFT;
-	vmcrp->eoim = (vmcr & ICH_VMCR_EOIM_MASK) >> ICH_VMCR_EOIM_SHIFT;
-	vmcrp->abpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
-	vmcrp->bpr  = (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
-	vmcrp->pmr  = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT;
-	vmcrp->grpen0 = (vmcr & ICH_VMCR_ENG0_MASK) >> ICH_VMCR_ENG0_SHIFT;
-	vmcrp->grpen1 = (vmcr & ICH_VMCR_ENG1_MASK) >> ICH_VMCR_ENG1_SHIFT;
+	vmcrp->cbpr = FIELD_GET(ICH_VMCR_EL2_VCBPR, vmcr);
+	vmcrp->eoim = FIELD_GET(ICH_VMCR_EL2_VEOIM, vmcr);
+	vmcrp->abpr = FIELD_GET(ICH_VMCR_EL2_VBPR1, vmcr);
+	vmcrp->bpr  = FIELD_GET(ICH_VMCR_EL2_VBPR0, vmcr);
+	vmcrp->pmr  = FIELD_GET(ICH_VMCR_EL2_VPMR, vmcr);
+	vmcrp->grpen0 = FIELD_GET(ICH_VMCR_EL2_VENG0, vmcr);
+	vmcrp->grpen1 = FIELD_GET(ICH_VMCR_EL2_VENG1, vmcr);
 }
 
 #define INITIAL_PENDBASER_VALUE						  \
-- 
2.34.1

Re: [PATCH 1/3] arm64/sysreg: Support feature-specific fields with 'Feat' descriptor

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3586 bytes --]

On Tue, Oct 07, 2025 at 03:35:14PM +0000, Sascha Bischoff wrote:

> Some system register field encodings change based on the available and
> in-use architecture features. In order to support these different
> field encodings, introduce the Feat descriptor (Feat, ElseFeat,
> EndFeat) for describing such sysregs.

We have other system registers which change layouts based on things
other than features, ESR_ELx is probably the most entertaining of these
but there's others like PAR_EL1.  Ideally we should probably do the
logic for generating the conditionals in a manner that's not tied to
features with a layer on top that generates standard naming for common
patterns like FEAT, but OTOH part of why that's not been done because
it's got a bunch of nasty issues so perhaps just doing the simpler case
is fine.

> The Feat descriptor can be used in the following way (Feat acts as
> both an if and an else-if):

>         Sysreg  EXAMPLE 0    1    2    3    4
>         Feat    FEAT_A
>         Field   63:0    Foo
>         Feat    FEAT_B

This assumes that there will never be nesting of these conditions in the
architecture.  I'm not sure I would want to assume that, even for plain
features though I can't think of any examples where it's an issue.
There are more serious issues with the implementation for practical
patterns with nesting (see below) which mean we might not want to deal
with that now but we should define the syntax for the file in a way that
will cope so I'd prefer not to have the implicit elses.

I'd also be inclined to say that something that's specificly for
features shouldn't repeat the FEAT_ so:

          Feat    A

instead, but that's purely a taste question.

> -	define("REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_" op2)
> -	define("SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", " crm ", " op2 ")")
> +	feat = null
> +
> +	define(feat, "REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_" op2)
> +	define(feat, "SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", " crm ", " op2 ")")
>  
> -	define("SYS_" reg "_Op0", op0)
> -	define("SYS_" reg "_Op1", op1)
> -	define("SYS_" reg "_CRn", crn)
> -	define("SYS_" reg "_CRm", crm)
> -	define("SYS_" reg "_Op2", op2)
> +	define(feat, "SYS_" reg "_Op0", op0)
> +	define(feat, "SYS_" reg "_Op1", op1)
> +	define(feat, "SYS_" reg "_CRn", crn)
> +	define(feat, "SYS_" reg "_CRm", crm)
> +	define(feat, "SYS_" reg "_Op2", op2)

Possibly it's worth having a reg_define() or something given the number
of things needing updating here?

> @@ -201,6 +205,7 @@ $1 == "EndSysreg" && block_current() == "Sysreg" {
>  	res0 = null
>  	res1 = null
>  	unkn = null
> +	feat = null
>  
>  	block_pop()
>  	next

Probably worth complaining if we end a sysreg with a feature/prefix
defined.

> +$1 == "Feat" && (block_current() == "Sysreg" || block_current() == "SysregFields" || block_current() == "Feat") {

...

> +	next_bit = 63
> +
> +	res0 = "UL(0)"
> +	res1 = "UL(0)"
> +	unkn = "UL(0)"

This is only going to work if the whole register is in a FEAT_ block, so
you coudn't have:

        Sysreg  EXAMPLE 0    1    2    3    4
	Res0	63
        Feat    FEAT_A
        Field   62:1    Foo
        Feat    FEAT_B
        Field   62:32    Foo
        Field   31:1     Bar
	EndFeat
	Res0	0
	EndSysreg

but then supporting partial registers does have entertaining
consequences for handling Res0 and Res1.  If we're OK with that
restriction then the program should complain if someone tries to 
define a smaller FEAT block.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] arm64/sysreg: Support feature-specific fields with 'Feat' descriptor

[Sascha Bischoff]

On Tue, 2025-10-07 at 17:28 +0100, Mark Brown wrote:
> On Tue, Oct 07, 2025 at 03:35:14PM +0000, Sascha Bischoff wrote:
> 
> > Some system register field encodings change based on the available
> > and
> > in-use architecture features. In order to support these different
> > field encodings, introduce the Feat descriptor (Feat, ElseFeat,
> > EndFeat) for describing such sysregs.
> 
> We have other system registers which change layouts based on things
> other than features, ESR_ELx is probably the most entertaining of
> these
> but there's others like PAR_EL1.  Ideally we should probably do the
> logic for generating the conditionals in a manner that's not tied to
> features with a layer on top that generates standard naming for
> common
> patterns like FEAT, but OTOH part of why that's not been done because
> it's got a bunch of nasty issues so perhaps just doing the simpler
> case
> is fine.

Hi Mark - thanks for your input!

I agree that there are plenty of cases where sysreg fields change based
on more than just architectural features. Whilst my intent for this
change wasn't strictly to cover all of those cases, it could arguably
be used for that. Effectively, all this change is doing is to add a
prefix to the field definitions so it can be used however works best.

It does feel as if Feat was potentially the wrong name to choose.
Something like Fieldset, Prefix, AltLayout, etc might be better. I'm
rather open to suggestions here - naming is hard!

> 
> > The Feat descriptor can be used in the following way (Feat acts as
> > both an if and an else-if):
> 
> >         Sysreg  EXAMPLE 0    1    2    3    4
> >         Feat    FEAT_A
> >         Field   63:0    Foo
> >         Feat    FEAT_B
> 
> This assumes that there will never be nesting of these conditions in
> the architecture.

My thinking was that for something like that one could do:

        Feat    FEAT_A_B

Or similar. Yes, it means that anything that is nested needs to be
flattened, but it does allow one to describe that situation. In the
end, all this effectively does is to add a prefix, and said prefix can
be anything. Double so if we move away from calling it Feat, and use
something more generic instead.

> I'm not sure I would want to assume that, even for plain
> features though I can't think of any examples where it's an issue.
> There are more serious issues with the implementation for practical
> patterns with nesting (see below) which mean we might not want to
> deal
> with that now but we should define the syntax for the file in a way
> that
> will cope so I'd prefer not to have the implicit elses.

I think we could do something more like this (sticking with calling it
Feat for now):

Sysreg EXAMPLE ...
Feat A
...
EndFeat
Feat B
..
EndFeat
Field ...
EndSysreg

This way each set of field layouts is explicitly wrapped in a block,
except for the "default" ones (if present/desired). It is a bit more
verbose, but removes as much of the implicit nature as possible, and
removes the need for the Else which is more in keeping with the rest of
the blocks. I'll do this for v2.

> 
> I'd also be inclined to say that something that's specificly for
> features shouldn't repeat the FEAT_ so:
> 
>           Feat    A
> 
> instead, but that's purely a taste question.

Yup. I think the correct thing here does somewhat depend on how generic
this becomes.

> 
> > -	define("REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_"
> > op2)
> > -	define("SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", "
> > crm ", " op2 ")")
> > +	feat = null
> > +
> > +	define(feat, "REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm
> > "_" op2)
> > +	define(feat, "SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn
> > ", " crm ", " op2 ")")
> >  
> > -	define("SYS_" reg "_Op0", op0)
> > -	define("SYS_" reg "_Op1", op1)
> > -	define("SYS_" reg "_CRn", crn)
> > -	define("SYS_" reg "_CRm", crm)
> > -	define("SYS_" reg "_Op2", op2)
> > +	define(feat, "SYS_" reg "_Op0", op0)
> > +	define(feat, "SYS_" reg "_Op1", op1)
> > +	define(feat, "SYS_" reg "_CRn", crn)
> > +	define(feat, "SYS_" reg "_CRm", crm)
> > +	define(feat, "SYS_" reg "_Op2", op2)
> 
> Possibly it's worth having a reg_define() or something given the
> number of things needing updating here?
> 
> > @@ -201,6 +205,7 @@ $1 == "EndSysreg" && block_current() ==
> > "Sysreg" {
> >  	res0 = null
> >  	res1 = null
> >  	unkn = null
> > +	feat = null
> >  
> >  	block_pop()
> >  	next
> 
> Probably worth complaining if we end a sysreg with a feature/prefix
> defined.

This is already be caught as we hit the EndSysreg directive while still
in the Feat block:

Error at 4690: unhandled statement
Error at 4690: Missing terminator for Feat block

I can be more specific in catching that, if you like.

I have missed a check to catch someone defining fewer than 64-bits in a
Feat block, though - will be fixed in v2.

> 
> > +$1 == "Feat" && (block_current() == "Sysreg" || block_current() ==
> > "SysregFields" || block_current() == "Feat") {
> 
> ...
> 
> > +	next_bit = 63
> > +
> > +	res0 = "UL(0)"
> > +	res1 = "UL(0)"
> > +	unkn = "UL(0)"
> 
> This is only going to work if the whole register is in a FEAT_ block,
> so
> you coudn't have:
> 
>         Sysreg  EXAMPLE 0    1    2    3    4
> 	Res0	63
>         Feat    FEAT_A
>         Field   62:1    Foo
>         Feat    FEAT_B
>         Field   62:32    Foo
>         Field   31:1     Bar
> 	EndFeat
> 	Res0	0
> 	EndSysreg
> 
> but then supporting partial registers does have entertaining
> consequences for handling Res0 and Res1.  If we're OK with that
> restriction then the program should complain if someone tries to 
> define a smaller FEAT block.

It was my intent to only support whole registers here. Anything else
gets insanely complex rather quickly, and much of the benefit gets lost
IMO. By only operating on whole Sysregs (or SysregFields) we are able
to re-use the existing logic to ensure that definitions are complete,
and the generator's state tracking remains simple. And, as you say, it
would break the ResX, Unkn side of things if we did anything else.

When I add the missing check I mentioned above, then it will complain
if fewer than 64 bits are in a defined block.

Thanks,
Sascha

Re: [PATCH 1/3] arm64/sysreg: Support feature-specific fields with 'Feat' descriptor

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3904 bytes --]

On Thu, Oct 09, 2025 at 09:48:36AM +0000, Sascha Bischoff wrote:
> On Tue, 2025-10-07 at 17:28 +0100, Mark Brown wrote:

> > logic for generating the conditionals in a manner that's not tied to
> > features with a layer on top that generates standard naming for
> > common
> > patterns like FEAT, but OTOH part of why that's not been done because
> > it's got a bunch of nasty issues so perhaps just doing the simpler
> > case
> > is fine.

> I agree that there are plenty of cases where sysreg fields change based
> on more than just architectural features. Whilst my intent for this
> change wasn't strictly to cover all of those cases, it could arguably
> be used for that. Effectively, all this change is doing is to add a
> prefix to the field definitions so it can be used however works best.

Yeah, I don't know that we need to cover all the cases in the user
visible syntax right now - my thought was more about the code.

> It does feel as if Feat was potentially the wrong name to choose.
> Something like Fieldset, Prefix, AltLayout, etc might be better. I'm
> rather open to suggestions here - naming is hard!

It is :)

> > > The Feat descriptor can be used in the following way (Feat acts as
> > > both an if and an else-if):

> > >         Sysreg  EXAMPLE 0    1    2    3    4
> > >         Feat    FEAT_A
> > >         Field   63:0    Foo
> > >         Feat    FEAT_B

> > This assumes that there will never be nesting of these conditions in
> > the architecture.

> My thinking was that for something like that one could do:

>         Feat    FEAT_A_B

> Or similar. Yes, it means that anything that is nested needs to be
> flattened, but it does allow one to describe that situation. In the
> end, all this effectively does is to add a prefix, and said prefix can
> be anything. Double so if we move away from calling it Feat, and use
> something more generic instead.

You could definitely do it that way, and for the limited implementation
you're proposing it probably works as well.  My thinking here is about
what happens with the syntax when someone comes along and implements
nesting of fatures - the existing syntax would be broken since you
couldn't distinguish between an implicit else and nested FEAT_.

> > Probably worth complaining if we end a sysreg with a feature/prefix
> > defined.

> This is already be caught as we hit the EndSysreg directive while still
> in the Feat block:

> Error at 4690: unhandled statement
> Error at 4690: Missing terminator for Feat block

> I can be more specific in catching that, if you like.

No, it's fine so long sa there's a warning.

> > This is only going to work if the whole register is in a FEAT_ block,
> > so
> > you coudn't have:

...

> > but then supporting partial registers does have entertaining
> > consequences for handling Res0 and Res1.  If we're OK with that
> > restriction then the program should complain if someone tries to 
> > define a smaller FEAT block.

> It was my intent to only support whole registers here. Anything else
> gets insanely complex rather quickly, and much of the benefit gets lost
> IMO. By only operating on whole Sysregs (or SysregFields) we are able
> to re-use the existing logic to ensure that definitions are complete,
> and the generator's state tracking remains simple. And, as you say, it
> would break the ResX, Unkn side of things if we did anything else.

> When I add the missing check I mentioned above, then it will complain
> if fewer than 64 bits are in a defined block.

Yes, I think it's a reasonable simplification for the implementation and
so long as the syntax in the file can cope with someone doing the more
complicated cases it seems fine to leave them for later.  It's not like
it's a small bit of extra work it'd be easy to tack on, there's some
thorny issues there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]