| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zq4QuLbGkXQvhbB0@J2N7QTR9R3>
Date: Sat, 3 Aug 2024 12:12:56 +0100
From: Mark Rutland <mark.rutland@....com>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Mark Brown <broonie@...nel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] arm64/tools/sysreg: Add Sysreg128/SysregFields128
On Thu, Aug 01, 2024 at 11:14:36AM +0530, Anshuman Khandual wrote:
> FEAT_SYSREG128 enables 128 bit wide system registers which also need to be
> defined in (arch/arm64/toos/sysreg) for auto mask generation. This adds two
> new field types i.e Sysreg128 and SysregFields128 for that same purpose. It
> utilizes recently added macro GENMASK_U128() while also adding some helpers
> such as define_field_128() and parse_bitdef_128().
>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Mark Brown <broonie@...nel.org>
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> ---
> arch/arm64/tools/gen-sysreg.awk | 231 ++++++++++++++++++++++++++++++++
> 1 file changed, 231 insertions(+)
Having Sysreg128 and SysregFields128 sounds reasonable, though I suspect
we can share more code with Sysreg and SysregFields (e.g. by always
using GENMASK128() even for regular SYsreg and SysregFIilds).
Regardless of this patch in particular, I think we want to see some
end-to-end usage (i.e. some actual bit definitions, along with asm and C
code that uses these definitions) so that we're confident this is the
right way to capture these definitions.
Sending this piecemeal, separate to those elements and sepate to
GENMASK_U128() makes this very painful to review effectively. Please
combine those elements into a single series so that reviewers can see
the entire picture.
Mark.
> diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk
> index d1254a056114..a1571881d1c3 100755
> --- a/arch/arm64/tools/gen-sysreg.awk
> +++ b/arch/arm64/tools/gen-sysreg.awk
> @@ -56,6 +56,13 @@ function define_field(reg, field, msb, lsb) {
> define(reg "_" field "_WIDTH", msb - lsb + 1)
> }
>
> +function define_field_128(reg, field, msb, lsb) {
> + define(reg "_" field, "GENMASK_U128(" msb ", " lsb ")")
> + define(reg "_" field "_MASK", "GENMASK_U128(" msb ", " lsb ")")
> + define(reg "_" field "_SHIFT", lsb)
> + define(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)
> @@ -89,6 +96,33 @@ function parse_bitdef(reg, field, bitdef, _bits)
> next_bit = lsb - 1
> }
>
> +function parse_bitdef_128(reg, field, bitdef, _bits)
> +{
> + if (bitdef ~ /^[0-9]+$/) {
> + msb = bitdef
> + lsb = bitdef
> + } else if (split(bitdef, _bits, ":") == 2) {
> + msb = _bits[1]
> + lsb = _bits[2]
> + } else {
> + fatal("invalid bit-range definition '" bitdef "'")
> + }
> +
> +
> + if (msb != next_bit)
> + fatal(reg "." field " starts at " msb " not " next_bit)
> + if (127 < msb || msb < 0)
> + fatal(reg "." field " invalid high bit in '" bitdef "'")
> + if (127 < lsb || lsb < 0)
> + fatal(reg "." field " invalid low bit in '" bitdef "'")
> + if (msb < lsb)
> + fatal(reg "." field " invalid bit-range '" bitdef "'")
> + if (low > high)
> + fatal(reg "." field " has invalid range " high "-" low)
> +
> + next_bit = lsb - 1
> +}
> +
> BEGIN {
> print "#ifndef __ASM_SYSREG_DEFS_H"
> print "#define __ASM_SYSREG_DEFS_H"
> @@ -111,6 +145,99 @@ END {
> /^$/ { next }
> /^[\t ]*#/ { next }
>
> +/^SysregFields128/ && block_current() == "Root" {
> + block_push("SysregFields128")
> +
> + expect_fields(2)
> +
> + reg = $2
> +
> + res0 = "UL(0)"
> + res1 = "UL(0)"
> + unkn = "UL(0)"
> +
> + next_bit = 127
> +
> + next
> +}
> +
> +/^EndSysregFields128/ && block_current() == "SysregFields128" {
> + if (next_bit > 0)
> + fatal("Unspecified bits in " reg)
> +
> + define(reg "_RES0", "(" res0 ")")
> + define(reg "_RES1", "(" res1 ")")
> + define(reg "_UNKN", "(" unkn ")")
> + print ""
> +
> + reg = null
> + res0 = null
> + res1 = null
> + unkn = null
> +
> + block_pop()
> + next
> +}
> +
> +/^Sysreg128/ && block_current() == "Root" {
> + block_push("Sysreg128")
> +
> + expect_fields(7)
> +
> + reg = $2
> + op0 = $3
> + op1 = $4
> + crn = $5
> + crm = $6
> + op2 = $7
> +
> + 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 ")")
> +
> + 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)
> +
> + print ""
> +
> + next_bit = 127
> +
> + next
> +}
> +
> +/^EndSysreg128/ && block_current() == "Sysreg128" {
> + 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 ""
> +
> + reg = null
> + op0 = null
> + op1 = null
> + crn = null
> + crm = null
> + op2 = null
> + res0 = null
> + res1 = null
> + unkn = null
> +
> + block_pop()
> + next
> +}
> +
> /^SysregFields/ && block_current() == "Root" {
> block_push("SysregFields")
>
> @@ -223,6 +350,22 @@ END {
> next
> }
>
> +/^Fields/ && block_current() == "Sysreg128" {
> + expect_fields(2)
> +
> + if (next_bit != 127)
> + fatal("Some fields already defined for " reg)
> +
> + print "/* For " reg " fields see " $2 " */"
> + print ""
> +
> + next_bit = 0
> + res0 = null
> + res1 = null
> + unkn = null
> +
> + next
> +}
>
> /^Res0/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(2)
> @@ -234,6 +377,16 @@ END {
> next
> }
>
> +/^Res0/ && (block_current() == "Sysreg128" || block_current() == "SysregFields128") {
> + expect_fields(2)
> + parse_bitdef_128(reg, "RES0", $2)
> + field = "RES0_" msb "_" lsb
> +
> + res0 = res0 " | GENMASK_U128(" msb ", " lsb ")"
> +
> + next
> +}
> +
> /^Res1/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(2)
> parse_bitdef(reg, "RES1", $2)
> @@ -244,6 +397,16 @@ END {
> next
> }
>
> +/^Res1/ && (block_current() == "Sysreg128" || block_current() == "SysregFields128") {
> + expect_fields(2)
> + parse_bitdef_128(reg, "RES1", $2)
> + field = "RES1_" msb "_" lsb
> +
> + res1 = res1 " | GENMASK_U128(" msb ", " lsb ")"
> +
> + next
> +}
> +
> /^Unkn/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(2)
> parse_bitdef(reg, "UNKN", $2)
> @@ -254,6 +417,16 @@ END {
> next
> }
>
> +/^Unkn/ && (block_current() == "Sysreg128" || block_current() == "SysregFields128") {
> + expect_fields(2)
> + parse_bitdef_128(reg, "UNKN", $2)
> + field = "UNKN_" msb "_" lsb
> +
> + unkn = unkn " | GENMASK_U128(" msb ", " lsb ")"
> +
> + next
> +}
> +
> /^Field/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(3)
> field = $3
> @@ -265,6 +438,17 @@ END {
> next
> }
>
> +/^Field/ && (block_current() == "Sysreg128" || block_current() == "SysregFields128") {
> + expect_fields(3)
> + field = $3
> + parse_bitdef_128(reg, field, $2)
> +
> + define_field_128(reg, field, msb, lsb)
> + print ""
> +
> + next
> +}
> +
> /^Raz/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> expect_fields(2)
> parse_bitdef(reg, field, $2)
> @@ -272,6 +456,14 @@ END {
> next
> }
>
> +/^Raz/ && (block_current() == "Sysreg128" || block_current() == "SysregFields128") {
> + expect_fields(2)
> + parse_bitdef_128(reg, field, $2)
> +
> + next
> +}
> +
> +
> /^SignedEnum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> block_push("Enum")
>
> @@ -285,6 +477,19 @@ END {
> next
> }
>
> +/^SignedEnum/ && (block_current() == "Sysreg128" || block_current() == "SysregFields128") {
> + block_push("Enum")
> +
> + expect_fields(3)
> + field = $3
> + parse_bitdef_128(reg, field, $2)
> +
> + define_field_128(reg, field, msb, lsb)
> + define_field_sign(reg, field, "true")
> +
> + next
> +}
> +
> /^UnsignedEnum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> block_push("Enum")
>
> @@ -298,6 +503,20 @@ END {
> next
> }
>
> +/^UnsignedEnum/ && (block_current() == "Sysreg128" || block_current() == "SysregFields128") {
> + block_push("Enum")
> +
> + expect_fields(3)
> + field = $3
> + parse_bitdef_128(reg, field, $2)
> +
> + define_field_128(reg, field, msb, lsb)
> + define_field_sign(reg, field, "false")
> +
> + next
> +}
> +
> +
> /^Enum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> block_push("Enum")
>
> @@ -310,6 +529,18 @@ END {
> next
> }
>
> +/^Enum/ && (block_current() == "Sysreg128" || block_current() == "SysregFields128") {
> + block_push("Enum")
> +
> + expect_fields(3)
> + field = $3
> + parse_bitdef_128(reg, field, $2)
> +
> + define_field_128(reg, field, msb, lsb)
> +
> + next
> +}
> +
> /^EndEnum/ && block_current() == "Enum" {
>
> field = null
> --
> 2.30.2
>
>
Powered by blists - more mailing lists