linux-kernel - Re: [PATCH v2 1/2] RISC-V: Probe for unaligned access speed

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230713-unaligned-engaging-af88fa5fd9c8@spud>
Date:   Thu, 13 Jul 2023 18:21:35 +0100
From:   Conor Dooley <conor@...nel.org>
To:     Evan Green <evan@...osinc.com>
Cc:     Palmer Dabbelt <palmer@...osinc.com>, linux-doc@...r.kernel.org,
        Yangyu Chen <cyy@...self.name>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Guo Ren <guoren@...nel.org>,
        Jisheng Zhang <jszhang@...nel.org>,
        linux-riscv@...ts.infradead.org, Jonathan Corbet <corbet@....net>,
        Xianting Tian <xianting.tian@...ux.alibaba.com>,
        Marc Zyngier <maz@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Greentime Hu <greentime.hu@...ive.com>,
        Simon Hosie <shosie@...osinc.com>,
        Li Zhengyu <lizhengyu3@...wei.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Alexandre Ghiti <alexghiti@...osinc.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Heiko Stuebner <heiko.stuebner@...ll.eu>,
        Anup Patel <apatel@...tanamicro.com>,
        linux-kernel@...r.kernel.org,
        David Laight <David.Laight@...lab.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Andy Chiu <andy.chiu@...ive.com>,
        Andrew Jones <ajones@...tanamicro.com>
Subject: Re: [PATCH v2 1/2] RISC-V: Probe for unaligned access speed

On Wed, Jul 05, 2023 at 09:48:32AM -0700, Evan Green wrote:

I got kinda mad about the whole Zicclsm thing, so I decided to take a
bit before reading the words "aligned access" again.

> diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
> index 19165ebd82ba..88d7d64ec0bd 100644
> --- a/Documentation/riscv/hwprobe.rst
> +++ b/Documentation/riscv/hwprobe.rst
> @@ -87,13 +87,12 @@ The following keys are defined:
>      emulated via software, either in or below the kernel.  These accesses are
>      always extremely slow.
>  
> -  * :c:macro:`RISCV_HWPROBE_MISALIGNED_SLOW`: Misaligned accesses are supported
> -    in hardware, but are slower than the cooresponding aligned accesses
> -    sequences.
> +  * :c:macro:`RISCV_HWPROBE_MISALIGNED_SLOW`: Misaligned accesses are slower
> +    than equivalent byte accesses.  Misaligned accesses may be supported
> +    directly in hardware, or trapped and emulated by software.
>  
> -  * :c:macro:`RISCV_HWPROBE_MISALIGNED_FAST`: Misaligned accesses are supported
> -    in hardware and are faster than the cooresponding aligned accesses
> -    sequences.
> +  * :c:macro:`RISCV_HWPROBE_MISALIGNED_FAST`: Misaligned accesses are faster
> +       than equivalent byte accesses.

The indent here for line #2 looks odd. Is that an artifact of the patch?

> diff --git a/arch/riscv/kernel/copy-unaligned.h b/arch/riscv/kernel/copy-unaligned.h
> new file mode 100644
> index 000000000000..a4e8b6ad5b6a
> --- /dev/null
> +++ b/arch/riscv/kernel/copy-unaligned.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023 Rivos, Inc.
> + */
> +#ifndef __RISCV_KERNEL_COPY_UNALIGNED_H
> +#define __RISCV_KERNEL_COPY_UNALIGNED_H
> +
> +#include <linux/types.h>
> +
> +void __copy_words_unaligned(void *dst, const void *src, size_t size);
> +void __copy_bytes_unaligned(void *dst, const void *src, size_t size);

If we are putting this stuff in headers to call into asm, should we
prefix it with "riscv", or is __ enough?

> +void check_unaligned_access(int cpu)
> +{
> +	u64 c0, c1;

I quite dislike variables like "c0"/"c1", they make things harder to
read for no real benefit IMO. Would you mind renaming them?

> +	u64 word_cycles;
> +	u64 byte_cycles;
> +	int ratio;
> +	unsigned long j0, j1;
> +	struct page *page;
> +	void *dst;
> +	void *src;
> +	long speed = RISCV_HWPROBE_MISALIGNED_SLOW;

> +static int check_unaligned_access0(void)
> +{
> +	check_unaligned_access(0);
> +	return 0;
> +}

> +arch_initcall(check_unaligned_access0);

Could you please rename this function to match the actual use?
So something like s/0/_boot_cpu/?

Otherwise, I like the idea & we discussed the semantics last time around
and I was happy with them. I don't feel qualified to review the actual
speed test, so
Acked-by: Conor Dooley <conor.dooley@...rochip.com>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)