Re: [PATCH V7 2/6] arm64/perf: Add BRBE registers and fields

From: Anshuman Khandual
Date: Thu Feb 09 2023 - 00:49:19 EST

Next message: Oleksij Rempel: "Re: [PATCH net-next v6 1/9] net: dsa: microchip: enable EEE support"
Previous message: Yue Hu: "Re: [PATCH] erofs: make kobj_type structures constant"
In reply to: Mark Rutland: "Re: [PATCH V7 2/6] arm64/perf: Add BRBE registers and fields"
Next in thread: Mark Rutland: "Re: [PATCH V7 2/6] arm64/perf: Add BRBE registers and fields"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2/9/23 00:52, Mark Rutland wrote:
> On Fri, Jan 13, 2023 at 08:32:47AM +0530, Anshuman Khandual wrote:
>> On 1/12/23 18:54, Mark Rutland wrote:
>>> Hi Anshuman,
>>>
>>> On Thu, Jan 05, 2023 at 08:40:35AM +0530, Anshuman Khandual wrote:
>>>> This adds BRBE related register definitions and various other related field
>>>> macros there in. These will be used subsequently in a BRBE driver which is
>>>> being added later on.
>>>
>>> I haven't verified the specific values, but this looks good to me aside from
>>> one minor nit below.
>>>
>>> [...]
>>>
>>>> +# This is just a dummy register declaration to get all common field masks and
>>>> +# shifts for accessing given BRBINF contents.
>>>> +Sysreg BRBINF_EL1 2 1 8 0 0
>>>
>>> We don't need a dummy declaration, as we have 'SysregFields' that can be used
>>> for this, e.g.
>>>
>>> SysregFields BRBINFx_EL1
>>> ...
>>> EndSysregFields
>>>
>>> ... which will avoid accidental usage of the register encoding. Note that I've
>>> also added an 'x' there in place of the index, which we do for other registers,
>>> e.g. TTBRx_EL1.
>>>
>>> Could you please update to that?
>>
>> There is a problem in defining SysregFields (which I did explore earlier as well).
>> SysregFields unfortunately does not support enums fields. Following build failure
>> comes up, while trying to convert BRBINFx_EL1 into a SysregFields definition.
>>
>> Error at 932: unexpected Enum (inside SysregFields)
>
> This is a problem, but it's one that we can solve. We're in control of
> gen-sysreg.awk and the language it parses, so we can make this an expected and
> supported case -- see below.
>
>> ===============================================================================
>> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
>> index a7f9054bd84c..519c4f080898 100644
>> --- a/arch/arm64/tools/sysreg
>> +++ b/arch/arm64/tools/sysreg
>> @@ -921,10 +921,7 @@ Enum 3:0 BT
>> EndEnum
>> EndSysreg
>>
>> -
>> -# This is just a dummy register declaration to get all common field masks and
>> -# shifts for accessing given BRBINF contents.
>> -Sysreg BRBINF_EL1 2 1 8 0 0
>> +SysregFields BRBINFx_EL1
>> Res0 63:47
>> Field 46 CCU
>> Field 45:32 CC
>> @@ -967,7 +964,7 @@ Enum 1:0 VALID
>> 0b10 SOURCE
>> 0b11 FULL
>> EndEnum
>> -EndSysreg
>> +EndSysregFields
>>
>> Sysreg BRBCR_EL1 2 1 9 0 0
>> Res0 63:24
>> ===============================================================================
>>
>> There are three enum fields in BRBINFx_EL1 as listed here.
>>
>> Enum 13:8 TYPE
>> Enum 7:6 EL
>> Enum 1:0 VALID
>>
>> However, BRBINF_EL1 can be changed as BRBINFx_EL1, indicating its more generic
>> nature with a potential to be used for any index value register thereafter.
>
> It's certainly better to use the BRBINFx_EL1 name, but my main concern here is
> to avoid the dummy values used above to satisfy the tools, so that those cannot
> be accidentally misused.
>
> I'd prefer that we fix gen-sysreg.awk to support Enum blocks within
> SysregFields blocks (patch below), then use SysregFields as described above.

The following patch did not apply cleanly on v6.2-rc7 but eventually did so after
some changes. Is the patch against mainline or arm64-next ? Nonetheless, it does
solve the enum problem for SysregFields. With this patch in place, I was able to

