Message-ID: <8c1167fb-9580-45bd-a798-f7d32477f1bd@rivosinc.com>
Date: Fri, 21 Jun 2024 14:07:58 -0400
From: Jesse Taube <jesse@...osinc.com>
To: Charlie Jenkins <charlie@...osinc.com>,
 Conor Dooley <conor.dooley@...rochip.com>
Cc: linux-riscv@...ts.infradead.org, Jonathan Corbet <corbet@....net>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Conor Dooley <conor@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Clément Léger
 <cleger@...osinc.com>, Evan Green <evan@...osinc.com>,
 Andrew Jones <ajones@...tanamicro.com>, Xiao Wang <xiao.w.wang@...el.com>,
 Andy Chiu <andy.chiu@...ive.com>, Eric Biggers <ebiggers@...gle.com>,
 Greentime Hu <greentime.hu@...ive.com>, Björn Töpel
 <bjorn@...osinc.com>, Heiko Stuebner <heiko@...ech.de>,
 Costa Shulyupin <costa.shul@...hat.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Baoquan He <bhe@...hat.com>,
 Anup Patel <apatel@...tanamicro.com>, Zong Li <zong.li@...ive.com>,
 Sami Tolvanen <samitolvanen@...gle.com>,
 Ben Dooks <ben.dooks@...ethink.co.uk>,
 Alexandre Ghiti <alexghiti@...osinc.com>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Erick Archer <erick.archer@....com>, Joel Granados <j.granados@...sung.com>,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v2 4/6] RISC-V: Detect unaligned vector accesses
 supported.



On 6/21/24 13:18, Charlie Jenkins wrote:
> On Fri, Jun 21, 2024 at 11:06:31AM +0100, Conor Dooley wrote:
>> On Thu, Jun 20, 2024 at 03:14:14PM -0700, Charlie Jenkins wrote:
>>> On Thu, Jun 20, 2024 at 05:31:28PM -0400, Jesse Taube wrote:
>>>>
>>>>
>>>> On 6/17/24 22:09, Charlie Jenkins wrote:
>>>>> On Mon, Jun 17, 2024 at 06:43:32PM -0700, Charlie Jenkins wrote:
>>>>>> On Thu, Jun 13, 2024 at 03:16:13PM -0400, 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@...osinc.com>
>>>>>>> ---
>>>>>>> V1 -> V2:
>>>>>>>    - 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
>>>>>>> ---
>>>>>>>    arch/riscv/Kconfig                         |  41 +++++++
>>>>>>>    arch/riscv/include/asm/cpufeature.h        |   7 +-
>>>>>>>    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            |  41 +++++++
>>>>>>>    arch/riscv/kernel/traps_misaligned.c       | 119 ++++++++++++++++++++-
>>>>>>>    arch/riscv/kernel/unaligned_access_speed.c |   9 +-
>>>>>>>    arch/riscv/kernel/vector.c                 |   2 +-
>>>>>>>    11 files changed, 221 insertions(+), 21 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>>>>>> index b94176e25be1..f12df0ca6c18 100644
>>>>>>> --- a/arch/riscv/Kconfig
>>>>>>> +++ b/arch/riscv/Kconfig
>>>>>>> @@ -723,6 +723,12 @@ config RISCV_MISALIGNED
>>>>>>>    	help
>>>>>>>    	  Embed support for emulating misaligned loads and stores.
>>>>>>> +config RISCV_VECTOR_MISALIGNED
>>>>>>> +	bool
>>>>>>> +	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 +780,41 @@ 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.
>>
>> I haven't actually checked the patchset, but is it actually used by the
>> kernel to perform optimisations?
> 
> No ;)
> 
> Right now this patch is just providing the information to userspace
> through hwprobe and doesn't optimize anything in the kernel.
> 
>>
>>>>>>> It is also
>>>>>>> +	  exposed to user space via the hwprobe syscall. The hardware will be
>>>>>>> +	  probed at boot by default.
>>>>>>> +
>>>>>>> +config RISCV_DETECT_VECTOR_UNALIGNED_ACCESS
>>>>>>
>>>>>> This is not used anywhere, what is the reason for including it?
>>>>
>>>> This is so that we can check if they are supported or not, but not check the
>>>> speed of them. Similar to RISCV_EMULATED_UNALIGNED_ACCESS.
>>>
>>> What do you mean? It isn't used anywhere so this "check if they are
>>> supported or not" is not guarded by this config.
>>>
>>>>
>>>>>>
>>>>>>> +	bool "Detect support for vector unaligned accesses"
>>>>>>> +	select RISCV_VECTOR_MISALIGNED
>>>>>>> +	help
>>>>>>> +	  During boot, the kernel will detect if the system supports vector
>>>>>>> +	  unaligned accesses.
>>>>>>> +
>>>>>>> +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.
>>>>>>> +
>>>>>>> +config CONFIG_RISCV_UNALIGNED_ACCESS_UNSUPPORTED
>>>>>>
>>>>>> This should not be prefixed with CONFIG and does not include VECTOR in
>>>>>> the name.
>>>>
>>>> Huh thought it would warn fixed though
>>>
>>> What do you mean by "warn fixed"?
>>>
>>>>
>>>>> I assume you meant to put
>>>>>> "RISCV_VEC_UNALIGNED_ACCESS_UNSUPPORTED" here?
>>>>
>>>> This is to leave a faster path like SLOW or FAST to say that unaligned
>>>> access arent suported.
>>>
>>> I am not sure what you are responding to. This comment seems to be
>>> responding to my correction of
>>> CONFIG_RISCV_UNALIGNED_ACCESS_UNSUPPORTED->RISCV_VEC_UNALIGNED_ACCESS_UNSUPPORTED
>>> so I don't see how that ties into SLOW/FAST.
>>>
>>>>
>>>>>>
>>>>>> This was also intentionally left out on the scalar side [1]. The
>>>>>> implication here is that having this config will cause people to compile
>>>>>> kernels without unaligned access support which really shouldn't be
>>>>>> something we are explicitly supporting.
>>>>>>
>>>>>> If somebody does want to support hardware that does not handle vector
>>>>>> unaligned accesses, the solution should be to add emulation support to
>>>>>> the kernel.
>>>>
>>>> Yes but we dont have emulation support yet so I do think its a good idea.
>>>
>>> I am hesitant because it is very likely that somebody will add support
>>> for unaligned vector emulation. When there is emulation support, this
>>> config option should not exist to be consistent with scalar. However if
>>> we add this option in now, we must expect a user to enable this config,
>>> and then
>>
>> I dunno, I think there could be value in having the option here. For
>> scalar, we couldn't have an option that would break the uABI, so the
>> unsupported option wasn't okay. That's not a constraint that we have for
>> vector.
>>
>> For vector, if you have a system that doesn't support misaligned access,
>> you probably don't want to emulate the accesses either, since that's
>> likely remove any performance gains you get from using vector in the
>> first place, so I can see benefit in the option.
> 
> We have the RISCV_MISALIGNED option that disables the scalar emulation,
> but doesn't break the UABI because it's not saying that misaligned
> scalar is not supported, but just that the kernel doesn't emulate.
> Having an UNSUPPORTED option explicitly breaks the UABI which doesn't
> seem like something that the kernel should support. If we are okay with
> having options that break the UABI then this is fine, but I was under
> the impression that we did our best to avoid that.
> 
> There definitely is value in having an option like this for hardware
> that never wants to run misaligned code, but since we decided with the
> scalar accesses that we should not break the UABI I think vector should
> do the same.

