[v5, 5/6] riscv: lib: vectorize copy_to_user/copy_from_user

[Andy Chiu]

On Fri, Dec 15, 2023 at 9:52 PM Andrew Jones <ajones at ventanamicro.com> wrote:
>
> On Thu, Dec 14, 2023 at 10:25:49PM -0800, Charlie Jenkins wrote:
> > On Thu, Dec 14, 2023 at 03:57:20PM +0000, Andy Chiu wrote:
> ...
> > >  SYM_FUNC_START(__asm_copy_to_user)
> > > +#ifdef CONFIG_RISCV_ISA_V
> > > +   ALTERNATIVE("j fallback_scalar_usercopy", "nop", 0, RISCV_ISA_EXT_v, CONFIG_RISCV_ISA_V)
> >
> > has_vector uses riscv_has_extension_unlikely, but this is the equivalent
> > of riscv_has_extension_likely. It seems like this should be consistent
> > across all call sites. Since has_vector uses the unlikely version, this
> > should probably be rearranged so that the nop is in the non-vector
> > version and the jump is for the vector version.
>
> I think I prefer it the way it is, where the optimized path is fully
> optimized and the fallback path also suffers the jump. (I've also
> taken that approach for clear_page()). Also, as extensions are adopted
> by more an more platforms, and we start to consider switching unlikelys
> to likelys, then it would be easy to miss stuff like this.
>
> >
> > A neat optimization you can do here is replace the "nop" with the
> > instruction that will be executed first. With how it's written right now
> > you could replace the nop with the la instruction. It's just a nop so
> > the performance difference is probably not going to be noticable but
> > it's theoretically better without the nop. The downside of doing this is
>
> I think I prefer the nop, because it's easier to read and maintain the
> assembly function when the ALTERNATIVE doesn't do anything other than
> choose the entry point.

Good point. I would prefer this approach as well. Loading from a
symbol can take 2 instructions, so we will have to insert a nop
padding for the default path. Though the nop will never execute, it
will make assembly code a bit harder to read. Maybe we could leave it
for future optimization.

>
> > that it seems like alternatives do not work with macros so you couldn't
> > replace the nop with a REG_L instruction, unless there is some trick to
> > make it work.
>
> One should be able to use REG_L in an alternative since macro expansion
> will result in the string "ld" or "lw", which can then be concatenated
> with its parameters, e.g.
>
>   ALTERNATIVE(REG_L " a1, 0(a2)", "nop", 0, 0, 0)
>
> (But note the space before the a1. Without it, we'd get "lda1,")
>

Umm, perhaps I am using an older toolchain. it reports:

arch/riscv/lib/uaccess.S:17: Error: too many positional arguments

on binutil 2.38

> Thanks,
> drew

Thanks,
Andy