- Change Sysreg BRBINF_EL1 as SysregFields BRBINFx_EL1
- Change BRBINF_EL1_XXX fields usage as BRBINFx_EL1_XXX fields

Should I take this patch with this series as an initial prerequisite patch or you
would like to post this now for current merge window ?

>
> Thanks,
> Mark.
>
> ---->8----
>>From 0c194d92b0b9ff3b32f666a4610b077fdf1b4b93 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@xxxxxxx>
> Date: Wed, 8 Feb 2023 17:55:08 +0000
> Subject: [PATCH] arm64/sysreg: allow *Enum blocks in SysregFields blocks
>
> We'd like to support Enum/SignedEnum/UnsignedEnum blocks within
> SysregFields blocks, so that we can define enumerations for sets of
> registers. This isn't currently supported by gen-sysreg.awk due to the
> way we track the active block, which can't handle more than a single
> layer of nesting, which imposes an awkward requirement that when ending
> a block we know what the parent block is when calling change_block()
>
> Make this nicer by using a stack of active blocks, with block_push() to
> start a block, and block_pop() to end a block. Doing so means hat when
> ending a block we don't need to know the parent block type, and checks
> of the active block become more consistent. On top of that, it's easy to
> permit *Enum blocks within both Sysreg and SysregFields blocks.
>
> To aid debugging, the stack of active blocks is reported for fatal
> errors, and an error is raised if the file is terminated without ending
> the active block. For clarity I've renamed the top-level element from
> "None" to "Root".
>
> The Fields element it intended only for use within Systeg blocks, and
> does not make sense within SysregFields blocks, and so remains forbidden
> within a SysregFields block.
>
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> ---
> arch/arm64/tools/gen-sysreg.awk | 93 ++++++++++++++++++++-------------
> 1 file changed, 57 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk
> index 7f27d66a17e1..066ebf5410fa 100755
> --- a/arch/arm64/tools/gen-sysreg.awk
> +++ b/arch/arm64/tools/gen-sysreg.awk
> @@ -4,23 +4,35 @@
> #
> # Usage: awk -f gen-sysreg.awk sysregs.txt
>
> +function block_current() {
> + return __current_block[__current_block_depth];
> +}
> +
> # Log an error and terminate
> function fatal(msg) {
> print "Error at " NR ": " msg > "/dev/stderr"
> +
> + printf "Current block nesting:"
> +
> + for (i = 0; i <= __current_block_depth; i++) {
> + printf " " __current_block[i]
> + }
> + printf "\n"
> +
> exit 1
> }
>
> -# Sanity check that the start or end of a block makes sense at this point in
> -# the file. If not, produce an error and terminate.
> -#
> -# @this - the $Block or $EndBlock
> -# @prev - the only valid block to already be in (value of @block)
> -# @new - the new value of @block
> -function change_block(this, prev, new) {
> - if (block != prev)
> - fatal("unexpected " this " (inside " block ")")
> -
> - block = new
> +# Enter a new block, setting the active block to @block
> +function block_push(block) {
> + __current_block[++__current_block_depth] = block
> +}
> +
> +# Exit a block, setting the active block to the parent block
> +function block_pop() {
> + if (__current_block_depth == 0)
> + fatal("error: block_pop() in root block")
> +
> + __current_block_depth--;
> }
>
> # Sanity check the number of records for a field makes sense. If not, produce
> @@ -84,10 +96,14 @@ BEGIN {
> print "/* Generated file - do not edit */"
> print ""
>
> - block = "None"
> + __current_block_depth = 0
> + __current_block[__current_block_depth] = "Root"
> }
>
> END {
> + if (__current_block_depth != 0)
> + fatal("Missing terminator for " block_current() " block")
> +
> print "#endif /* __ASM_SYSREG_DEFS_H */"
> }
>
> @@ -95,8 +111,9 @@ END {
> /^$/ { next }
> /^[\t ]*#/ { next }
>
> -/^SysregFields/ {
> - change_block("SysregFields", "None", "SysregFields")
> +/^SysregFields/ && block_current() == "Root" {
> + block_push("SysregFields")
> +
> expect_fields(2)
>
> reg = $2
> @@ -109,12 +126,10 @@ END {
> next
> }
>
> -/^EndSysregFields/ {
> +/^EndSysregFields/ && block_current() == "SysregFields" {
> if (next_bit > 0)
> fatal("Unspecified bits in " reg)
>
> - change_block("EndSysregFields", "SysregFields", "None")
> -
> define(reg "_RES0", "(" res0 ")")
> define(reg "_RES1", "(" res1 ")")
> print ""
> @@ -123,11 +138,13 @@ END {
> res0 = null
> res1 = null
>
> + block_pop()
> next
> }
>
> -/^Sysreg/ {
> - change_block("Sysreg", "None", "Sysreg")
> +/^Sysreg/ && block_current() == "Root" {
> + block_push("Sysreg")
> +
> expect_fields(7)
>
> reg = $2
> @@ -156,12 +173,10 @@ END {
> next
> }
>
> -/^EndSysreg/ {
> +/^EndSysreg/ && block_current() == "Sysreg" {
> if (next_bit > 0)
> fatal("Unspecified bits in " reg)
>
> - change_block("EndSysreg", "Sysreg", "None")
> -
> if (res0 != null)
> define(reg "_RES0", "(" res0 ")")
> if (res1 != null)
> @@ -178,12 +193,13 @@ END {
> res0 = null
> res1 = null
>
> + block_pop()
> next
> }
>
> # Currently this is effectivey a comment, in future we may want to emit
> # defines for the fields.
> -/^Fields/ && (block == "Sysreg") {
> +/^Fields/ && block_current() == "Sysreg" {
> expect_fields(2)
>
> if (next_bit != 63)
> @@ -200,7 +216,7 @@ END {
> }
>
>
> -/^Res0/ && (block == "Sysreg" || block == "SysregFields") {
> +/^Res0/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(2)
> parse_bitdef(reg, "RES0", $2)
> field = "RES0_" msb "_" lsb
> @@ -210,7 +226,7 @@ END {
> next
> }
>
> -/^Res1/ && (block == "Sysreg" || block == "SysregFields") {
> +/^Res1/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(2)
> parse_bitdef(reg, "RES1", $2)
> field = "RES1_" msb "_" lsb
> @@ -220,7 +236,7 @@ END {
> next
> }
>
> -/^Field/ && (block == "Sysreg" || block == "SysregFields") {
> +/^Field/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(3)
> field = $3
> parse_bitdef(reg, field, $2)
> @@ -231,15 +247,16 @@ END {
> next
> }
>
> -/^Raz/ && (block == "Sysreg" || block == "SysregFields") {
> +/^Raz/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(2)
> parse_bitdef(reg, field, $2)
>
> next
> }
>
> -/^SignedEnum/ {
> - change_block("Enum<", "Sysreg", "Enum")
> +/^SignedEnum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> + block_push("Enum")
> +
> expect_fields(3)
> field = $3
> parse_bitdef(reg, field, $2)
> @@ -250,8 +267,9 @@ END {
> next
> }
>
> -/^UnsignedEnum/ {
> - change_block("Enum<", "Sysreg", "Enum")
> +/^UnsignedEnum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> + block_push("Enum")
> +
> expect_fields(3)
> field = $3
> parse_bitdef(reg, field, $2)
> @@ -262,8 +280,9 @@ END {
> next
> }
>
> -/^Enum/ {
> - change_block("Enum", "Sysreg", "Enum")
> +/^Enum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> + block_push("Enum")
> +
> expect_fields(3)
> field = $3
> parse_bitdef(reg, field, $2)
> @@ -273,16 +292,18 @@ END {
> next
> }
>
> -/^EndEnum/ {
> - change_block("EndEnum", "Enum", "Sysreg")
> +/^EndEnum/ && block_current() == "Enum" {
> +
> field = null
> msb = null
> lsb = null
> print ""
> +
> + block_pop()
> next
> }
>
> -/0b[01]+/ && block == "Enum" {
> +/0b[01]+/ && block_current() == "Enum" {
> expect_fields(2)
> val = $1
> name = $2

Next message: Oleksij Rempel: "Re: [PATCH net-next v6 1/9] net: dsa: microchip: enable EEE support"
Previous message: Yue Hu: "Re: [PATCH] erofs: make kobj_type structures constant"
In reply to: Mark Rutland: "Re: [PATCH V7 2/6] arm64/perf: Add BRBE registers and fields"
Next in thread: Mark Rutland: "Re: [PATCH V7 2/6] arm64/perf: Add BRBE registers and fields"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]