On Wed, May 20, 2020 at 2:54 AM Michael Ellerman <m...@ellerman.id.au> wrote: > > Hi Dan, > > Just a couple of minor things ... > > Dan Williams <dan.j.willi...@intel.com> 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!