1. Patchew
  2. linux
  3. [PATCH v2 0/4] riscv: uaccess: optimizations
  4. View patch

[PATCH v2 2/4] riscv: uaccess: use input constraints for ptr of __put_user

Cyril Bur posted 4 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH v2 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Posted by Cyril Bur 1 year, 2 months ago 
From: Jisheng Zhang <jszhang@kernel.org>

I believe the output constraints "=m" is not necessary, because
the instruction itself is "write", we don't need the compiler
to "write" for us. So tell compiler we read from memory instead
of writing.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
---
 arch/riscv/include/asm/uaccess.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 09d4ca37522c..84b084e388a7 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -186,11 +186,11 @@ do {								\
 	__typeof__(*(ptr)) __x = x;				\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
-		"	" insn " %z2, %1\n"			\
+		"	" insn " %z1, %2\n"			\
 		"2:\n"						\
 		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
-		: "+r" (err), "=m" (*(ptr))			\
-		: "rJ" (__x));					\
+		: "+r" (err)			\
+		: "rJ" (__x), "m"(*(ptr)));					\
 } while (0)
 
 #ifdef CONFIG_64BIT
@@ -203,16 +203,16 @@ do {								\
 	u64 __x = (__typeof__((x)-(x)))(x);			\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
-		"	sw %z3, %1\n"				\
+		"	sw %z1, %3\n"				\
 		"2:\n"						\
-		"	sw %z4, %2\n"				\
+		"	sw %z2, %4\n"				\
 		"3:\n"						\
 		_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0)		\
 		_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0)		\
-		: "+r" (err),					\
-			"=m" (__ptr[__LSW]),			\
-			"=m" (__ptr[__MSW])			\
-		: "rJ" (__x), "rJ" (__x >> 32));		\
+		: "+r" (err)					\
+		: "rJ" (__x), "rJ" (__x >> 32),			\
+			"m" (__ptr[__LSW]),			\
+			"m" (__ptr[__MSW]));			\
 } while (0)
 #endif /* CONFIG_64BIT */
 
-- 
2.34.1
Re: [PATCH v2 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Posted by Jessica Clarke 1 year ago 
On 18 Nov 2024, at 23:01, Cyril Bur <cyrilbur@tenstorrent.com> wrote:
> 
> From: Jisheng Zhang <jszhang@kernel.org>
> 
> I believe the output constraints "=m" is not necessary, because
> the instruction itself is "write", we don't need the compiler
> to "write" for us. So tell compiler we read from memory instead
> of writing.

That’s not what =m does. =m tells the compiler “this assembly will be
writing to this memory location, and needs the address of the location
for that operand”. If you move it from an output to an input then you
get the same assembly generated, but the compiler believes you are
*reading* from that memory, not *writing* to it, and so does not
believe the memory may be clobbered.

Now, it may well be that, by virtue of this being userspace memory that
is only ever accessed via assembly, and any inline assembly being
marked volatile, this in effect doesn’t matter. But it is still
technically wrong to model it that way, and you cannot use the
justification you are using, because it is false, and demonstrates a
lack of understanding for how inline assembly works. It has to be
correctly justified.

(I do note that neither x86 nor arm64 seems to model this as an output)

Jess

> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
> ---
> arch/riscv/include/asm/uaccess.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 09d4ca37522c..84b084e388a7 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -186,11 +186,11 @@ do { \
> __typeof__(*(ptr)) __x = x; \
> __asm__ __volatile__ ( \
> "1:\n" \
> - " " insn " %z2, %1\n" \
> + " " insn " %z1, %2\n" \
> "2:\n" \
> _ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0) \
> - : "+r" (err), "=m" (*(ptr)) \
> - : "rJ" (__x)); \
> + : "+r" (err) \
> + : "rJ" (__x), "m"(*(ptr))); \
> } while (0)
> 
> #ifdef CONFIG_64BIT
> @@ -203,16 +203,16 @@ do { \
> u64 __x = (__typeof__((x)-(x)))(x); \
> __asm__ __volatile__ ( \
> "1:\n" \
> - " sw %z3, %1\n" \
> + " sw %z1, %3\n" \
> "2:\n" \
> - " sw %z4, %2\n" \
> + " sw %z2, %4\n" \
> "3:\n" \
> _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0) \
> _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0) \
> - : "+r" (err), \
> - "=m" (__ptr[__LSW]), \
> - "=m" (__ptr[__MSW]) \
> - : "rJ" (__x), "rJ" (__x >> 32)); \
> + : "+r" (err) \
> + : "rJ" (__x), "rJ" (__x >> 32), \
> + "m" (__ptr[__LSW]), \
> + "m" (__ptr[__MSW])); \
> } while (0)
> #endif /* CONFIG_64BIT */
> 
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v2 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Posted by Cyril Bur 1 year ago 

