Re: [PATCH v3 1/2] x86, powerpc: Rename memcpy_mcsafe() to copy_mc_to_{user, kernel}()

[Dan Williams]

On Wed, May 20, 2020 at 2:54 AM Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
>
> Hi Dan,
>
> Just a couple of minor things ...
>
> Dan Williams <dan.j.williams@xxxxxxxxx> writes:
> > In reaction to a proposal to introduce a memcpy_mcsafe_fast()
> > implementation Linus points out that memcpy_mcsafe() is poorly named
> > relative to communicating the scope of the interface. Specifically what
> > addresses are valid to pass as source, destination, and what faults /
> > exceptions are handled. Of particular concern is that even though x86
> > might be able to handle the semantics of copy_mc_to_user() with its
> > common copy_user_generic() implementation other archs likely need / want
> > an explicit path for this case:
> ...
>
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 0969285996cb..dcbbcbf3552c 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -348,6 +348,32 @@ do {                                                             \
> >  extern unsigned long __copy_tofrom_user(void __user *to,
> >               const void __user *from, unsigned long size);
> >
> > +#ifdef CONFIG_ARCH_HAS_COPY_MC
> > +extern unsigned long __must_check
>
> We try not to add extern in headers anymore.

Ok, I was doing the copy-pasta dance, but I'll remove this.

>
> > +copy_mc_generic(void *to, const void *from, unsigned long size);
> > +
> > +static inline unsigned long __must_check
> > +copy_mc_to_kernel(void *to, const void *from, unsigned long size)
> > +{
> > +     return copy_mc_generic(to, from, size);
> > +}
> > +#define copy_mc_to_kernel copy_mc_to_kernel
> > +
> > +static inline unsigned long __must_check
> > +copy_mc_to_user(void __user *to, const void *from, unsigned long n)
> > +{
> > +     if (likely(check_copy_size(from, n, true))) {
> > +             if (access_ok(to, n)) {
> > +                     allow_write_to_user(to, n);
> > +                     n = copy_mc_generic((void *)to, from, n);
> > +                     prevent_write_to_user(to, n);
> > +             }
> > +     }
> > +
> > +     return n;
> > +}
> > +#endif
>
> Otherwise that looks fine.

Cool.

>
> ...
>
> > diff --git a/tools/testing/selftests/powerpc/copyloops/Makefile b/tools/testing/selftests/powerpc/copyloops/Makefile
> > index 0917983a1c78..959817e7567c 100644
> > --- a/tools/testing/selftests/powerpc/copyloops/Makefile
> > +++ b/tools/testing/selftests/powerpc/copyloops/Makefile
> > @@ -45,9 +45,9 @@ $(OUTPUT)/memcpy_p7_t%:     memcpy_power7.S $(EXTRA_SOURCES)
> >               -D SELFTEST_CASE=$(subst memcpy_p7_t,,$(notdir $@)) \
> >               -o $@ $^
> >
> > -$(OUTPUT)/memcpy_mcsafe_64: memcpy_mcsafe_64.S $(EXTRA_SOURCES)
> > +$(OUTPUT)/copy_mc: copy_mc.S $(EXTRA_SOURCES)
> >       $(CC) $(CPPFLAGS) $(CFLAGS) \
> > -             -D COPY_LOOP=test_memcpy_mcsafe \
> > +             -D COPY_LOOP=test_copy_mc \

Ok.

>
> This needs a fixup:
>
> diff --git a/tools/testing/selftests/powerpc/copyloops/Makefile b/tools/testing/selftests/powerpc/copyloops/Makefile
> index 959817e7567c..b4eb5c4c6858 100644
> --- a/tools/testing/selftests/powerpc/copyloops/Makefile
> +++ b/tools/testing/selftests/powerpc/copyloops/Makefile
> @@ -47,7 +47,7 @@ $(OUTPUT)/memcpy_p7_t%:       memcpy_power7.S $(EXTRA_SOURCES)
>
>  $(OUTPUT)/copy_mc: copy_mc.S $(EXTRA_SOURCES)
>         $(CC) $(CPPFLAGS) $(CFLAGS) \
> -               -D COPY_LOOP=test_copy_mc \
> +               -D COPY_LOOP=test_copy_mc_generic \
>                 -o $@ $^
>
>  $(OUTPUT)/copyuser_64_exc_t%: copyuser_64.S exc_validate.c ../harness.c \
>
>
> >               -o $@ $^
> >
> >  $(OUTPUT)/copyuser_64_exc_t%: copyuser_64.S exc_validate.c ../harness.c \
> > diff --git a/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S b/tools/testing/selftests/powerpc/copyloops/copy_mc.S
> > similarity index 100%
> > rename from tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S
> > rename to tools/testing/selftests/powerpc/copyloops/copy_mc.S
>
> This file is a symlink to the file in arch/powerpc/lib, so the name of
> the link needs updating, as well as the target.
>
> Also is there a reason you dropped the "_64"? It would make most sense
> to keep it I think, as then the file in selftests and the file in arch/
> have the same name.
>
> If you want to keep the copy_mc.S name it needs the diff below. Though
> as I said I think it would be better to use copy_mc_64.S.

copy_mc_64.S looks good to me.

Thanks Michael!