Re: [PATCH 1/2] riscv: Add memmove string operation.

[Nick Hu]

On Wed, Aug 14, 2019 at 10:22:15AM +0800, Paul Walmsley wrote:
> On Tue, 13 Aug 2019, Palmer Dabbelt wrote:
> 
> > On Mon, 12 Aug 2019 08:04:46 PDT (-0700), Christoph Hellwig wrote:
> > > On Wed, Aug 07, 2019 at 03:19:14PM +0800, Nick Hu wrote:
> > > > There are some features which need this string operation for compilation,
> > > > like KASAN. So the purpose of this porting is for the features like KASAN
> > > > which cannot be compiled without it.
> > > > 
> > > > KASAN's string operations would replace the original string operations and
> > > > call for the architecture defined string operations. Since we don't have
> > > > this in current kernel, this patch provides the implementation.
> > > > 
> > > > This porting refers to the 'arch/nds32/lib/memmove.S'.
> > > 
> > > This looks sensible to me, although my stringop asm is rather rusty,
> > > so just an ack and not a real review-by:
> > > 
> > > Acked-by: Christoph Hellwig <hch@xxxxxx>
> > 
> > FWIW, we just write this in C everywhere else and rely on the compiler to
> > unroll the loops.  I always prefer C to assembly when possible, so I'd prefer
> > if we just adopt the string code from newlib.  We have a RISC-V-specific
> > memcpy in there, but just use the generic memmove.
> > 
> > Maybe the best bet here would be to adopt the newlib memcpy/memmove as generic
> > Linux functions?  They're both in C so they should be fine, and they both look
> > faster than what's in lib/string.c.  Then everyone would benefit and we don't
> > need this tricky RISC-V assembly.  Also, from the look of it the newlib code
> > is faster because the inner loop is unrolled.
> 
> There's a generic memmove implementation in the kernel already:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/string.h#n362
> 
> Nick, could you tell us more about why the generic memmove() isn't 
> suitable?
> 
> 
> - Paul

Hi Paul,

KASAN has its own string operations(memcpy/memmove/memset) because it needs to
hook some code to check memory region. It would undefined the original string
operations and called the string operations with the prefix '__'. But the
generic string operations didn't declare with the prefix. Other archs with
KASAN support like arm64 and xtensa all have their own string operations and
defined with the prefix.

Nick