On 18/1/2025 9:34 am, Jessica Clarke wrote:
> On 18 Nov 2024, at 23:01, Cyril Bur <cyrilbur@tenstorrent.com> wrote:
>>
>> From: Jisheng Zhang <jszhang@kernel.org>
>>
>> I believe the output constraints "=m" is not necessary, because
>> the instruction itself is "write", we don't need the compiler
>> to "write" for us. So tell compiler we read from memory instead
>> of writing.
> 
> That’s not what =m does. =m tells the compiler “this assembly will be
> writing to this memory location, and needs the address of the location
> for that operand”. If you move it from an output to an input then you
> get the same assembly generated, but the compiler believes you are
> *reading* from that memory, not *writing* to it, and so does not
> believe the memory may be clobbered.
> 
> Now, it may well be that, by virtue of this being userspace memory that
> is only ever accessed via assembly, and any inline assembly being
> marked volatile, this in effect doesn’t matter. But it is still
> technically wrong to model it that way, and you cannot use the
> justification you are using, because it is false, and demonstrates a
> lack of understanding for how inline assembly works. It has to be
> correctly justified.
> 
> (I do note that neither x86 nor arm64 seems to model this as an output)
> 
> Jess

Good points, I agree. I am happy to respin this patch move the =m to 
output constraint.

@Charlie Please let me know if you would like me to keep your tags on 
the tweaked version.

Thanks,

