mbox series

[v3,0/8] RISC-V: Detect and report speed of unaligned vector accesses

Message ID 20240625005001.37901-1-jesse@rivosinc.com
Headers show
Series RISC-V: Detect and report speed of unaligned vector accesses | expand

Message

Jesse Taube June 25, 2024, 12:49 a.m. UTC
Adds support for detecting and reporting the speed of unaligned vector
accesses on RISC-V CPUs. Adds vec_misaligned_speed key to the hwprobe
adds Zicclsm to cpufeature and fixes the check for scalar unaligned
emulated all CPUs. The vec_misaligned_speed key keeps the same format
as the scalar unaligned access speed key.

This set does not emulate unaligned vector accesses on CPUs that do not
support them. Only reports if userspace can run them and speed of
unaligned vector accesses if supported.

If Zicclsm is present, the kernel will set both scalar and vector unaligned access speed to FAST.

This patch requires the following patche to be applied first:
RISC-V: fix vector insn load/store width mask
https://lore.kernel.org/all/20240606182800.415831-1-jesse@rivosinc.com/

Jesse Taube (8):
  RISC-V: Add Zicclsm to cpufeature and hwprobe
  dt-bindings: riscv: Add Zicclsm ISA extension description.
  RISC-V: Check scalar unaligned access on all CPUs
  RISC-V: Check Zicclsm to set unaligned access speed
  RISC-V: Replace RISCV_MISALIGNED with RISCV_SCALAR_MISALIGNED
  RISC-V: Detect unaligned vector accesses supported
  RISC-V: Report vector unaligned access speed hwprobe
  RISC-V: hwprobe: Document unaligned vector perf key

 Documentation/arch/riscv/hwprobe.rst          |  19 +++
 .../devicetree/bindings/riscv/extensions.yaml |   7 +
 arch/riscv/Kconfig                            |  57 ++++++-
 arch/riscv/include/asm/cpufeature.h           |   7 +-
 arch/riscv/include/asm/entry-common.h         |  11 --
 arch/riscv/include/asm/hwcap.h                |   1 +
 arch/riscv/include/asm/hwprobe.h              |   2 +-
 arch/riscv/include/asm/vector.h               |   1 +
 arch/riscv/include/uapi/asm/hwprobe.h         |   6 +
 arch/riscv/kernel/Makefile                    |   3 +-
 arch/riscv/kernel/copy-unaligned.h            |   5 +
 arch/riscv/kernel/cpufeature.c                |   1 +
 arch/riscv/kernel/fpu.S                       |   4 +-
 arch/riscv/kernel/sys_hwprobe.c               |  42 +++++
 arch/riscv/kernel/traps_misaligned.c          | 143 +++++++++++++---
 arch/riscv/kernel/unaligned_access_speed.c    | 159 +++++++++++++++++-
 arch/riscv/kernel/vec-copy-unaligned.S        |  58 +++++++
 arch/riscv/kernel/vector.c                    |   2 +-
 18 files changed, 480 insertions(+), 48 deletions(-)
 create mode 100644 arch/riscv/kernel/vec-copy-unaligned.S

Comments

Conor Dooley June 26, 2024, 2:37 p.m. UTC | #1
On Mon, Jun 24, 2024 at 08:50:01PM -0400, Jesse Taube wrote:
> Document key for reporting the speed of unaligned vector accesses.
> The descriptions are the same as the scalar equivalent values.
> 
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> ---
> V1 -> V2:
>   - New patch
> V2 -> V3:
>  - Specify access width
> ---
>  Documentation/arch/riscv/hwprobe.rst | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/arch/riscv/hwprobe.rst b/Documentation/arch/riscv/hwprobe.rst
> index 7085a694b801..d102b4a16d55 100644
> --- a/Documentation/arch/riscv/hwprobe.rst
> +++ b/Documentation/arch/riscv/hwprobe.rst
> @@ -236,3 +236,19 @@ The following keys are defined:
>  
>  * :c:macro:`RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE`: An unsigned int which
>    represents the size of the Zicboz block in bytes.
> +
> +* :c:macro:`RISCV_HWPROBE_KEY_VEC_MISALIGNED_PERF`: An enum value describing the
> +  performance of misaligned vector accesses on the selected set of processors.
> +
> +  * :c:macro:`RISCV_HWPROBE_VEC_MISALIGNED_UNKNOWN`: The performance of misaligned
> +    accesses is unknown.
> +
> +  * :c:macro:`RISCV_HWPROBE_VEC_MISALIGNED_SLOW`: 32bit misaligned accesses are slower

s/32bit/32-bit/

Thanks,
Conor.

> +    than equivalent byte accesses.  Misaligned accesses may be supported
> +    directly in hardware, or trapped and emulated by software.
> +
> +  * :c:macro:`RISCV_HWPROBE_VEC_MISALIGNED_FAST`: 32bit misaligned accesses are faster
> +    than equivalent byte accesses.
> +
> +  * :c:macro:`RISCV_HWPROBE_VEC_MISALIGNED_UNSUPPORTED`: Misaligned accesses are
> +    not supported at all and will generate a misaligned address fault.
> -- 
> 2.45.2
>
Conor Dooley June 26, 2024, 2:39 p.m. UTC | #2
On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
> Check for Zicclsm before checking for unaligned access speed. This will
> greatly reduce the boot up time as finding the access speed is no longer
> necessary.
> 
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> ---
> V2 -> V3:
>  - New patch split from previous patch
> ---
>  arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
> index a9a6bcb02acf..329fd289b5c8 100644
> --- a/arch/riscv/kernel/unaligned_access_speed.c
> +++ b/arch/riscv/kernel/unaligned_access_speed.c
> @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
>  	kfree(bufs);
>  	return 0;
>  }
> +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
> +static int check_unaligned_access_speed_all_cpus(void)
> +{
> +	return 0;
> +}
> +#endif
>  
>  static int check_unaligned_access_all_cpus(void)
>  {
> -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
> +	bool all_cpus_emulated;
> +	int cpu;
> +
> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
> +		for_each_online_cpu(cpu) {
> +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;

- const: zicclsm
  description:
    The standard Zicclsm extension for misaligned support for all regular
    load and store instructions (including scalar and vector) but not AMOs
    or other specialized forms of memory access. Defined in the
    RISC-V RVA Profiles Specification. 

Doesn't, unfortunately, say anywhere there that they're actually fast :(

Thanks,
Conor.

> +		}
> +		return 0;
> +	}
> +
> +	all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
>  
>  	if (!all_cpus_emulated)
>  		return check_unaligned_access_speed_all_cpus();
>  
>  	return 0;
>  }
> -#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
> -static int check_unaligned_access_all_cpus(void)
> -{
> -	check_unaligned_access_emulated_all_cpus();
> -
> -	return 0;
> -}
> -#endif
>  
>  arch_initcall(check_unaligned_access_all_cpus);
> -- 
> 2.45.2
>
Conor Dooley June 26, 2024, 2:39 p.m. UTC | #3
On Mon, Jun 24, 2024 at 08:49:58PM -0400, Jesse Taube wrote:
> Replace RISCV_MISALIGNED with RISCV_SCALAR_MISALIGNED to allow
> for the addition of RISCV_VECTOR_MISALIGNED in a later patch.
> 
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Conor Dooley June 26, 2024, 2:41 p.m. UTC | #4
On Mon, Jun 24, 2024 at 08:49:54PM -0400, Jesse Taube wrote:
> > Zicclsm Misaligned loads and stores to main memory regions with both
> > the cacheability and coherence PMAs must be supported.
> > Note:
> > This introduces a new extension name for this feature.
> > This requires misaligned support for all regular load and store
> > instructions (including scalar and vector) but not AMOs or other
> > specialized forms of memory access. Even though mandated, misaligned
> > loads and stores might execute extremely slowly. Standard software
> > distributions should assume their existence only for correctness,
> > not for performance.
> 
> Detecing zicclsm allows the kernel to report if the
> hardware supports misaligned accesses even if support wasn't probed.
> 
> This is useful for usermode to know if vector misaligned accesses are
> supported.
> 
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Andy Chiu <andy.chiu@sifive.com>
> ---
> V1 -> V2:
>  - Add documentation for Zicclsm
>  - Move Zicclsm to correct location
> V2 -> V3:
>  - No changes
> ---
>  Documentation/arch/riscv/hwprobe.rst  | 3 +++
>  arch/riscv/include/asm/hwcap.h        | 1 +
>  arch/riscv/include/uapi/asm/hwprobe.h | 1 +
>  arch/riscv/kernel/cpufeature.c        | 1 +
>  arch/riscv/kernel/sys_hwprobe.c       | 1 +
>  5 files changed, 7 insertions(+)
> 
> diff --git a/Documentation/arch/riscv/hwprobe.rst b/Documentation/arch/riscv/hwprobe.rst
> index df5045103e73..7085a694b801 100644
> --- a/Documentation/arch/riscv/hwprobe.rst
> +++ b/Documentation/arch/riscv/hwprobe.rst
> @@ -207,6 +207,9 @@ The following keys are defined:
>    * :c:macro:`RISCV_HWPROBE_EXT_ZVE64D`: The Vector sub-extension Zve64d is
>      supported, as defined by version 1.0 of the RISC-V Vector extension manual.
>  
> +  * :c:macro:`RISCV_HWPROBE_EXT_ZICCLSM`: The Zicclsm extension is supported as
> +       defined in the RISC-V RVA Profiles Specification.

I'd rather that you regurgitated the definition here, these keys/values
cannot change their meaning, but RISC-V specs are not stable.

