Received-SPF: pass (google.com: domain of linux-kernel+bounces-194918-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <tencent_C5BBD6F106D73F662084BD91904F54504805@qq.com>
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.500.171.1.1\))
Subject: Re: [PATCH] RISC-V: hwprobe: Add MISALIGNED_PERF key
From: Yangyu Chen <cyy@cyyself.name>
In-Reply-To: <20240529182649.2635123-1-evan@rivosinc.com>
Date: Thu, 30 May 2024 11:36:45 +0800
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
 Albert Ou <aou@eecs.berkeley.edu>,
 Andrew Jones <ajones@ventanamicro.com>,
 Andy Chiu <andy.chiu@sifive.com>,
 =?utf-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com>,
 Conor Dooley <conor.dooley@microchip.com>,
 Costa Shulyupin <costa.shul@redhat.com>,
 Jonathan Corbet <corbet@lwn.net>,
 Paul Walmsley <paul.walmsley@sifive.com>,
 Sami Tolvanen <samitolvanen@google.com>,
 linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org,
 linux-riscv@lists.infradead.org
Content-Transfer-Encoding: quoted-printable
References: <20240529182649.2635123-1-evan@rivosinc.com>
To: Evan Green <evan@rivosinc.com>

On 2024/5/30 02:26, Evan Green wrote:
> RISCV_HWPROBE_KEY_CPUPERF_0 was mistakenly flagged as a bitmask in
> hwprobe_key_is_bitmask(), when in reality it was an enum value. This
> causes problems when used in conjunction with =
RISCV_HWPROBE_WHICH_CPUS,
> since SLOW, FAST, and EMULATED have values whose bits overlap with
> each other. If the caller asked for the set of CPUs that was SLOW or
> EMULATED, the returned set would also include CPUs that were FAST.
>=20
> Introduce a new hwprobe key, RISCV_HWPROBE_KEY_MISALIGNED_PERF, which
> returns the same values in response to a direct query (with no flags),
> but is properly handled as an enumerated value. As a result, SLOW,
> FAST, and EMULATED are all correctly treated as distinct values under
> the new key when queried with the WHICH_CPUS flag.
>=20
> Leave the old key in place to avoid disturbing applications which may
> have already come to rely on the broken behavior.
>=20
> Fixes: e178bf146e4b ("RISC-V: hwprobe: Introduce which-cpus flag")
> Signed-off-by: Evan Green <evan@rivosinc.com>
>=20
> ---
>=20
>=20
> Note: Yangyu also has a fix out for this issue at [1]. That fix is =
much
> tidier, but comes with the slight risk that some very broken userspace
> application may break now that FAST cpus are not included for the =
query
> of which cpus are SLOW or EMULATED.

Indeed. Since the value of FAST is 0b11, the SLOW and EMULATED are 0b10 =
and
0b01 respectively.

When this key is treated as a bitmask and query with
RISCV_HWPROBE_WHICH_CPUS if a CPU has a superset bitmask of the =
requested
value on the requested key, it will remain in the CPU mask. Otherwise, =
the
CPU will be clear in the CPU mask. But when a key is treated as a value, =
we
will just do a comparison. if it is not equal, then the CPU will be =
clear
in the CPU. That's why FAST cpus are included when querying with SLOW or
EMULATED with RISCV_HWPROBE_KEY_CPUPERF_0 key now.

For me, deprecating the original hwprobe key and introducing a new key
would be a better solution than changing the behavior as my patch did.

> I wanted to get this fix out so that
> we have both as options, and can discuss. These fixes are mutually
> exclusive, don't take both.

It's better to note this strange behavior on
Documentation/arch/riscv/hwprobe.rst so users can quickly understand the
differences on the behavior of these two keys.

The C code part looks good to me.

>=20
> [1] =
https://lore.kernel.org/linux-riscv/tencent_01F8E0050FB4B11CC170C3639E43F4=
1A1709@qq.com/
>=20
> ---
> Documentation/arch/riscv/hwprobe.rst | 8 ++++++--
> arch/riscv/include/asm/hwprobe.h | 2 +-
> arch/riscv/include/uapi/asm/hwprobe.h | 1 +
> arch/riscv/kernel/sys_hwprobe.c | 1 +
> 4 files changed, 9 insertions(+), 3 deletions(-)
>=20
> diff --git a/Documentation/arch/riscv/hwprobe.rst =
b/Documentation/arch/riscv/hwprobe.rst
> index 204cd4433af5..616ee372adaf 100644
> --- a/Documentation/arch/riscv/hwprobe.rst
> +++ b/Documentation/arch/riscv/hwprobe.rst
> @@ -192,8 +192,12 @@ The following keys are defined:
> supported as defined in the RISC-V ISA manual starting from commit
> d8ab5c78c207 ("Zihintpause is ratified").
>=20
> -* :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains =
performance
> - information about the selected set of processors.
> +* :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: Deprecated. Returns similar =
values to
> + :c:macro:`RISCV_HWPROBE_KEY_MISALIGNED_PERF`, but the key was =
mistakenly
> + classified as a bitmask rather than a value.
> +
> +* :c:macro:`RISCV_HWPROBE_KEY_MISALIGNED_PERF`: An enum value =
describing the
> + performance of misaligned scalar accesses on the selected set of =
processors.
>=20
> * :c:macro:`RISCV_HWPROBE_MISALIGNED_UNKNOWN`: The performance of =
misaligned
> accesses is unknown.
> diff --git a/arch/riscv/include/asm/hwprobe.h =
b/arch/riscv/include/asm/hwprobe.h
> index 630507dff5ea..150a9877b0af 100644
> --- a/arch/riscv/include/asm/hwprobe.h
> +++ b/arch/riscv/include/asm/hwprobe.h
> @@ -8,7 +8,7 @@
>=20
> #include <uapi/asm/hwprobe.h>
>=20
> -#define RISCV_HWPROBE_MAX_KEY 6
> +#define RISCV_HWPROBE_MAX_KEY 7
>=20
> static inline bool riscv_hwprobe_key_is_valid(__s64 key)
> {
> diff --git a/arch/riscv/include/uapi/asm/hwprobe.h =
b/arch/riscv/include/uapi/asm/hwprobe.h
> index dda76a05420b..bc34e33fef23 100644
> --- a/arch/riscv/include/uapi/asm/hwprobe.h
> +++ b/arch/riscv/include/uapi/asm/hwprobe.h
> @@ -68,6 +68,7 @@ struct riscv_hwprobe {
> #define RISCV_HWPROBE_MISALIGNED_UNSUPPORTED (4 << 0)
> #define RISCV_HWPROBE_MISALIGNED_MASK (7 << 0)
> #define RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE 6
> +#define RISCV_HWPROBE_KEY_MISALIGNED_PERF 7
> /* Increase RISCV_HWPROBE_MAX_KEY when adding items. */
>=20
> /* Flags */
> diff --git a/arch/riscv/kernel/sys_hwprobe.c =
b/arch/riscv/kernel/sys_hwprobe.c
> index 969ef3d59dbe..c8b7d57eb55e 100644
> --- a/arch/riscv/kernel/sys_hwprobe.c
> +++ b/arch/riscv/kernel/sys_hwprobe.c
> @@ -208,6 +208,7 @@ static void hwprobe_one_pair(struct riscv_hwprobe =
*pair,
> break;
>=20
> case RISCV_HWPROBE_KEY_CPUPERF_0:
> + case RISCV_HWPROBE_KEY_MISALIGNED_PERF:
> pair->value =3D hwprobe_misaligned(cpus);
> break;
>=20