The rational for scalar accesses was slightly different as
The base ISA spec said/says: ". The base ISA supports misaligned 
accesses, but these might run extremely slowly depending on the 
implementation."

Section: 2.6 Load and Store Instructions. Available: 
https://riscv.org/wp-content/uploads/2017/05/riscv-spec-v2.2.pdf

> 
>> Enabling the probing is going to end up with same outcome for userspace
>> as having this option on such a system, so it comes down to whether you
>> want to allow people to skip the probing if they know their system has
>> this problem.
>>
>>> we will have to get rid of it later. Users are not always happy
>>> when config options are removed.
>>
>> I dunno, I don't think that adding emulation requires that we remove
>> this unsupported option.
> 
> I am probably being too paranoid about this but I am hesistant to cause
> vector and scalar misaligned access implemenations to diverge. It is
> inconsistent to allow an unsupported option for vector but not for
> scalar when both support emulation. The probing is very fast as it just
> checks if a misaligned access causes a trap and then sets the access
> speed to unsupported if it does trap.

Charlie is right about probing for support being fast. There is
RISCV_DETECT_VECTOR_UNALIGNED_ACCESS to only detect support not the 
speed. I thought it might be a good idea to add this config option, but
I'll just drop it to shorten this thread.

> 
>>
>> Additionally, what are we doing in the kernel if we detect that
>> misaligned stuff isn't supported? Are we going to mandate that kernel
>> code is aligned only

As of now yes.

>> disable in-kernel vector or some other mechanism
>> to make sure that things like crypto code don't have/introduce code
>> that'll not run on these systems?

I'm not too familiar with the uses of unaligned vector accesses, but if
it significantly improves performance then I think there should be a 
faster unaligned access pathway, and a aligned access pathway, so both 
are supported. This also ties into your first question.

Thanks,
Jesse Taube

> UNSUPPORTED will still be set by the quick probe so it would be possible
> for the kernel/userspace to avoid running misaligned vector when it's
> unsupported. Any kernel methods would probably want to always run
> aligned vector unless misaligned support was determined to be FAST
> anyway, I am doubtful that code will have different optimizations for
> FAST, SLOW, and UNSUPPORTED but it is possible.
> 
> I would prefer consistency between scalar and vector misaligned support,
> but this is not a deal breaker for this patch. I am not convinced it is
> the best choice, but I am okay with leaving this option in the kernel.
> 
> - Charlie
> 
>>
>> Cheers,
>> Conor.
> 
> 

Powered by blists - more mailing lists