| Message ID | 20221218051412.384657-2-akihiko.odaki@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Normalize cache configuration | expand |
On Sun, 18 Dec 2022 05:14:06 +0000, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > Convert CCSIDR_EL1 to automatic generation as per DDI0487I.a. The field > definition is for case when FEAT_CCIDX is not implemented. Fields WT, > WB, RA and WA are defined as per A.j since they are now reserved and > may have UNKNOWN values in I.a, which the file format cannot represent. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > arch/arm64/include/asm/sysreg.h | 1 - > arch/arm64/tools/sysreg | 11 +++++++++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 7d301700d1a9..910e960661d3 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -425,7 +425,6 @@ > > #define SYS_CNTKCTL_EL1 sys_reg(3, 0, 14, 1, 0) > > -#define SYS_CCSIDR_EL1 sys_reg(3, 1, 0, 0, 0) > #define SYS_AIDR_EL1 sys_reg(3, 1, 0, 0, 7) > > #define SYS_RNDR_EL0 sys_reg(3, 3, 2, 4, 0) > diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg > index 384757a7eda9..acc79b5ccf92 100644 > --- a/arch/arm64/tools/sysreg > +++ b/arch/arm64/tools/sysreg > @@ -871,6 +871,17 @@ Sysreg SCXTNUM_EL1 3 0 13 0 7 > Field 63:0 SoftwareContextNumber > EndSysreg > > +Sysreg CCSIDR_EL1 3 1 0 0 0 > +Res0 63:32 > +Field 31:31 WT > +Field 30:30 WB > +Field 29:29 RA > +Field 28:28 WA For fields described as a single bit, the tool supports simply indicating the bit number (28 rather than 28:28). However, I strongly recommend against describing fields that have been dropped from the architecture. This only happens when these fields are never used by any implementation, so describing them is at best useless. > +Field 27:13 NumSets > +Field 12:3 Associavity > +Field 2:0 LineSize > +EndSysreg > + I don't think we have a good solution for overlapping fields that depend on other factors, either contextual (such as a mode that changes the layout of a sysreg), or architecture warts such as FEAT_CCIDX (which changes the layout of a well-known sysreg). At least, put a comment here that indicates the context of the description. Thanks, M.
On 2022/12/18 20:23, Marc Zyngier wrote: > On Sun, 18 Dec 2022 05:14:06 +0000, > Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> Convert CCSIDR_EL1 to automatic generation as per DDI0487I.a. The field >> definition is for case when FEAT_CCIDX is not implemented. Fields WT, >> WB, RA and WA are defined as per A.j since they are now reserved and >> may have UNKNOWN values in I.a, which the file format cannot represent. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> arch/arm64/include/asm/sysreg.h | 1 - >> arch/arm64/tools/sysreg | 11 +++++++++++ >> 2 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h >> index 7d301700d1a9..910e960661d3 100644 >> --- a/arch/arm64/include/asm/sysreg.h >> +++ b/arch/arm64/include/asm/sysreg.h >> @@ -425,7 +425,6 @@ >> >> #define SYS_CNTKCTL_EL1 sys_reg(3, 0, 14, 1, 0) >> >> -#define SYS_CCSIDR_EL1 sys_reg(3, 1, 0, 0, 0) >> #define SYS_AIDR_EL1 sys_reg(3, 1, 0, 0, 7) >> >> #define SYS_RNDR_EL0 sys_reg(3, 3, 2, 4, 0) >> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg >> index 384757a7eda9..acc79b5ccf92 100644 >> --- a/arch/arm64/tools/sysreg >> +++ b/arch/arm64/tools/sysreg >> @@ -871,6 +871,17 @@ Sysreg SCXTNUM_EL1 3 0 13 0 7 >> Field 63:0 SoftwareContextNumber >> EndSysreg >> >> +Sysreg CCSIDR_EL1 3 1 0 0 0 >> +Res0 63:32 >> +Field 31:31 WT >> +Field 30:30 WB >> +Field 29:29 RA >> +Field 28:28 WA > > For fields described as a single bit, the tool supports simply > indicating the bit number (28 rather than 28:28). > > However, I strongly recommend against describing fields that have been > dropped from the architecture. This only happens when these fields > are never used by any implementation, so describing them is at best > useless. arch/arm64/tools/gen-sysreg.awk does not allow a hole and requires all bits are described hence these descriptions. If you have an alternative idea I'd like to hear. > >> +Field 27:13 NumSets >> +Field 12:3 Associavity >> +Field 2:0 LineSize >> +EndSysreg >> + > > I don't think we have a good solution for overlapping fields that > depend on other factors, either contextual (such as a mode that > changes the layout of a sysreg), or architecture warts such as > FEAT_CCIDX (which changes the layout of a well-known sysreg). > > At least, put a comment here that indicates the context of the > description. Sounds good. I'll do so with the next version. Regards, Akihiko Odaki > > Thanks, > > M. >
On Sun, 18 Dec 2022 11:35:12 +0000, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2022/12/18 20:23, Marc Zyngier wrote: > > On Sun, 18 Dec 2022 05:14:06 +0000, > > Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> > >> Convert CCSIDR_EL1 to automatic generation as per DDI0487I.a. The field > >> definition is for case when FEAT_CCIDX is not implemented. Fields WT, > >> WB, RA and WA are defined as per A.j since they are now reserved and > >> may have UNKNOWN values in I.a, which the file format cannot represent. > >> > >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > >> --- > >> arch/arm64/include/asm/sysreg.h | 1 - > >> arch/arm64/tools/sysreg | 11 +++++++++++ > >> 2 files changed, 11 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > >> index 7d301700d1a9..910e960661d3 100644 > >> --- a/arch/arm64/include/asm/sysreg.h > >> +++ b/arch/arm64/include/asm/sysreg.h > >> @@ -425,7 +425,6 @@ > >> #define SYS_CNTKCTL_EL1 sys_reg(3, 0, 14, 1, > >> 0) > >> -#define SYS_CCSIDR_EL1 sys_reg(3, 1, 0, 0, 0) > >> #define SYS_AIDR_EL1 sys_reg(3, 1, 0, 0, 7) > >> #define SYS_RNDR_EL0 sys_reg(3, 3, 2, 4, 0) > >> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg > >> index 384757a7eda9..acc79b5ccf92 100644 > >> --- a/arch/arm64/tools/sysreg > >> +++ b/arch/arm64/tools/sysreg > >> @@ -871,6 +871,17 @@ Sysreg SCXTNUM_EL1 3 0 13 0 7 > >> Field 63:0 SoftwareContextNumber > >> EndSysreg > >> +Sysreg CCSIDR_EL1 3 1 0 0 0 > >> +Res0 63:32 > >> +Field 31:31 WT > >> +Field 30:30 WB > >> +Field 29:29 RA > >> +Field 28:28 WA > > > > For fields described as a single bit, the tool supports simply > > indicating the bit number (28 rather than 28:28). > > > > However, I strongly recommend against describing fields that have been > > dropped from the architecture. This only happens when these fields > > are never used by any implementation, so describing them is at best > > useless. > > arch/arm64/tools/gen-sysreg.awk does not allow a hole and requires all > bits are described hence these descriptions. If you have an > alternative idea I'd like to hear. I'd simply suggest creating an UNKNOWN field encompassing bits [21:28]. Alternatively, feel free to try the patch below, which allows you to describe these 4 bits as "Unkn 31:28", similar to Res0/Res1. > > > > >> +Field 27:13 NumSets > >> +Field 12:3 Associavity Also, you may want to fix the typo here (Associativity). Thanks, M. From 3112be25ec785de4c92d11d5964d54f216a2289c Mon Sep 17 00:00:00 2001 From: Marc Zyngier <maz@kernel.org> Date: Sun, 18 Dec 2022 12:55:23 +0000 Subject: [PATCH] arm64: Allow the definition of UNKNOWN system register fields The CCSIDR_EL1 register contains an UNKNOWN field (which replaces fields that were actually defined in previous revisions of the architecture). Define an 'Unkn' field type modeled after the Res0/Res1 types to allow such description. This allows the generation of #define CCSIDR_EL1_UNKN (UL(0) | GENMASK_ULL(31, 28)) which may have its use one day. Hopefully the architecture doesn't add too many of those in the future. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/tools/gen-sysreg.awk | 20 +++++++++++++++++++- arch/arm64/tools/sysreg | 2 ++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk index c350164a3955..e1df4b956596 100755 --- a/arch/arm64/tools/gen-sysreg.awk +++ b/arch/arm64/tools/gen-sysreg.awk @@ -98,6 +98,7 @@ END { res0 = "UL(0)" res1 = "UL(0)" + unkn = "UL(0)" next_bit = 63 @@ -112,11 +113,13 @@ END { define(reg "_RES0", "(" res0 ")") define(reg "_RES1", "(" res1 ")") + define(reg "_UNKN", "(" unkn ")") print "" reg = null res0 = null res1 = null + unkn = null next } @@ -134,6 +137,7 @@ END { res0 = "UL(0)" 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 ")") @@ -161,7 +165,9 @@ END { define(reg "_RES0", "(" res0 ")") if (res1 != null) define(reg "_RES1", "(" res1 ")") - if (res0 != null || res1 != null) + if (unkn != null) + define(reg "_UNKN", "(" unkn ")") + if (res0 != null || res1 != null || unkn != null) print "" reg = null @@ -172,6 +178,7 @@ END { op2 = null res0 = null res1 = null + unkn = null next } @@ -190,6 +197,7 @@ END { next_bit = 0 res0 = null res1 = null + unkn = null next } @@ -215,6 +223,16 @@ END { next } +/^Unkn/ && (block == "Sysreg" || block == "SysregFields") { + expect_fields(2) + parse_bitdef(reg, "UNKN", $2) + field = "UNKN_" msb "_" lsb + + unkn = unkn " | GENMASK_ULL(" msb ", " lsb ")" + + next +} + /^Field/ && (block == "Sysreg" || block == "SysregFields") { expect_fields(3) field = $3 diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg index bd5fceb26c54..472f68f020d9 100644 --- a/arch/arm64/tools/sysreg +++ b/arch/arm64/tools/sysreg @@ -15,6 +15,8 @@ # Res1 <msb>[:<lsb>] +# Unkn <msb>[:<lsb>] + # Field <msb>[:<lsb>] <name> # Enum <msb>[:<lsb>] <name>
On Sun, Dec 18, 2022 at 01:11:01PM +0000, Marc Zyngier wrote: > Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > arch/arm64/tools/gen-sysreg.awk does not allow a hole and requires all > > bits are described hence these descriptions. If you have an > > alternative idea I'd like to hear. > I'd simply suggest creating an UNKNOWN field encompassing bits > [21:28]. Alternatively, feel free to try the patch below, which allows > you to describe these 4 bits as "Unkn 31:28", similar to Res0/Res1. I agree, where practical we should add new field types and other features as needed rather than trying to shoehorn things into what the tool currently supports. It is very much a work in progress which can't fully represent everything in the spec yet. For things like the registers with multiple possible views it's much more effort which shouldn't get in the way of progress on features but with something like this just updating the tool so we can match the architecture spec is the right thing. > Define an 'Unkn' field type modeled after the Res0/Res1 types > to allow such description. This allows the generation of I'd be tempted to spell out Unknown fully since Unkn is not such a common abbreviation but I can see the desire to keep the name shorter and it doesn't really matter so either way: Reviewed-by: Mark Brown <broonie@kernel.org>
On Mon, 19 Dec 2022 15:00:15 +0000, Mark Brown <broonie@kernel.org> wrote: > > [1 <text/plain; us-ascii (7bit)>] > On Sun, Dec 18, 2022 at 01:11:01PM +0000, Marc Zyngier wrote: > > Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > arch/arm64/tools/gen-sysreg.awk does not allow a hole and requires all > > > bits are described hence these descriptions. If you have an > > > alternative idea I'd like to hear. > > > I'd simply suggest creating an UNKNOWN field encompassing bits > > [21:28]. Alternatively, feel free to try the patch below, which allows > > you to describe these 4 bits as "Unkn 31:28", similar to Res0/Res1. > > I agree, where practical we should add new field types and other > features as needed rather than trying to shoehorn things into what the > tool currently supports. It is very much a work in progress which can't > fully represent everything in the spec yet. For things like the > registers with multiple possible views it's much more effort which > shouldn't get in the way of progress on features but with something like > this just updating the tool so we can match the architecture spec is the > right thing. I was tempted to add a Namespace tag that wouldn't generate the sysreg #defines, but only generate the fields with a feature-specific namespace. For example: Sysreg CCSIDR_EL1 3 1 0 0 0 Res0 63:32 Unkn 31:28 Field 27:13 NumSets Field 12:3 Associativity Field 2:0 LineSize EndSysreg Namespace CCIDX CCSIDR_EL1 Res0 63:56 Field 55:32 NumSets Res0 31:25 Field 24:3 Associativity Field 2:0 LineSize EndSysreg the later generating: #define CCIDR_EL1_CCIDX_RES0 (GENMASK(63, 56) | GENMASK(31, 25)) #define CCIDR_EL1_CCIDX_NumSets GENMASK(55, 32) #define CCIDR_EL1_CCIDX_Associativity GENMASK(24, 3) #define CCIDR_EL1_CCIDX_LineSize GENMASK(2, 0) Thoughts? > > > Define an 'Unkn' field type modeled after the Res0/Res1 types > > to allow such description. This allows the generation of > > I'd be tempted to spell out Unknown fully since Unkn is not such a > common abbreviation but I can see the desire to keep the name shorter > and it doesn't really matter so either way: > > Reviewed-by: Mark Brown <broonie@kernel.org> Yeah, this stuff is write-only most of the time, and I like my fields aligned if at all possible. Thanks, M.
On Mon, Dec 19, 2022 at 03:27:17PM +0000, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > fully represent everything in the spec yet. For things like the > > registers with multiple possible views it's much more effort which > > shouldn't get in the way of progress on features but with something like > > this just updating the tool so we can match the architecture spec is the > > right thing. > I was tempted to add a Namespace tag that wouldn't generate the sysreg > #defines, but only generate the fields with a feature-specific > namespace. For example: I think this is roughly where we'd end up - I was using the term view when thinking about it but that's just bikeshed. > Sysreg CCSIDR_EL1 3 1 0 0 0 > Res0 63:32 > Unkn 31:28 > Field 27:13 NumSets > Field 12:3 Associativity > Field 2:0 LineSize > EndSysreg > > Namespace CCIDX CCSIDR_EL1 > Res0 63:56 > Field 55:32 NumSets > Res0 31:25 > Field 24:3 Associativity > Field 2:0 LineSize > EndSysreg Yeah, something like that. I think we also want a way to label bits in the root register as only existing in namespaces/views for things where there's no default (eg, where a feature adds two views at once or things have been there since the base architecture), and I wasn't sure if it made sense to nest the declaration of the views inside the Sysreg (I'm tempted to think it's more trouble than it's worth especially on the tooling side). I also wanted to go through and do an audit of all the current registers to make sure there were no nasty cases that'd complicate things. I don't think there'd be anything but... > the later generating: > #define CCIDR_EL1_CCIDX_RES0 (GENMASK(63, 56) | GENMASK(31, 25)) > #define CCIDR_EL1_CCIDX_NumSets GENMASK(55, 32) > #define CCIDR_EL1_CCIDX_Associativity GENMASK(24, 3) > #define CCIDR_EL1_CCIDX_LineSize GENMASK(2, 0) > Thoughts? Definitely that for the output.
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 7d301700d1a9..910e960661d3 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -425,7 +425,6 @@ #define SYS_CNTKCTL_EL1 sys_reg(3, 0, 14, 1, 0) -#define SYS_CCSIDR_EL1 sys_reg(3, 1, 0, 0, 0) #define SYS_AIDR_EL1 sys_reg(3, 1, 0, 0, 7) #define SYS_RNDR_EL0 sys_reg(3, 3, 2, 4, 0) diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg index 384757a7eda9..acc79b5ccf92 100644 --- a/arch/arm64/tools/sysreg +++ b/arch/arm64/tools/sysreg @@ -871,6 +871,17 @@ Sysreg SCXTNUM_EL1 3 0 13 0 7 Field 63:0 SoftwareContextNumber EndSysreg +Sysreg CCSIDR_EL1 3 1 0 0 0 +Res0 63:32 +Field 31:31 WT +Field 30:30 WB +Field 29:29 RA +Field 28:28 WA +Field 27:13 NumSets +Field 12:3 Associavity +Field 2:0 LineSize +EndSysreg + Sysreg CLIDR_EL1 3 1 0 0 1 Res0 63:47 Field 46:33 Ttypen
Convert CCSIDR_EL1 to automatic generation as per DDI0487I.a. The field definition is for case when FEAT_CCIDX is not implemented. Fields WT, WB, RA and WA are defined as per A.j since they are now reserved and may have UNKNOWN values in I.a, which the file format cannot represent. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- arch/arm64/include/asm/sysreg.h | 1 - arch/arm64/tools/sysreg | 11 +++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-)