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