Cyril
> 
>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>> Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
>> ---
>> arch/riscv/include/asm/uaccess.h | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
>> index 09d4ca37522c..84b084e388a7 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -186,11 +186,11 @@ do { \
>> __typeof__(*(ptr)) __x = x; \
>> __asm__ __volatile__ ( \
>> "1:\n" \
>> - " " insn " %z2, %1\n" \
>> + " " insn " %z1, %2\n" \
>> "2:\n" \
>> _ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0) \
>> - : "+r" (err), "=m" (*(ptr)) \
>> - : "rJ" (__x)); \
>> + : "+r" (err) \
>> + : "rJ" (__x), "m"(*(ptr))); \
>> } while (0)
>>
>> #ifdef CONFIG_64BIT
>> @@ -203,16 +203,16 @@ do { \
>> u64 __x = (__typeof__((x)-(x)))(x); \
>> __asm__ __volatile__ ( \
>> "1:\n" \
>> - " sw %z3, %1\n" \
>> + " sw %z1, %3\n" \
>> "2:\n" \
>> - " sw %z4, %2\n" \
>> + " sw %z2, %4\n" \
>> "3:\n" \
>> _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0) \
>> _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0) \
>> - : "+r" (err), \
>> - "=m" (__ptr[__LSW]), \
>> - "=m" (__ptr[__MSW]) \
>> - : "rJ" (__x), "rJ" (__x >> 32)); \
>> + : "+r" (err) \
>> + : "rJ" (__x), "rJ" (__x >> 32), \
>> + "m" (__ptr[__LSW]), \
>> + "m" (__ptr[__MSW])); \
>> } while (0)
>> #endif /* CONFIG_64BIT */
>>
>> -- 
>> 2.34.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
Re: [PATCH v2 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Posted by Charlie Jenkins 1 year ago 
On Fri, Jan 31, 2025 at 12:31:38PM +1100, Cyril Bur wrote:
> 
> 
> On 18/1/2025 9:34 am, Jessica Clarke wrote:
> > On 18 Nov 2024, at 23:01, Cyril Bur <cyrilbur@tenstorrent.com> wrote:
> > > 
> > > From: Jisheng Zhang <jszhang@kernel.org>
> > > 
> > > I believe the output constraints "=m" is not necessary, because
> > > the instruction itself is "write", we don't need the compiler
> > > to "write" for us. So tell compiler we read from memory instead
> > > of writing.
> > 
> > That’s not what =m does. =m tells the compiler “this assembly will be
> > writing to this memory location, and needs the address of the location
> > for that operand”. If you move it from an output to an input then you
> > get the same assembly generated, but the compiler believes you are
> > *reading* from that memory, not *writing* to it, and so does not
> > believe the memory may be clobbered.
> > 
> > Now, it may well be that, by virtue of this being userspace memory that
> > is only ever accessed via assembly, and any inline assembly being
> > marked volatile, this in effect doesn’t matter. But it is still
> > technically wrong to model it that way, and you cannot use the
> > justification you are using, because it is false, and demonstrates a
> > lack of understanding for how inline assembly works. It has to be
> > correctly justified.
> > 
> > (I do note that neither x86 nor arm64 seems to model this as an output)
> > 
> > Jess
> 
> Good points, I agree. I am happy to respin this patch
> move the =m to output constraint.
> 
> @Charlie Please let me know if you would like me to
> keep your tags on the tweaked version.

Can you remove the tags? Copy me on the next version and I will review
it again.

- Charlie

> 
> Thanks,
> 
> Cyril
> > 
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
> > > ---
> > > arch/riscv/include/asm/uaccess.h | 18 +++++++++---------
> > > 1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> > > index 09d4ca37522c..84b084e388a7 100644
> > > --- a/arch/riscv/include/asm/uaccess.h
> > > +++ b/arch/riscv/include/asm/uaccess.h
> > > @@ -186,11 +186,11 @@ do { \
> > > __typeof__(*(ptr)) __x = x; \
> > > __asm__ __volatile__ ( \
> > > "1:\n" \
> > > - " " insn " %z2, %1\n" \
> > > + " " insn " %z1, %2\n" \
> > > "2:\n" \
> > > _ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0) \
> > > - : "+r" (err), "=m" (*(ptr)) \
> > > - : "rJ" (__x)); \
> > > + : "+r" (err) \
> > > + : "rJ" (__x), "m"(*(ptr))); \
> > > } while (0)
> > > 
> > > #ifdef CONFIG_64BIT
> > > @@ -203,16 +203,16 @@ do { \
> > > u64 __x = (__typeof__((x)-(x)))(x); \
> > > __asm__ __volatile__ ( \
> > > "1:\n" \
> > > - " sw %z3, %1\n" \
> > > + " sw %z1, %3\n" \
> > > "2:\n" \
> > > - " sw %z4, %2\n" \
> > > + " sw %z2, %4\n" \
> > > "3:\n" \
> > > _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0) \
> > > _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0) \
> > > - : "+r" (err), \
> > > - "=m" (__ptr[__LSW]), \
> > > - "=m" (__ptr[__MSW]) \
> > > - : "rJ" (__x), "rJ" (__x >> 32)); \
> > > + : "+r" (err) \
> > > + : "rJ" (__x), "rJ" (__x >> 32), \
> > > + "m" (__ptr[__LSW]), \
> > > + "m" (__ptr[__MSW])); \
> > > } while (0)
> > > #endif /* CONFIG_64BIT */
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > 
>
Re: [PATCH v2 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Posted by Charlie Jenkins 1 year ago 
On Mon, Nov 18, 2024 at 11:01:10PM +0000, Cyril Bur wrote:
> From: Jisheng Zhang <jszhang@kernel.org>
> 
> I believe the output constraints "=m" is not necessary, because
> the instruction itself is "write", we don't need the compiler
> to "write" for us. So tell compiler we read from memory instead
> of writing.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.