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

From: Michael Ellerman <mpe@ellerman.id.au>
To: Dan Williams <dan.j.williams@intel.com>,
	tglx@linutronix.de, mingo@redhat.com
Cc: x86@kernel.org, stable@vger.kernel.org,
	Borislav Petkov <bp@alien8.de>, Tony Luck <tony.luck@intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Paul Mackerras <paulus@samba.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Mikulas Patocka <mpatocka@redhat.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org
Subject: Re: [PATCH v3 1/2] x86, powerpc: Rename memcpy_mcsafe() to copy_mc_to_{user, kernel}()
Date: Wed, 20 May 2020 19:53:05 +1000	[thread overview]
Message-ID: <87d06z7x1a.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <158992635697.403910.6957168747147028694.stgit@dwillia2-desk3.amr.corp.intel.com>

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.

> +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.

...

> 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 \

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.

cheers


diff --git a/tools/testing/selftests/powerpc/copyloops/copy_mc.S b/tools/testing/selftests/powerpc/copyloops/copy_mc.S
new file mode 120000
index 000000000000..dcbe06d500fb
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/copy_mc.S
@@ -0,0 +1 @@
+../../../../../arch/powerpc/lib/copy_mc_64.S
\ No newline at end of file
diff --git a/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S b/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S
deleted file mode 120000
index f0feef3062f6..000000000000
--- a/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S
+++ /dev/null
@@ -1 +0,0 @@
-../../../../../arch/powerpc/lib/memcpy_mcsafe_64.S
\ No newline at end of file
-- 
2.25.1

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
