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@xxxxxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