Cheers,
Conor.
Charlie Jenkins June 27, 2024, 9:20 p.m. UTC | #5
On Wed, Jun 26, 2024 at 03:39:14PM +0100, Conor Dooley wrote:
> On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
> > Check for Zicclsm before checking for unaligned access speed. This will
> > greatly reduce the boot up time as finding the access speed is no longer
> > necessary.
> > 
> > Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> > ---
> > V2 -> V3:
> >  - New patch split from previous patch
> > ---
> >  arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
> > index a9a6bcb02acf..329fd289b5c8 100644
> > --- a/arch/riscv/kernel/unaligned_access_speed.c
> > +++ b/arch/riscv/kernel/unaligned_access_speed.c
> > @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
> >  	kfree(bufs);
> >  	return 0;
> >  }
> > +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
> > +static int check_unaligned_access_speed_all_cpus(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> >  
> >  static int check_unaligned_access_all_cpus(void)
> >  {
> > -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
> > +	bool all_cpus_emulated;
> > +	int cpu;
> > +
> > +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
> > +		for_each_online_cpu(cpu) {
> > +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;
> 
> - const: zicclsm
>   description:
>     The standard Zicclsm extension for misaligned support for all regular
>     load and store instructions (including scalar and vector) but not AMOs
>     or other specialized forms of memory access. Defined in the
>     RISC-V RVA Profiles Specification. 
> 
> Doesn't, unfortunately, say anywhere there that they're actually fast :(

Oh no! That is unfortunate that the ISA does not explicitly call that
out, but I think that acceptable.

If a vendor puts Zicclsm in their isa string, they should expect
software to take advantage of misaligned accesses. FAST is our signal to
tell software that they should emit misaligned accesses.

This allows for a generic kernel, like the one a distro would compile, to
skip the probing when booting on a system that explicitly called out
that the hardware supports misaligned accesses.

- Charlie

> 
> Thanks,
> Conor.
> 
> > +		}
> > +		return 0;
> > +	}
> > +
> > +	all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
> >  
> >  	if (!all_cpus_emulated)
> >  		return check_unaligned_access_speed_all_cpus();
> >  
> >  	return 0;
> >  }
> > -#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
> > -static int check_unaligned_access_all_cpus(void)
> > -{
> > -	check_unaligned_access_emulated_all_cpus();
> > -
> > -	return 0;
> > -}
> > -#endif
> >  
> >  arch_initcall(check_unaligned_access_all_cpus);
> > -- 
> > 2.45.2
> >
Clément Léger July 1, 2024, 7:15 a.m. UTC | #6
On 27/06/2024 23:20, Charlie Jenkins wrote:
> On Wed, Jun 26, 2024 at 03:39:14PM +0100, Conor Dooley wrote:
>> On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
>>> Check for Zicclsm before checking for unaligned access speed. This will
>>> greatly reduce the boot up time as finding the access speed is no longer
>>> necessary.
>>>
>>> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
>>> ---
>>> V2 -> V3:
>>>  - New patch split from previous patch
>>> ---
>>>  arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
>>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
>>> index a9a6bcb02acf..329fd289b5c8 100644
>>> --- a/arch/riscv/kernel/unaligned_access_speed.c
>>> +++ b/arch/riscv/kernel/unaligned_access_speed.c
>>> @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
>>>  	kfree(bufs);
>>>  	return 0;
>>>  }
>>> +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
>>> +static int check_unaligned_access_speed_all_cpus(void)
>>> +{
>>> +	return 0;
>>> +}
>>> +#endif
>>>  
>>>  static int check_unaligned_access_all_cpus(void)
>>>  {
>>> -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
>>> +	bool all_cpus_emulated;
>>> +	int cpu;
>>> +
>>> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
>>> +		for_each_online_cpu(cpu) {
>>> +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;
>>
>> - const: zicclsm
>>   description:
>>     The standard Zicclsm extension for misaligned support for all regular
>>     load and store instructions (including scalar and vector) but not AMOs
>>     or other specialized forms of memory access. Defined in the
>>     RISC-V RVA Profiles Specification. 
>>
>> Doesn't, unfortunately, say anywhere there that they're actually fast :(
> 
> Oh no! That is unfortunate that the ISA does not explicitly call that
> out, but I think that acceptable.
> 
> If a vendor puts Zicclsm in their isa string, they should expect
> software to take advantage of misaligned accesses. FAST is our signal to
> tell software that they should emit misaligned accesses.

AFAIK, Zicclsm is not even an ISA extension, simply a profile
specification which means that only the execution environment which
provides the profile support misaligned accesses (cf
https://lists.riscv.org/g/tech-profiles/message/56). I don't think we
can extrapolate that the misaligned accesses will be fast at all.

Thanks,

Clément

> 
> This allows for a generic kernel, like the one a distro would compile, to
> skip the probing when booting on a system that explicitly called out
> that the hardware supports misaligned accesses.
> 
> - Charlie
> 
>>
>> Thanks,
>> Conor.
>>
>>> +		}
>>> +		return 0;
>>> +	}
>>> +
>>> +	all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
>>>  
>>>  	if (!all_cpus_emulated)
>>>  		return check_unaligned_access_speed_all_cpus();
>>>  
>>>  	return 0;
>>>  }
>>> -#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
>>> -static int check_unaligned_access_all_cpus(void)
>>> -{
>>> -	check_unaligned_access_emulated_all_cpus();
>>> -
>>> -	return 0;
>>> -}
>>> -#endif
>>>  
>>>  arch_initcall(check_unaligned_access_all_cpus);
>>> -- 
>>> 2.45.2
>>>
> 
>
Conor Dooley July 1, 2024, 1:58 p.m. UTC | #7
On Mon, Jul 01, 2024 at 09:15:09AM +0200, Clément Léger wrote:
> 
> 
> On 27/06/2024 23:20, Charlie Jenkins wrote:
> > On Wed, Jun 26, 2024 at 03:39:14PM +0100, Conor Dooley wrote:
> >> On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
> >>> Check for Zicclsm before checking for unaligned access speed. This will
> >>> greatly reduce the boot up time as finding the access speed is no longer
> >>> necessary.
> >>>
> >>> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> >>> ---
> >>> V2 -> V3:
> >>>  - New patch split from previous patch
> >>> ---
> >>>  arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
> >>>  1 file changed, 17 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
> >>> index a9a6bcb02acf..329fd289b5c8 100644
> >>> --- a/arch/riscv/kernel/unaligned_access_speed.c
> >>> +++ b/arch/riscv/kernel/unaligned_access_speed.c
> >>> @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
> >>>  	kfree(bufs);
> >>>  	return 0;
> >>>  }
> >>> +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
> >>> +static int check_unaligned_access_speed_all_cpus(void)
> >>> +{
> >>> +	return 0;
> >>> +}
> >>> +#endif
> >>>  
> >>>  static int check_unaligned_access_all_cpus(void)
> >>>  {
> >>> -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
> >>> +	bool all_cpus_emulated;
> >>> +	int cpu;
> >>> +
> >>> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
> >>> +		for_each_online_cpu(cpu) {
> >>> +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;
> >>
> >> - const: zicclsm
> >>   description:
> >>     The standard Zicclsm extension for misaligned support for all regular
> >>     load and store instructions (including scalar and vector) but not AMOs
> >>     or other specialized forms of memory access. Defined in the
> >>     RISC-V RVA Profiles Specification. 
> >>
> >> Doesn't, unfortunately, say anywhere there that they're actually fast :(
> > 
> > Oh no! That is unfortunate that the ISA does not explicitly call that
> > out, but I think that acceptable.
> > 
> > If a vendor puts Zicclsm in their isa string, they should expect
> > software to take advantage of misaligned accesses. FAST is our signal to
> > tell software that they should emit misaligned accesses.
> 
> AFAIK, Zicclsm is not even an ISA extension, simply a profile
> specification which means that only the execution environment which
> provides the profile support misaligned accesses (cf
> https://lists.riscv.org/g/tech-profiles/message/56).

I dunno, the specification status page used to describe it as an
extension:
https://wiki.riscv.org/display/HOME/Specification+Status+-+Historical
My understanding was that these could be considered extensions, just
like we are considering svade to be one.

> . I don't think we
> can extrapolate that the misaligned accesses will be fast at all.

That is my opinion on it too. If it doesn't say "fast" and give a
definition for what that means in the binding, then we can't assume that
it is fast. I'm also wary of extending definitions of extensions in the
binding, because a) I am 90% sure that people writing devicetrees don't
care and b) it'd be a potential difference between DT and ACPI without a
real justification (unlike the zkr or svade/svadu situations).

> > This allows for a generic kernel, like the one a distro would compile, to
> > skip the probing when booting on a system that explicitly called out
> > that the hardware supports misaligned accesses.
Clément Léger July 1, 2024, 2:20 p.m. UTC | #8
On 01/07/2024 15:58, Conor Dooley wrote:
> On Mon, Jul 01, 2024 at 09:15:09AM +0200, Clément Léger wrote:
>>
>>
>> On 27/06/2024 23:20, Charlie Jenkins wrote:
>>> On Wed, Jun 26, 2024 at 03:39:14PM +0100, Conor Dooley wrote:
>>>> On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
>>>>> Check for Zicclsm before checking for unaligned access speed. This will
>>>>> greatly reduce the boot up time as finding the access speed is no longer
>>>>> necessary.
>>>>>
>>>>> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
>>>>> ---
>>>>> V2 -> V3:
>>>>>  - New patch split from previous patch
>>>>> ---
>>>>>  arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
>>>>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
>>>>> index a9a6bcb02acf..329fd289b5c8 100644
>>>>> --- a/arch/riscv/kernel/unaligned_access_speed.c
>>>>> +++ b/arch/riscv/kernel/unaligned_access_speed.c
>>>>> @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
>>>>>  	kfree(bufs);
>>>>>  	return 0;
>>>>>  }
>>>>> +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
>>>>> +static int check_unaligned_access_speed_all_cpus(void)
>>>>> +{
>>>>> +	return 0;
>>>>> +}
>>>>> +#endif
>>>>>  
>>>>>  static int check_unaligned_access_all_cpus(void)
>>>>>  {
>>>>> -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
>>>>> +	bool all_cpus_emulated;
>>>>> +	int cpu;
>>>>> +
>>>>> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
>>>>> +		for_each_online_cpu(cpu) {
>>>>> +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;
>>>>
>>>> - const: zicclsm
>>>>   description:
>>>>     The standard Zicclsm extension for misaligned support for all regular
>>>>     load and store instructions (including scalar and vector) but not AMOs
>>>>     or other specialized forms of memory access. Defined in the
>>>>     RISC-V RVA Profiles Specification. 
>>>>
>>>> Doesn't, unfortunately, say anywhere there that they're actually fast :(
>>>
>>> Oh no! That is unfortunate that the ISA does not explicitly call that
>>> out, but I think that acceptable.
>>>
>>> If a vendor puts Zicclsm in their isa string, they should expect
>>> software to take advantage of misaligned accesses. FAST is our signal to
>>> tell software that they should emit misaligned accesses.
>>
>> AFAIK, Zicclsm is not even an ISA extension, simply a profile
>> specification which means that only the execution environment which
>> provides the profile support misaligned accesses (cf
>> https://lists.riscv.org/g/tech-profiles/message/56).
> 
> I dunno, the specification status page used to describe it as an
> extension:
> https://wiki.riscv.org/display/HOME/Specification+Status+-+Historical
> My understanding was that these could be considered extensions, just
> like we are considering svade to be one.
> 
>> . I don't think we
>> can extrapolate that the misaligned accesses will be fast at all.
> 
> That is my opinion on it too. If it doesn't say "fast" and give a
> definition for what that means in the binding, then we can't assume that
> it is fast. I'm also wary of extending definitions of extensions in the
> binding, because a) I am 90% sure that people writing devicetrees don't
> care and b) it'd be a potential difference between DT and ACPI without a
> real justification (unlike the zkr or svade/svadu situations).

BTW, the profile spec [1] has a note that states the following for Zicclsm:

"Even though mandated, misaligned loads and stores might execute
extremely slowly. Standard software distributions should assume their
existence only for correctness, not for performance."

Which was also quoted in patch 1, so I guess that settles it.

Thanks,

Clément

Link:
https://github.com/riscv/riscv-profiles/blob/main/src/profiles.adoc?plain=1#L524
[1]

> 
>>> This allows for a generic kernel, like the one a distro would compile, to
>>> skip the probing when booting on a system that explicitly called out
>>> that the hardware supports misaligned accesses.
Samuel Holland July 1, 2024, 3:13 p.m. UTC | #9
Hi Jesse,

On 2024-06-24 7:49 PM, Jesse Taube wrote:
> Run a unaligned vector access to test if the system supports
> vector unaligned access. Add the result to a new key in hwprobe.
> This is useful for usermode to know if vector misaligned accesses are
> supported and if they are faster or slower than equivalent byte accesses.
> 
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> V1 -> V2:

You are missing the "---" here, so the changelog is leaking into the commit message.

>  - Add Kconfig options
>  - Add insn_is_vector
>  - Add handle_vector_misaligned_load
>  - Fix build
>  - Seperate vector from scalar misaligned access
>  - This patch was almost completely rewritten
> V2 -> V3:
>  - Fixed CONFIG_ in Kconfig
>  - Fixed check_vector_unaligned_access_emulated leaving
>      vector_misaligned_access as unknown.
>  - Remove local_irq_enable
>  - Remove RISCV_DETECT_VECTOR_UNALIGNED_ACCESS
>  - Remove RISCV_VEC_UNALIGNED_ACCESS_UNSUPPORTED
> ---
>  arch/riscv/Kconfig                         |  35 ++++++
>  arch/riscv/include/asm/cpufeature.h        |   5 +
>  arch/riscv/include/asm/entry-common.h      |  11 --
>  arch/riscv/include/asm/hwprobe.h           |   2 +-
>  arch/riscv/include/asm/vector.h            |   1 +
>  arch/riscv/include/uapi/asm/hwprobe.h      |   5 +
>  arch/riscv/kernel/Makefile                 |   4 +-
>  arch/riscv/kernel/sys_hwprobe.c            |  35 ++++++
>  arch/riscv/kernel/traps_misaligned.c       | 120 ++++++++++++++++++++-
>  arch/riscv/kernel/unaligned_access_speed.c |   9 +-
>  arch/riscv/kernel/vector.c                 |   2 +-
>  11 files changed, 209 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 34d24242e37a..ffbe0fdd7fb3 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -717,12 +717,26 @@ config THREAD_SIZE_ORDER
>  	  Specify the Pages of thread stack size (from 4KB to 64KB), which also
>  	  affects irq stack size, which is equal to thread stack size.
>  
> +config RISCV_MISALIGNED
> +	bool
> +	help
> +	  Embed support for detecting and emulating misaligned
> +	  scalar or vector loads and stores.
> +
>  config RISCV_SCALAR_MISALIGNED
>  	bool
> +	select RISCV_MISALIGNED
>  	select SYSCTL_ARCH_UNALIGN_ALLOW
>  	help
>  	  Embed support for emulating misaligned loads and stores.
>  
> +config RISCV_VECTOR_MISALIGNED
> +	bool
> +	select RISCV_MISALIGNED
> +	depends on RISCV_ISA_V
> +	help
> +	  Enable detecting support for vector misaligned loads and stores.
> +
>  choice
>  	prompt "Unaligned Accesses Support"
>  	default RISCV_PROBE_UNALIGNED_ACCESS
> @@ -774,6 +788,27 @@ config RISCV_EFFICIENT_UNALIGNED_ACCESS
>  
>  endchoice
>  
> +choice
> +	prompt "Vector unaligned Accesses Support"
> +	depends on RISCV_ISA_V
> +	default RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
> +	help
> +	  This determines the level of support for vector unaligned accesses. This
> +	  information is used by the kernel to perform optimizations. It is also
> +	  exposed to user space via the hwprobe syscall. The hardware will be
> +	  probed at boot by default.
> +
> +config RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
> +	bool "Probe speed of vector unaligned accesses"
> +	select RISCV_VECTOR_MISALIGNED
> +	help
> +	  During boot, the kernel will run a series of tests to determine the
> +	  speed of vector unaligned accesses if they are supported. This probing
> +	  will dynamically determine the speed of vector unaligned accesses on
> +	  the underlying system if they are supported.
> +
> +endchoice
> +
>  endmenu # "Platform type"
>  
>  menu "Kernel features"
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index 0ed7d99c14dd..f25f56f9bfaa 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -45,6 +45,11 @@ static inline bool unaligned_ctl_available(void)
>  }
>  #endif
>  
> +bool check_vector_unaligned_access_emulated_all_cpus(void);
> +#if defined(CONFIG_RISCV_VECTOR_MISALIGNED)
> +DECLARE_PER_CPU(long, vector_misaligned_access);
> +#endif
> +
>  #if defined(CONFIG_RISCV_PROBE_UNALIGNED_ACCESS)
>  DECLARE_STATIC_KEY_FALSE(fast_unaligned_access_speed_key);
>  
> diff --git a/arch/riscv/include/asm/entry-common.h b/arch/riscv/include/asm/entry-common.h
> index 0a4e3544c877..7b32d2b08bb6 100644
> --- a/arch/riscv/include/asm/entry-common.h
> +++ b/arch/riscv/include/asm/entry-common.h
> @@ -25,18 +25,7 @@ static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
>  void handle_page_fault(struct pt_regs *regs);
>  void handle_break(struct pt_regs *regs);
>  
> -#ifdef CONFIG_RISCV_SCALAR_MISALIGNED
>  int handle_misaligned_load(struct pt_regs *regs);
>  int handle_misaligned_store(struct pt_regs *regs);
> -#else
> -static inline int handle_misaligned_load(struct pt_regs *regs)
> -{
> -	return -1;
> -}
> -static inline int handle_misaligned_store(struct pt_regs *regs)
> -{
> -	return -1;
> -}
> -#endif
>  
>  #endif /* _ASM_RISCV_ENTRY_COMMON_H */
> diff --git a/arch/riscv/include/asm/hwprobe.h b/arch/riscv/include/asm/hwprobe.h
> index 150a9877b0af..ef01c182af2b 100644
> --- a/arch/riscv/include/asm/hwprobe.h
> +++ b/arch/riscv/include/asm/hwprobe.h
> @@ -8,7 +8,7 @@
>  
>  #include <uapi/asm/hwprobe.h>
>  
> -#define RISCV_HWPROBE_MAX_KEY 7
> +#define RISCV_HWPROBE_MAX_KEY 8
>  
>  static inline bool riscv_hwprobe_key_is_valid(__s64 key)
>  {
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index be7d309cca8a..99b0f91db9ee 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -21,6 +21,7 @@
>  
>  extern unsigned long riscv_v_vsize;
>  int riscv_v_setup_vsize(void);
> +bool insn_is_vector(u32 insn_buf);
>  bool riscv_v_first_use_handler(struct pt_regs *regs);
>  void kernel_vector_begin(void);
>  void kernel_vector_end(void);
> diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
> index 023b7771d1b7..2fee870e41bb 100644
> --- a/arch/riscv/include/uapi/asm/hwprobe.h
> +++ b/arch/riscv/include/uapi/asm/hwprobe.h
> @@ -75,6 +75,11 @@ struct riscv_hwprobe {
>  #define		RISCV_HWPROBE_MISALIGNED_MASK		(7 << 0)
>  #define RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE	6
>  #define RISCV_HWPROBE_KEY_MISALIGNED_PERF	7
> +#define RISCV_HWPROBE_KEY_VEC_MISALIGNED_PERF	8
> +#define		RISCV_HWPROBE_VEC_MISALIGNED_UNKNOWN		0
> +#define		RISCV_HWPROBE_VEC_MISALIGNED_SLOW		2
> +#define		RISCV_HWPROBE_VEC_MISALIGNED_FAST		3
> +#define		RISCV_HWPROBE_VEC_MISALIGNED_UNSUPPORTED	4

We are spelling out "SCALAR" in the fixes series here[1], so for consistency we
should spell out "VECTOR" as well.

Regards,
Samuel

[1]: https://lore.kernel.org/linux-riscv/20240627172238.2460840-2-evan@rivosinc.com/
Evan Green July 1, 2024, 10:51 p.m. UTC | #10
On Mon, Jun 24, 2024 at 5:52 PM Jesse Taube <jesse@rivosinc.com> wrote:
>
> Detect if vector misaligned accesses are faster or slower than
> equivalent vector byte accesses. This is useful for usermode to know
> whether vector byte accesses or vector misaligned accesses have a better
> bandwidth for operations like memcpy.
>
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> ---
> V1 -> V2:
>  - Add Kconfig options
>  - Add WORD_EEW to vec-copy-unaligned.S
> V2 -> V3:
>  - Remove unnecessary comment
>  - Remove local_irq_enable
> ---
>  arch/riscv/Kconfig                         |  18 +++
>  arch/riscv/kernel/Makefile                 |   3 +-
>  arch/riscv/kernel/copy-unaligned.h         |   5 +
>  arch/riscv/kernel/sys_hwprobe.c            |   6 +
>  arch/riscv/kernel/unaligned_access_speed.c | 132 ++++++++++++++++++++-
>  arch/riscv/kernel/vec-copy-unaligned.S     |  58 +++++++++
>  6 files changed, 219 insertions(+), 3 deletions(-)
>  create mode 100644 arch/riscv/kernel/vec-copy-unaligned.S
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index ffbe0fdd7fb3..6f9fd3748916 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -807,6 +807,24 @@ config RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
>           will dynamically determine the speed of vector unaligned accesses on
>           the underlying system if they are supported.
>
> +config RISCV_SLOW_VEC_UNALIGNED_ACCESS
> +       bool "Assume the system supports slow vector unaligned memory accesses"
> +       depends on NONPORTABLE
> +       help
> +         Assume that the system supports slow vector unaligned memory accesses. The
> +         kernel and userspace programs may not be able to run at all on systems
> +         that do not support unaligned memory accesses.
> +
> +config RISCV_EFFICIENT_VEC_UNALIGNED_ACCESS
> +       bool "Assume the system supports fast vector unaligned memory accesses"
> +       depends on NONPORTABLE
> +       help
> +         Assume that the system supports fast vector unaligned memory accesses. When
> +         enabled, this option improves the performance of the kernel on such
> +         systems. However, the kernel and userspace programs will run much more
> +         slowly, or will not be able to run at all, on systems that do not
> +         support efficient unaligned memory accesses.
> +
>  endchoice
>
>  endmenu # "Platform type"
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index 5b243d46f4b1..291935a084d5 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -64,7 +64,8 @@ obj-$(CONFIG_MMU) += vdso.o vdso/
>
>  obj-$(CONFIG_RISCV_MISALIGNED) += traps_misaligned.o
>  obj-$(CONFIG_RISCV_MISALIGNED) += unaligned_access_speed.o
> -obj-$(CONFIG_RISCV_PROBE_UNALIGNED_ACCESS)     += copy-unaligned.o
> +obj-$(CONFIG_RISCV_PROBE_UNALIGNED_ACCESS)             += copy-unaligned.o
> +obj-$(CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS)      += vec-copy-unaligned.o
>
>  obj-$(CONFIG_FPU)              += fpu.o
>  obj-$(CONFIG_FPU)              += kernel_mode_fpu.o
> diff --git a/arch/riscv/kernel/copy-unaligned.h b/arch/riscv/kernel/copy-unaligned.h
> index e3d70d35b708..85d4d11450cb 100644
> --- a/arch/riscv/kernel/copy-unaligned.h
> +++ b/arch/riscv/kernel/copy-unaligned.h
> @@ -10,4 +10,9 @@
>  void __riscv_copy_words_unaligned(void *dst, const void *src, size_t size);
>  void __riscv_copy_bytes_unaligned(void *dst, const void *src, size_t size);
>
> +#ifdef CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
> +void __riscv_copy_vec_words_unaligned(void *dst, const void *src, size_t size);
> +void __riscv_copy_vec_bytes_unaligned(void *dst, const void *src, size_t size);
> +#endif
> +
>  #endif /* __RISCV_KERNEL_COPY_UNALIGNED_H */
> diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> index 5b78ea5a84d1..46de3f145154 100644
> --- a/arch/riscv/kernel/sys_hwprobe.c
> +++ b/arch/riscv/kernel/sys_hwprobe.c
> @@ -221,6 +221,12 @@ static u64 hwprobe_vec_misaligned(const struct cpumask *cpus)
>  #else
>  static u64 hwprobe_vec_misaligned(const struct cpumask *cpus)
>  {
> +       if (IS_ENABLED(CONFIG_RISCV_EFFICIENT_VEC_UNALIGNED_ACCESS))
> +               return RISCV_HWPROBE_VEC_MISALIGNED_FAST;
> +
> +       if (IS_ENABLED(CONFIG_RISCV_SLOW_VEC_UNALIGNED_ACCESS))
> +               return RISCV_HWPROBE_VEC_MISALIGNED_SLOW;
> +
>         return RISCV_HWPROBE_VEC_MISALIGNED_UNKNOWN;
>  }
>  #endif
> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
> index 8489e012cf23..a2a8f948500b 100644
> --- a/arch/riscv/kernel/unaligned_access_speed.c
> +++ b/arch/riscv/kernel/unaligned_access_speed.c
> @@ -8,9 +8,11 @@
>  #include <linux/jump_label.h>
>  #include <linux/mm.h>
>  #include <linux/smp.h>
> +#include <linux/kthread.h>
>  #include <linux/types.h>
>  #include <asm/cpufeature.h>
>  #include <asm/hwprobe.h>
> +#include <asm/vector.h>
>
>  #include "copy-unaligned.h"
>
> @@ -267,9 +269,129 @@ static int check_unaligned_access_speed_all_cpus(void)
>  }
>  #endif
>
> +#ifdef CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
> +static void check_vector_unaligned_access(struct work_struct *unused)
> +{
> +       int cpu = smp_processor_id();
> +       u64 start_cycles, end_cycles;
> +       u64 word_cycles;
> +       u64 byte_cycles;
> +       int ratio;
> +       unsigned long start_jiffies, now;
> +       struct page *page;
> +       void *dst;
> +       void *src;
> +       long speed = RISCV_HWPROBE_VEC_MISALIGNED_SLOW;
> +
> +       if (per_cpu(vector_misaligned_access, cpu) != RISCV_HWPROBE_VEC_MISALIGNED_SLOW)
> +               return;
> +
> +       page = alloc_pages(GFP_KERNEL, MISALIGNED_BUFFER_ORDER);
> +       if (!page) {
> +               pr_warn("Allocation failure, not measuring vector misaligned performance\n");
> +               return;
> +       }
> +
> +       /* Make an unaligned destination buffer. */
> +       dst = (void *)((unsigned long)page_address(page) | 0x1);
> +       /* Unalign src as well, but differently (off by 1 + 2 = 3). */
> +       src = dst + (MISALIGNED_BUFFER_SIZE / 2);
> +       src += 2;
> +       word_cycles = -1ULL;
> +
> +       /* Do a warmup. */
> +       kernel_vector_begin();

Should there be a preempt_disable() in here too, or is that implied
already by schedule_on_each_cpu? Mainly we want to minimize the amount
of things that might soak up our jiffy and steal iterations from the
loop.

> +       __riscv_copy_vec_words_unaligned(dst, src, MISALIGNED_COPY_SIZE);
> +
> +       start_jiffies = jiffies;
> +       while ((now = jiffies) == start_jiffies)
> +               cpu_relax();
> +
> +       /*
> +        * For a fixed amount of time, repeatedly try the function, and take
> +        * the best time in cycles as the measurement.
> +        */
> +       while (time_before(jiffies, now + (1 << MISALIGNED_ACCESS_JIFFIES_LG2))) {
> +               start_cycles = get_cycles64();
> +               /* Ensure the CSR read can't reorder WRT to the copy. */
> +               mb();
> +               __riscv_copy_vec_words_unaligned(dst, src, MISALIGNED_COPY_SIZE);
> +               /* Ensure the copy ends before the end time is snapped. */
> +               mb();
> +               end_cycles = get_cycles64();
> +               if ((end_cycles - start_cycles) < word_cycles)
> +                       word_cycles = end_cycles - start_cycles;
> +       }
> +
> +       byte_cycles = -1ULL;
> +       __riscv_copy_vec_bytes_unaligned(dst, src, MISALIGNED_COPY_SIZE);
> +       start_jiffies = jiffies;
> +       while ((now = jiffies) == start_jiffies)
> +               cpu_relax();
> +
> +       while (time_before(jiffies, now + (1 << MISALIGNED_ACCESS_JIFFIES_LG2))) {
> +               start_cycles = get_cycles64();
> +               mb();
> +               __riscv_copy_vec_bytes_unaligned(dst, src, MISALIGNED_COPY_SIZE);
> +               mb();
> +               end_cycles = get_cycles64();
> +               if ((end_cycles - start_cycles) < byte_cycles)
> +                       byte_cycles = end_cycles - start_cycles;
> +       }
> +
> +       kernel_vector_end();
> +
> +       /* Don't divide by zero. */
> +       if (!word_cycles || !byte_cycles) {
> +               pr_warn("cpu%d: rdtime lacks granularity needed to measure unaligned vector access speed\n",
> +                       cpu);
> +
> +               return;
> +       }
> +
> +       if (word_cycles < byte_cycles)
> +               speed = RISCV_HWPROBE_VEC_MISALIGNED_FAST;
> +
> +       ratio = div_u64((byte_cycles * 100), word_cycles);
> +       pr_info("cpu%d: Ratio of vector byte access time to vector unaligned word access is %d.%02d, unaligned accesses are %s\n",
> +               cpu,
> +               ratio / 100,
> +               ratio % 100,
> +               (speed ==  RISCV_HWPROBE_VEC_MISALIGNED_FAST) ? "fast" : "slow");
> +
> +       per_cpu(vector_misaligned_access, cpu) = speed;
> +}
> +
> +static int riscv_online_cpu_vec(unsigned int cpu)
> +{
> +       check_vector_unaligned_access(NULL);
> +       return 0;
> +}
> +
> +/* Measure unaligned access speed on all CPUs present at boot in parallel. */
> +static int vec_check_unaligned_access_speed_all_cpus(void *unused)
> +{
> +       schedule_on_each_cpu(check_vector_unaligned_access);
> +
> +       /*
> +        * Setup hotplug callbacks for any new CPUs that come online or go
> +        * offline.
> +        */
> +       cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "riscv:online",
> +                                 riscv_online_cpu_vec, NULL);
> +
> +       return 0;
> +}
> +#else /* CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS */
> +static int vec_check_unaligned_access_speed_all_cpus(void *unused)
> +{
> +       return 0;
> +}
> +#endif
> +
>  static int check_unaligned_access_all_cpus(void)
>  {
> -       bool all_cpus_emulated;
> +       bool all_cpus_emulated, all_cpus_vec_supported;
>         int cpu;
>
>         if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
> @@ -285,7 +407,13 @@ static int check_unaligned_access_all_cpus(void)
>         }
>
>         all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
> -       check_vector_unaligned_access_emulated_all_cpus();
> +       all_cpus_vec_supported = check_vector_unaligned_access_emulated_all_cpus();
> +
> +       if (all_cpus_vec_supported &&

Note that with this here, if a heterogeneous system ever shows up with
a mix of supported/unsupported, the supported cpus will end up with a
misaligned vector speed of UNKNOWN. It might be nicer to change the
semantics of that return value from all_cpus_vec_supported (aka
entirely_supported) to entirely_unsupported. Then you'd only fire up
the thread if (!entirely_unsupported). If you did that, you'd also
need to bail early in the check on any CPUs that already had their
value set to UNSUPPORTED. That way we can still avoid firing up this
thread for machines that don't have misaligned V (or V at all), but
not leave the question UNKNOWN in some cases.

> +           IS_ENABLED(CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS)) {
> +               kthread_run(vec_check_unaligned_access_speed_all_cpus,
> +                           NULL, "vec_check_unaligned_access_speed_all_cpus");
> +       }
>
>         if (!all_cpus_emulated)
>                 return check_unaligned_access_speed_all_cpus();
> diff --git a/arch/riscv/kernel/vec-copy-unaligned.S b/arch/riscv/kernel/vec-copy-unaligned.S
> new file mode 100644
> index 000000000000..e5bc94917e60
> --- /dev/null
> +++ b/arch/riscv/kernel/vec-copy-unaligned.S
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2024 Rivos Inc. */
> +
> +#include <linux/linkage.h>
> +#include <asm/asm.h>
> +#include <linux/args.h>
> +
> +       .text
> +
> +#define WORD_EEW 32
> +
> +#define WORD_SEW CONCATENATE(e, WORD_EEW)
> +#define VEC_L CONCATENATE(vle, WORD_EEW).v
> +#define VEC_S CONCATENATE(vle, WORD_EEW).v
> +
> +/* void __riscv_copy_vec_words_unaligned(void *, const void *, size_t) */
> +/* Performs a memcpy without aligning buffers, using word loads and stores. */
> +/* Note: The size is truncated to a multiple of WORD_EEW */
> +SYM_FUNC_START(__riscv_copy_vec_words_unaligned)
> +       andi  a4, a2, ~(WORD_EEW-1)
> +       beqz  a4, 2f
> +       add   a3, a1, a4
> +       .option push
> +       .option arch, +zve32x
> +1:
> +       vsetivli t0, 8, WORD_SEW, m8, ta, ma
> +       VEC_L v0, (a1)
> +       VEC_S v0, (a0)
> +       addi  a0, a0, WORD_EEW
> +       addi  a1, a1, WORD_EEW
> +       bltu  a1, a3, 1b
> +
> +2:
> +       .option pop
> +       ret
> +SYM_FUNC_END(__riscv_copy_vec_words_unaligned)
> +
> +/* void __riscv_copy_vec_bytes_unaligned(void *, const void *, size_t) */
> +/* Performs a memcpy without aligning buffers, using only byte accesses. */
> +/* Note: The size is truncated to a multiple of 8 */
> +SYM_FUNC_START(__riscv_copy_vec_bytes_unaligned)
> +       andi a4, a2, ~(8-1)
> +       beqz a4, 2f
> +       add  a3, a1, a4
> +       .option push
> +       .option arch, +zve32x
> +1:
> +       vsetivli t0, 8, e8, m8, ta, ma
> +       vle8.v v0, (a1)
> +       vse8.v v0, (a0)
> +       addi a0, a0, 8
> +       addi a1, a1, 8
> +       bltu a1, a3, 1b
> +
> +2:
> +       .option pop
> +       ret
> +SYM_FUNC_END(__riscv_copy_vec_bytes_unaligned)
> --
> 2.45.2
>
Evan Green July 1, 2024, 10:55 p.m. UTC | #11
On Mon, Jun 24, 2024 at 5:52 PM Jesse Taube <jesse@rivosinc.com> wrote:
>
> Document key for reporting the speed of unaligned vector accesses.
> The descriptions are the same as the scalar equivalent values.
>
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> ---
> V1 -> V2:
>   - New patch
> V2 -> V3:
>  - Specify access width
> ---
>  Documentation/arch/riscv/hwprobe.rst | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/arch/riscv/hwprobe.rst b/Documentation/arch/riscv/hwprobe.rst
> index 7085a694b801..d102b4a16d55 100644
> --- a/Documentation/arch/riscv/hwprobe.rst
> +++ b/Documentation/arch/riscv/hwprobe.rst
> @@ -236,3 +236,19 @@ The following keys are defined:
>
>  * :c:macro:`RISCV_HWPROBE_KEY_ZICBOZ_BLOCK_SIZE`: An unsigned int which
>    represents the size of the Zicboz block in bytes.
> +
> +* :c:macro:`RISCV_HWPROBE_KEY_VEC_MISALIGNED_PERF`: An enum value describing the
> +  performance of misaligned vector accesses on the selected set of processors.
> +
> +  * :c:macro:`RISCV_HWPROBE_VEC_MISALIGNED_UNKNOWN`: The performance of misaligned
> +    accesses is unknown.
> +
> +  * :c:macro:`RISCV_HWPROBE_VEC_MISALIGNED_SLOW`: 32bit misaligned accesses are slower
> +    than equivalent byte accesses.  Misaligned accesses may be supported

Do you think it's worth specifying that we're talking about byte
accesses using vector registers? In other words, clarifying that we're
not comparing misaligned vector loads to loads/stores into the scalar
registers. Maybe something like:

32-bit misaligned accesses using vector registers are slower than the
equivalent quantity of byte accesses via vector registers. Misaligned
accesses may ...

-Evan

> +    directly in hardware, or trapped and emulated by software.
> +
> +  * :c:macro:`RISCV_HWPROBE_VEC_MISALIGNED_FAST`: 32bit misaligned accesses are faster
> +    than equivalent byte accesses.
> +
> +  * :c:macro:`RISCV_HWPROBE_VEC_MISALIGNED_UNSUPPORTED`: Misaligned accesses are
> +    not supported at all and will generate a misaligned address fault.
> --
> 2.45.2
>
Charlie Jenkins July 2, 2024, 10:22 p.m. UTC | #12
On Mon, Jul 01, 2024 at 04:20:15PM +0200, Clément Léger wrote:
> 
> 
> On 01/07/2024 15:58, Conor Dooley wrote:
> > On Mon, Jul 01, 2024 at 09:15:09AM +0200, Clément Léger wrote:
> >>
> >>
> >> On 27/06/2024 23:20, Charlie Jenkins wrote:
> >>> On Wed, Jun 26, 2024 at 03:39:14PM +0100, Conor Dooley wrote:
> >>>> On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
> >>>>> Check for Zicclsm before checking for unaligned access speed. This will
> >>>>> greatly reduce the boot up time as finding the access speed is no longer
> >>>>> necessary.
> >>>>>
> >>>>> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> >>>>> ---
> >>>>> V2 -> V3:
> >>>>>  - New patch split from previous patch
> >>>>> ---
> >>>>>  arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
> >>>>>  1 file changed, 17 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
> >>>>> index a9a6bcb02acf..329fd289b5c8 100644
> >>>>> --- a/arch/riscv/kernel/unaligned_access_speed.c
> >>>>> +++ b/arch/riscv/kernel/unaligned_access_speed.c
> >>>>> @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
> >>>>>  	kfree(bufs);
> >>>>>  	return 0;
> >>>>>  }
> >>>>> +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
> >>>>> +static int check_unaligned_access_speed_all_cpus(void)
> >>>>> +{
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +#endif
> >>>>>  
> >>>>>  static int check_unaligned_access_all_cpus(void)
> >>>>>  {
> >>>>> -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
> >>>>> +	bool all_cpus_emulated;
> >>>>> +	int cpu;
> >>>>> +
> >>>>> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
> >>>>> +		for_each_online_cpu(cpu) {
> >>>>> +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;
> >>>>
> >>>> - const: zicclsm
> >>>>   description:
> >>>>     The standard Zicclsm extension for misaligned support for all regular
> >>>>     load and store instructions (including scalar and vector) but not AMOs
> >>>>     or other specialized forms of memory access. Defined in the
> >>>>     RISC-V RVA Profiles Specification. 
> >>>>
> >>>> Doesn't, unfortunately, say anywhere there that they're actually fast :(
> >>>
> >>> Oh no! That is unfortunate that the ISA does not explicitly call that
> >>> out, but I think that acceptable.
> >>>
> >>> If a vendor puts Zicclsm in their isa string, they should expect
> >>> software to take advantage of misaligned accesses. FAST is our signal to
> >>> tell software that they should emit misaligned accesses.
> >>
> >> AFAIK, Zicclsm is not even an ISA extension, simply a profile
> >> specification which means that only the execution environment which
> >> provides the profile support misaligned accesses (cf
> >> https://lists.riscv.org/g/tech-profiles/message/56).
> > 
> > I dunno, the specification status page used to describe it as an
> > extension:
> > https://wiki.riscv.org/display/HOME/Specification+Status+-+Historical
> > My understanding was that these could be considered extensions, just
> > like we are considering svade to be one.
> > 
> >> . I don't think we
> >> can extrapolate that the misaligned accesses will be fast at all.
> > 
> > That is my opinion on it too. If it doesn't say "fast" and give a
> > definition for what that means in the binding, then we can't assume that
> > it is fast. I'm also wary of extending definitions of extensions in the
> > binding, because a) I am 90% sure that people writing devicetrees don't
> > care and b) it'd be a potential difference between DT and ACPI without a
> > real justification (unlike the zkr or svade/svadu situations).
> 
> BTW, the profile spec [1] has a note that states the following for Zicclsm:
> 
> "Even though mandated, misaligned loads and stores might execute
> extremely slowly. Standard software distributions should assume their
> existence only for correctness, not for performance."
> 
> Which was also quoted in patch 1, so I guess that settles it.

The intention here was to allow vendors to configure an option to skip
the probing. This extension does not seem useful as it is written! A way
around this would be to add a kernel arg to set the access speed but
maybe it doesn't matter. For the sake of this patch, it looks like we
should get rid of this Zicclsm check.

- Charlie

> 
> Thanks,
> 
> Clément
> 
> Link:
> https://github.com/riscv/riscv-profiles/blob/main/src/profiles.adoc?plain=1#L524
> [1]
> 
> > 
> >>> This allows for a generic kernel, like the one a distro would compile, to
> >>> skip the probing when booting on a system that explicitly called out
> >>> that the hardware supports misaligned accesses.
Clément Léger July 3, 2024, 7:13 a.m. UTC | #13
On 03/07/2024 00:22, Charlie Jenkins wrote:
> On Mon, Jul 01, 2024 at 04:20:15PM +0200, Clément Léger wrote:
>>
>>
>> On 01/07/2024 15:58, Conor Dooley wrote:
>>> On Mon, Jul 01, 2024 at 09:15:09AM +0200, Clément Léger wrote:
>>>>
>>>>
>>>> On 27/06/2024 23:20, Charlie Jenkins wrote:
>>>>> On Wed, Jun 26, 2024 at 03:39:14PM +0100, Conor Dooley wrote:
>>>>>> On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
>>>>>>> Check for Zicclsm before checking for unaligned access speed. This will
>>>>>>> greatly reduce the boot up time as finding the access speed is no longer
>>>>>>> necessary.
>>>>>>>
>>>>>>> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
>>>>>>> ---
>>>>>>> V2 -> V3:
>>>>>>>  - New patch split from previous patch
>>>>>>> ---
>>>>>>>  arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
>>>>>>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>> index a9a6bcb02acf..329fd289b5c8 100644
>>>>>>> --- a/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>> +++ b/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>> @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
>>>>>>>  	kfree(bufs);
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>> +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
>>>>>>> +static int check_unaligned_access_speed_all_cpus(void)
>>>>>>> +{
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +#endif
>>>>>>>  
>>>>>>>  static int check_unaligned_access_all_cpus(void)
>>>>>>>  {
>>>>>>> -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
>>>>>>> +	bool all_cpus_emulated;
>>>>>>> +	int cpu;
>>>>>>> +
>>>>>>> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
>>>>>>> +		for_each_online_cpu(cpu) {
>>>>>>> +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;
>>>>>>
>>>>>> - const: zicclsm
>>>>>>   description:
>>>>>>     The standard Zicclsm extension for misaligned support for all regular
>>>>>>     load and store instructions (including scalar and vector) but not AMOs
>>>>>>     or other specialized forms of memory access. Defined in the
>>>>>>     RISC-V RVA Profiles Specification. 
>>>>>>
>>>>>> Doesn't, unfortunately, say anywhere there that they're actually fast :(
>>>>>
>>>>> Oh no! That is unfortunate that the ISA does not explicitly call that
>>>>> out, but I think that acceptable.
>>>>>
>>>>> If a vendor puts Zicclsm in their isa string, they should expect
>>>>> software to take advantage of misaligned accesses. FAST is our signal to
>>>>> tell software that they should emit misaligned accesses.
>>>>
>>>> AFAIK, Zicclsm is not even an ISA extension, simply a profile
>>>> specification which means that only the execution environment which
>>>> provides the profile support misaligned accesses (cf
>>>> https://lists.riscv.org/g/tech-profiles/message/56).
>>>
>>> I dunno, the specification status page used to describe it as an
>>> extension:
>>> https://wiki.riscv.org/display/HOME/Specification+Status+-+Historical
>>> My understanding was that these could be considered extensions, just
>>> like we are considering svade to be one.
>>>
>>>> . I don't think we
>>>> can extrapolate that the misaligned accesses will be fast at all.
>>>
>>> That is my opinion on it too. If it doesn't say "fast" and give a
>>> definition for what that means in the binding, then we can't assume that
>>> it is fast. I'm also wary of extending definitions of extensions in the
>>> binding, because a) I am 90% sure that people writing devicetrees don't
>>> care and b) it'd be a potential difference between DT and ACPI without a
>>> real justification (unlike the zkr or svade/svadu situations).
>>
>> BTW, the profile spec [1] has a note that states the following for Zicclsm:
>>
>> "Even though mandated, misaligned loads and stores might execute
>> extremely slowly. Standard software distributions should assume their
>> existence only for correctness, not for performance."
>>
>> Which was also quoted in patch 1, so I guess that settles it.
> 
> The intention here was to allow vendors to configure an option to skip
> the probing. This extension does not seem useful as it is written! A way
> around this would be to add a kernel arg to set the access speed but
> maybe it doesn't matter. For the sake of this patch, it looks like we
> should get rid of this Zicclsm check.

I think a parameter could be appropriate for vendors that want to skip
the probing and gain a bit of time on boot time. Other options already
exists to force specific settings so, why not !

Thanks,

Clément

> 
> - Charlie
> 
>>
>> Thanks,
>>
>> Clément
>>
>> Link:
>> https://github.com/riscv/riscv-profiles/blob/main/src/profiles.adoc?plain=1#L524
>> [1]
>>
>>>
>>>>> This allows for a generic kernel, like the one a distro would compile, to
>>>>> skip the probing when booting on a system that explicitly called out
>>>>> that the hardware supports misaligned accesses.
Jesse Taube July 3, 2024, 9:47 p.m. UTC | #14
On 7/3/24 03:13, Clément Léger wrote:
> 
> 
> On 03/07/2024 00:22, Charlie Jenkins wrote:
>> On Mon, Jul 01, 2024 at 04:20:15PM +0200, Clément Léger wrote:
>>>
>>>
>>> On 01/07/2024 15:58, Conor Dooley wrote:
>>>> On Mon, Jul 01, 2024 at 09:15:09AM +0200, Clément Léger wrote:
>>>>>
>>>>>
>>>>> On 27/06/2024 23:20, Charlie Jenkins wrote:
>>>>>> On Wed, Jun 26, 2024 at 03:39:14PM +0100, Conor Dooley wrote:
>>>>>>> On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
>>>>>>>> Check for Zicclsm before checking for unaligned access speed. This will
>>>>>>>> greatly reduce the boot up time as finding the access speed is no longer
>>>>>>>> necessary.
>>>>>>>>
>>>>>>>> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
>>>>>>>> ---
>>>>>>>> V2 -> V3:
>>>>>>>>   - New patch split from previous patch
>>>>>>>> ---
>>>>>>>>   arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
>>>>>>>>   1 file changed, 17 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>>> index a9a6bcb02acf..329fd289b5c8 100644
>>>>>>>> --- a/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>>> +++ b/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>>> @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
>>>>>>>>   	kfree(bufs);
>>>>>>>>   	return 0;
>>>>>>>>   }
>>>>>>>> +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
>>>>>>>> +static int check_unaligned_access_speed_all_cpus(void)
>>>>>>>> +{
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +#endif
>>>>>>>>   
>>>>>>>>   static int check_unaligned_access_all_cpus(void)
>>>>>>>>   {
>>>>>>>> -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
>>>>>>>> +	bool all_cpus_emulated;
>>>>>>>> +	int cpu;
>>>>>>>> +
>>>>>>>> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
>>>>>>>> +		for_each_online_cpu(cpu) {
>>>>>>>> +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;
>>>>>>>
>>>>>>> - const: zicclsm
>>>>>>>    description:
>>>>>>>      The standard Zicclsm extension for misaligned support for all regular
>>>>>>>      load and store instructions (including scalar and vector) but not AMOs
>>>>>>>      or other specialized forms of memory access. Defined in the
>>>>>>>      RISC-V RVA Profiles Specification.
>>>>>>>
>>>>>>> Doesn't, unfortunately, say anywhere there that they're actually fast :(
>>>>>>
>>>>>> Oh no! That is unfortunate that the ISA does not explicitly call that
>>>>>> out, but I think that acceptable.
>>>>>>
>>>>>> If a vendor puts Zicclsm in their isa string, they should expect
>>>>>> software to take advantage of misaligned accesses. FAST is our signal to
>>>>>> tell software that they should emit misaligned accesses.
>>>>>
>>>>> AFAIK, Zicclsm is not even an ISA extension, simply a profile
>>>>> specification which means that only the execution environment which
>>>>> provides the profile support misaligned accesses (cf
>>>>> https://lists.riscv.org/g/tech-profiles/message/56).
>>>>
>>>> I dunno, the specification status page used to describe it as an
>>>> extension:
>>>> https://wiki.riscv.org/display/HOME/Specification+Status+-+Historical
>>>> My understanding was that these could be considered extensions, just
>>>> like we are considering svade to be one.
>>>>
>>>>> . I don't think we
>>>>> can extrapolate that the misaligned accesses will be fast at all.
>>>>
>>>> That is my opinion on it too. If it doesn't say "fast" and give a
>>>> definition for what that means in the binding, then we can't assume that
>>>> it is fast. I'm also wary of extending definitions of extensions in the
>>>> binding, because a) I am 90% sure that people writing devicetrees don't
>>>> care and b) it'd be a potential difference between DT and ACPI without a
>>>> real justification (unlike the zkr or svade/svadu situations).
>>>
>>> BTW, the profile spec [1] has a note that states the following for Zicclsm:
>>>
>>> "Even though mandated, misaligned loads and stores might execute
>>> extremely slowly. Standard software distributions should assume their
>>> existence only for correctness, not for performance."
>>>
>>> Which was also quoted in patch 1, so I guess that settles it.
>>
>> The intention here was to allow vendors to configure an option to skip
>> the probing. This extension does not seem useful as it is written! A way
>> around this would be to add a kernel arg to set the access speed but
>> maybe it doesn't matter. For the sake of this patch, it looks like we
>> should get rid of this Zicclsm check.
> 
> I think a parameter could be appropriate for vendors that want to skip
> the probing and gain a bit of time on boot time. Other options already
> exists to force specific settings so, why not !

Sounds like a good solution thanks for the idea Charlie!
I will drop this patch and send a separate patch to implement two kernel 
parameters for skipping(and setting the speed of) vector and scalar 
speed tests.

Thanks,
Jesse Taube

> 
> Thanks,
> 
> Clément
> 
>>
>> - Charlie
>>
>>>
>>> Thanks,
>>>
>>> Clément
>>>
>>> Link:
>>> https://github.com/riscv/riscv-profiles/blob/main/src/profiles.adoc?plain=1#L524
>>> [1]
>>>
>>>>
>>>>>> This allows for a generic kernel, like the one a distro would compile, to
>>>>>> skip the probing when booting on a system that explicitly called out
>>>>>> that the hardware supports misaligned accesses.
Evan Green July 10, 2024, 3:55 p.m. UTC | #15
On Mon, Jun 24, 2024 at 5:51 PM Jesse Taube <jesse@rivosinc.com> wrote:
>
> Originally, the check_unaligned_access_emulated_all_cpus function
> only checked the boot hart. This fixes the function to check all
> harts.
>
> Fixes: 71c54b3d169d ("riscv: report misaligned accesses emulation to hwprobe")
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> Cc: stable@vger.kernel.org
> ---
> V1 -> V2:
>  - New patch
> V2 -> V3:
>  - Split patch
> ---
>  arch/riscv/kernel/traps_misaligned.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index b62d5a2f4541..8fadbe00dd62 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -526,31 +526,17 @@ int handle_misaligned_store(struct pt_regs *regs)
>         return 0;
>  }
>
> -static bool check_unaligned_access_emulated(int cpu)
> +static void check_unaligned_access_emulated(struct work_struct *unused)
>  {
> +       int cpu = smp_processor_id();
>         long *mas_ptr = per_cpu_ptr(&misaligned_access_speed, cpu);
>         unsigned long tmp_var, tmp_val;
> -       bool misaligned_emu_detected;
>
>         *mas_ptr = RISCV_HWPROBE_MISALIGNED_UNKNOWN;
>
>         __asm__ __volatile__ (
>                 "       "REG_L" %[tmp], 1(%[ptr])\n"
>                 : [tmp] "=r" (tmp_val) : [ptr] "r" (&tmp_var) : "memory");
> -
> -       misaligned_emu_detected = (*mas_ptr == RISCV_HWPROBE_MISALIGNED_EMULATED);
> -       /*
> -        * If unaligned_ctl is already set, this means that we detected that all
> -        * CPUS uses emulated misaligned access at boot time. If that changed
> -        * when hotplugging the new cpu, this is something we don't handle.
> -        */
> -       if (unlikely(unaligned_ctl && !misaligned_emu_detected)) {
> -               pr_crit("CPU misaligned accesses non homogeneous (expected all emulated)\n");
> -               while (true)
> -                       cpu_relax();
> -       }

This chunk was meant to detect and refuse to run on a system where a
heterogeneous CPU is hotplugged into a previously homogenous system.
The commit message doesn't mention this change, how come you
deleted it?


> -
> -       return misaligned_emu_detected;
>  }
>
>  bool check_unaligned_access_emulated_all_cpus(void)
> @@ -562,8 +548,11 @@ bool check_unaligned_access_emulated_all_cpus(void)
>          * accesses emulated since tasks requesting such control can run on any
>          * CPU.
>          */
> +       schedule_on_each_cpu(check_unaligned_access_emulated);
> +
>         for_each_online_cpu(cpu)
> -               if (!check_unaligned_access_emulated(cpu))
> +               if (per_cpu(misaligned_access_speed, cpu)
> +                   != RISCV_HWPROBE_MISALIGNED_EMULATED)
>                         return false;
>
>         unaligned_ctl = true;
> --
> 2.45.2
>
Jesse Taube July 11, 2024, 8:25 p.m. UTC | #16
On 7/10/24 11:55, Evan Green wrote:
> On Mon, Jun 24, 2024 at 5:51 PM Jesse Taube <jesse@rivosinc.com> wrote:
>>
>> Originally, the check_unaligned_access_emulated_all_cpus function
>> only checked the boot hart. This fixes the function to check all
>> harts.
>>
>> Fixes: 71c54b3d169d ("riscv: report misaligned accesses emulation to hwprobe")
>> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
>> Cc: stable@vger.kernel.org
>> ---
>> V1 -> V2:
>>   - New patch
>> V2 -> V3:
>>   - Split patch
>> ---
>>   arch/riscv/kernel/traps_misaligned.c | 23 ++++++-----------------
>>   1 file changed, 6 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
>> index b62d5a2f4541..8fadbe00dd62 100644
>> --- a/arch/riscv/kernel/traps_misaligned.c
>> +++ b/arch/riscv/kernel/traps_misaligned.c
>> @@ -526,31 +526,17 @@ int handle_misaligned_store(struct pt_regs *regs)
>>          return 0;
>>   }
>>
>> -static bool check_unaligned_access_emulated(int cpu)
>> +static void check_unaligned_access_emulated(struct work_struct *unused)
>>   {
>> +       int cpu = smp_processor_id();
>>          long *mas_ptr = per_cpu_ptr(&misaligned_access_speed, cpu);
>>          unsigned long tmp_var, tmp_val;
>> -       bool misaligned_emu_detected;
>>
>>          *mas_ptr = RISCV_HWPROBE_MISALIGNED_UNKNOWN;
>>
>>          __asm__ __volatile__ (
>>                  "       "REG_L" %[tmp], 1(%[ptr])\n"
>>                  : [tmp] "=r" (tmp_val) : [ptr] "r" (&tmp_var) : "memory");
>> -
>> -       misaligned_emu_detected = (*mas_ptr == RISCV_HWPROBE_MISALIGNED_EMULATED);
>> -       /*
>> -        * If unaligned_ctl is already set, this means that we detected that all
>> -        * CPUS uses emulated misaligned access at boot time. If that changed
>> -        * when hotplugging the new cpu, this is something we don't handle.
>> -        */
>> -       if (unlikely(unaligned_ctl && !misaligned_emu_detected)) {
>> -               pr_crit("CPU misaligned accesses non homogeneous (expected all emulated)\n");
>> -               while (true)
>> -                       cpu_relax();
>> -       }
> 
> This chunk was meant to detect and refuse to run on a system where a
> heterogeneous CPU is hotplugged into a previously homogenous system.
> The commit message doesn't mention this change, how come you
> deleted it?

Sorry for the long wait.
I do not remember why I removed this.
Your right it shouldn't be removed, I added it back.

Thanks,
Jesse Taube
> 
> 
>> -
>> -       return misaligned_emu_detected;
>>   }
>>
>>   bool check_unaligned_access_emulated_all_cpus(void)
>> @@ -562,8 +548,11 @@ bool check_unaligned_access_emulated_all_cpus(void)
>>           * accesses emulated since tasks requesting such control can run on any
>>           * CPU.
>>           */
>> +       schedule_on_each_cpu(check_unaligned_access_emulated);
>> +
>>          for_each_online_cpu(cpu)
>> -               if (!check_unaligned_access_emulated(cpu))
>> +               if (per_cpu(misaligned_access_speed, cpu)
>> +                   != RISCV_HWPROBE_MISALIGNED_EMULATED)
>>                          return false;
>>
>>          unaligned_ctl = true;
>> --
>> 2.45.2
>>
Jesse Taube July 11, 2024, 8:35 p.m. UTC | #17
On 7/1/24 18:51, Evan Green wrote:
> On Mon, Jun 24, 2024 at 5:52 PM Jesse Taube <jesse@rivosinc.com> wrote:
>>
>> Detect if vector misaligned accesses are faster or slower than
>> equivalent vector byte accesses. This is useful for usermode to know
>> whether vector byte accesses or vector misaligned accesses have a better
>> bandwidth for operations like memcpy.
>>
>> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
>> ---
>> V1 -> V2:
>>   - Add Kconfig options
>>   - Add WORD_EEW to vec-copy-unaligned.S
>> V2 -> V3:
>>   - Remove unnecessary comment
>>   - Remove local_irq_enable
>> ---
>>   arch/riscv/Kconfig                         |  18 +++
>>   arch/riscv/kernel/Makefile                 |   3 +-
>>   arch/riscv/kernel/copy-unaligned.h         |   5 +
>>   arch/riscv/kernel/sys_hwprobe.c            |   6 +
>>   arch/riscv/kernel/unaligned_access_speed.c | 132 ++++++++++++++++++++-
>>   arch/riscv/kernel/vec-copy-unaligned.S     |  58 +++++++++
>>   6 files changed, 219 insertions(+), 3 deletions(-)
>>   create mode 100644 arch/riscv/kernel/vec-copy-unaligned.S
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index ffbe0fdd7fb3..6f9fd3748916 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -807,6 +807,24 @@ config RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
>>            will dynamically determine the speed of vector unaligned accesses on
>>            the underlying system if they are supported.
>>
>> +config RISCV_SLOW_VEC_UNALIGNED_ACCESS
>> +       bool "Assume the system supports slow vector unaligned memory accesses"
>> +       depends on NONPORTABLE
>> +       help
>> +         Assume that the system supports slow vector unaligned memory accesses. The
>> +         kernel and userspace programs may not be able to run at all on systems
>> +         that do not support unaligned memory accesses.
>> +
>> +config RISCV_EFFICIENT_VEC_UNALIGNED_ACCESS
>> +       bool "Assume the system supports fast vector unaligned memory accesses"
>> +       depends on NONPORTABLE
>> +       help
>> +         Assume that the system supports fast vector unaligned memory accesses. When
>> +         enabled, this option improves the performance of the kernel on such
>> +         systems. However, the kernel and userspace programs will run much more
>> +         slowly, or will not be able to run at all, on systems that do not
>> +         support efficient unaligned memory accesses.
>> +
>>   endchoice
>>
>>   endmenu # "Platform type"
>> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
>> index 5b243d46f4b1..291935a084d5 100644
>> --- a/arch/riscv/kernel/Makefile
>> +++ b/arch/riscv/kernel/Makefile
>> @@ -64,7 +64,8 @@ obj-$(CONFIG_MMU) += vdso.o vdso/
>>
>>   obj-$(CONFIG_RISCV_MISALIGNED) += traps_misaligned.o
>>   obj-$(CONFIG_RISCV_MISALIGNED) += unaligned_access_speed.o
>> -obj-$(CONFIG_RISCV_PROBE_UNALIGNED_ACCESS)     += copy-unaligned.o
>> +obj-$(CONFIG_RISCV_PROBE_UNALIGNED_ACCESS)             += copy-unaligned.o
>> +obj-$(CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS)      += vec-copy-unaligned.o
>>
>>   obj-$(CONFIG_FPU)              += fpu.o
>>   obj-$(CONFIG_FPU)              += kernel_mode_fpu.o
>> diff --git a/arch/riscv/kernel/copy-unaligned.h b/arch/riscv/kernel/copy-unaligned.h
>> index e3d70d35b708..85d4d11450cb 100644
>> --- a/arch/riscv/kernel/copy-unaligned.h
>> +++ b/arch/riscv/kernel/copy-unaligned.h
>> @@ -10,4 +10,9 @@
>>   void __riscv_copy_words_unaligned(void *dst, const void *src, size_t size);
>>   void __riscv_copy_bytes_unaligned(void *dst, const void *src, size_t size);
>>
>> +#ifdef CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
>> +void __riscv_copy_vec_words_unaligned(void *dst, const void *src, size_t size);
>> +void __riscv_copy_vec_bytes_unaligned(void *dst, const void *src, size_t size);
>> +#endif
>> +
>>   #endif /* __RISCV_KERNEL_COPY_UNALIGNED_H */
>> diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
>> index 5b78ea5a84d1..46de3f145154 100644
>> --- a/arch/riscv/kernel/sys_hwprobe.c
>> +++ b/arch/riscv/kernel/sys_hwprobe.c
>> @@ -221,6 +221,12 @@ static u64 hwprobe_vec_misaligned(const struct cpumask *cpus)
>>   #else
>>   static u64 hwprobe_vec_misaligned(const struct cpumask *cpus)
>>   {
>> +       if (IS_ENABLED(CONFIG_RISCV_EFFICIENT_VEC_UNALIGNED_ACCESS))
>> +               return RISCV_HWPROBE_VEC_MISALIGNED_FAST;
>> +
>> +       if (IS_ENABLED(CONFIG_RISCV_SLOW_VEC_UNALIGNED_ACCESS))
>> +               return RISCV_HWPROBE_VEC_MISALIGNED_SLOW;
>> +
>>          return RISCV_HWPROBE_VEC_MISALIGNED_UNKNOWN;
>>   }
>>   #endif
>> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
>> index 8489e012cf23..a2a8f948500b 100644
>> --- a/arch/riscv/kernel/unaligned_access_speed.c
>> +++ b/arch/riscv/kernel/unaligned_access_speed.c
>> @@ -8,9 +8,11 @@
>>   #include <linux/jump_label.h>
>>   #include <linux/mm.h>
>>   #include <linux/smp.h>
>> +#include <linux/kthread.h>
>>   #include <linux/types.h>
>>   #include <asm/cpufeature.h>
>>   #include <asm/hwprobe.h>
>> +#include <asm/vector.h>
>>
>>   #include "copy-unaligned.h"
>>
>> @@ -267,9 +269,129 @@ static int check_unaligned_access_speed_all_cpus(void)
>>   }
>>   #endif
>>
>> +#ifdef CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
>> +static void check_vector_unaligned_access(struct work_struct *unused)
>> +{
>> +       int cpu = smp_processor_id();
>> +       u64 start_cycles, end_cycles;
>> +       u64 word_cycles;
>> +       u64 byte_cycles;
>> +       int ratio;
>> +       unsigned long start_jiffies, now;
>> +       struct page *page;
>> +       void *dst;
>> +       void *src;
>> +       long speed = RISCV_HWPROBE_VEC_MISALIGNED_SLOW;
>> +
>> +       if (per_cpu(vector_misaligned_access, cpu) != RISCV_HWPROBE_VEC_MISALIGNED_SLOW)
>> +               return;
>> +
>> +       page = alloc_pages(GFP_KERNEL, MISALIGNED_BUFFER_ORDER);
>> +       if (!page) {
>> +               pr_warn("Allocation failure, not measuring vector misaligned performance\n");
>> +               return;
>> +       }
>> +
>> +       /* Make an unaligned destination buffer. */
>> +       dst = (void *)((unsigned long)page_address(page) | 0x1);
>> +       /* Unalign src as well, but differently (off by 1 + 2 = 3). */
>> +       src = dst + (MISALIGNED_BUFFER_SIZE / 2);
>> +       src += 2;
>> +       word_cycles = -1ULL;
>> +
>> +       /* Do a warmup. */
>> +       kernel_vector_begin();
> 
> Should there be a preempt_disable() in here too, or is that implied
> already by schedule_on_each_cpu? Mainly we want to minimize the amount
> of things that might soak up our jiffy and steal iterations from the
> loop.

To test I put a schedule here and it says OOPS we are attomic.

I added preempt_disable and preempt_enable to be sure preemption is 
disabled anyway.

> 
>> +       __riscv_copy_vec_words_unaligned(dst, src, MISALIGNED_COPY_SIZE);
>> +
>> +       start_jiffies = jiffies;
>> +       while ((now = jiffies) == start_jiffies)
>> +               cpu_relax();
>> +
>> +       /*
>> +        * For a fixed amount of time, repeatedly try the function, and take
>> +        * the best time in cycles as the measurement.
>> +        */
>> +       while (time_before(jiffies, now + (1 << MISALIGNED_ACCESS_JIFFIES_LG2))) {
>> +               start_cycles = get_cycles64();
>> +               /* Ensure the CSR read can't reorder WRT to the copy. */
>> +               mb();
>> +               __riscv_copy_vec_words_unaligned(dst, src, MISALIGNED_COPY_SIZE);
>> +               /* Ensure the copy ends before the end time is snapped. */
>> +               mb();
>> +               end_cycles = get_cycles64();
>> +               if ((end_cycles - start_cycles) < word_cycles)
>> +                       word_cycles = end_cycles - start_cycles;
>> +       }
>> +
>> +       byte_cycles = -1ULL;
>> +       __riscv_copy_vec_bytes_unaligned(dst, src, MISALIGNED_COPY_SIZE);
>> +       start_jiffies = jiffies;
>> +       while ((now = jiffies) == start_jiffies)
>> +               cpu_relax();
>> +
>> +       while (time_before(jiffies, now + (1 << MISALIGNED_ACCESS_JIFFIES_LG2))) {
>> +               start_cycles = get_cycles64();
>> +               mb();
>> +               __riscv_copy_vec_bytes_unaligned(dst, src, MISALIGNED_COPY_SIZE);
>> +               mb();
>> +               end_cycles = get_cycles64();
>> +               if ((end_cycles - start_cycles) < byte_cycles)
>> +                       byte_cycles = end_cycles - start_cycles;
>> +       }
>> +
>> +       kernel_vector_end();
>> +
>> +       /* Don't divide by zero. */
>> +       if (!word_cycles || !byte_cycles) {
>> +               pr_warn("cpu%d: rdtime lacks granularity needed to measure unaligned vector access speed\n",
>> +                       cpu);
>> +
>> +               return;
>> +       }
>> +
>> +       if (word_cycles < byte_cycles)
>> +               speed = RISCV_HWPROBE_VEC_MISALIGNED_FAST;
>> +
>> +       ratio = div_u64((byte_cycles * 100), word_cycles);
>> +       pr_info("cpu%d: Ratio of vector byte access time to vector unaligned word access is %d.%02d, unaligned accesses are %s\n",
>> +               cpu,
>> +               ratio / 100,
>> +               ratio % 100,
>> +               (speed ==  RISCV_HWPROBE_VEC_MISALIGNED_FAST) ? "fast" : "slow");
>> +
>> +       per_cpu(vector_misaligned_access, cpu) = speed;
>> +}
>> +
>> +static int riscv_online_cpu_vec(unsigned int cpu)
>> +{
>> +       check_vector_unaligned_access(NULL);
>> +       return 0;
>> +}
>> +
>> +/* Measure unaligned access speed on all CPUs present at boot in parallel. */
>> +static int vec_check_unaligned_access_speed_all_cpus(void *unused)
>> +{
>> +       schedule_on_each_cpu(check_vector_unaligned_access);
>> +
>> +       /*
>> +        * Setup hotplug callbacks for any new CPUs that come online or go
>> +        * offline.
>> +        */
>> +       cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "riscv:online",
>> +                                 riscv_online_cpu_vec, NULL);
>> +
>> +       return 0;
>> +}
>> +#else /* CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS */
>> +static int vec_check_unaligned_access_speed_all_cpus(void *unused)
>> +{
>> +       return 0;
>> +}
>> +#endif
>> +
>>   static int check_unaligned_access_all_cpus(void)
>>   {
>> -       bool all_cpus_emulated;
>> +       bool all_cpus_emulated, all_cpus_vec_supported;
>>          int cpu;
>>
>>          if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
>> @@ -285,7 +407,13 @@ static int check_unaligned_access_all_cpus(void)
>>          }
>>
>>          all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
>> -       check_vector_unaligned_access_emulated_all_cpus();
>> +       all_cpus_vec_supported = check_vector_unaligned_access_emulated_all_cpus();
>> +
>> +       if (all_cpus_vec_supported &&
> 
> Note that with this here, if a heterogeneous system ever shows up with
> a mix of supported/unsupported, the supported cpus will end up with a
> misaligned vector speed of UNKNOWN. It might be nicer to change the
> semantics of that return value from all_cpus_vec_supported (aka
> entirely_supported) to entirely_unsupported. Then you'd only fire up
> the thread if (!entirely_unsupported). If you did that, you'd also
> need to bail early in the check on any CPUs that already had their
> value set to UNSUPPORTED.

Fortunatly check_vector_unaligned_access already checks this.

> That way we can still avoid firing up this
> thread for machines that don't have misaligned V (or V at all), but
> not leave the question UNKNOWN in some cases.

That makes a lot of sence I'm not sure why I didnt think ot that.
I changed it to `all_cpus_vec_unsupported` so it will check the speed if 
atleast one cpu has unaligned vector support.

Thanks,
Jesse taube

> 
>> +           IS_ENABLED(CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS)) {
>> +               kthread_run(vec_check_unaligned_access_speed_all_cpus,
>> +                           NULL, "vec_check_unaligned_access_speed_all_cpus");
>> +       }
>>
>>          if (!all_cpus_emulated)
>>                  return check_unaligned_access_speed_all_cpus();
>> diff --git a/arch/riscv/kernel/vec-copy-unaligned.S b/arch/riscv/kernel/vec-copy-unaligned.S
>> new file mode 100644
>> index 000000000000..e5bc94917e60
>> --- /dev/null
>> +++ b/arch/riscv/kernel/vec-copy-unaligned.S
>> @@ -0,0 +1,58 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2024 Rivos Inc. */
>> +
>> +#include <linux/linkage.h>
>> +#include <asm/asm.h>
>> +#include <linux/args.h>
>> +
>> +       .text
>> +
>> +#define WORD_EEW 32
>> +
>> +#define WORD_SEW CONCATENATE(e, WORD_EEW)
>> +#define VEC_L CONCATENATE(vle, WORD_EEW).v
>> +#define VEC_S CONCATENATE(vle, WORD_EEW).v
>> +
>> +/* void __riscv_copy_vec_words_unaligned(void *, const void *, size_t) */
>> +/* Performs a memcpy without aligning buffers, using word loads and stores. */
>> +/* Note: The size is truncated to a multiple of WORD_EEW */
>> +SYM_FUNC_START(__riscv_copy_vec_words_unaligned)
>> +       andi  a4, a2, ~(WORD_EEW-1)
>> +       beqz  a4, 2f
>> +       add   a3, a1, a4
>> +       .option push
>> +       .option arch, +zve32x
>> +1:
>> +       vsetivli t0, 8, WORD_SEW, m8, ta, ma
>> +       VEC_L v0, (a1)
>> +       VEC_S v0, (a0)
>> +       addi  a0, a0, WORD_EEW
>> +       addi  a1, a1, WORD_EEW
>> +       bltu  a1, a3, 1b
>> +
>> +2:
>> +       .option pop
>> +       ret
>> +SYM_FUNC_END(__riscv_copy_vec_words_unaligned)
>> +
>> +/* void __riscv_copy_vec_bytes_unaligned(void *, const void *, size_t) */
>> +/* Performs a memcpy without aligning buffers, using only byte accesses. */
>> +/* Note: The size is truncated to a multiple of 8 */
>> +SYM_FUNC_START(__riscv_copy_vec_bytes_unaligned)
>> +       andi a4, a2, ~(8-1)
>> +       beqz a4, 2f
>> +       add  a3, a1, a4
>> +       .option push
>> +       .option arch, +zve32x
>> +1:
>> +       vsetivli t0, 8, e8, m8, ta, ma
>> +       vle8.v v0, (a1)
>> +       vse8.v v0, (a0)
>> +       addi a0, a0, 8
>> +       addi a1, a1, 8
>> +       bltu a1, a3, 1b
>> +
>> +2:
>> +       .option pop
>> +       ret
>> +SYM_FUNC_END(__riscv_copy_vec_bytes_unaligned)
>> --
>> 2.45.2
>>