[PATCH 1/4] x86/asm: inline constant inputs in rdpkru(), wrpkru()

[PATCH 1/4] x86/asm: inline constant inputs in rdpkru(), wrpkru()

[Alexey Dobriyan]

Put immediate values directly into registers deleting dummy variables.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 arch/x86/include/asm/special_insns.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 21ce480658b1..494a1aa19f05 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -79,7 +79,6 @@ void native_write_cr4(unsigned long val);
 #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
 static inline u32 rdpkru(void)
 {
-	u32 ecx = 0;
 	u32 edx, pkru;
 
 	/*
@@ -88,20 +87,18 @@ static inline u32 rdpkru(void)
 	 */
 	asm volatile(".byte 0x0f,0x01,0xee\n\t"
 		     : "=a" (pkru), "=d" (edx)
-		     : "c" (ecx));
+		     : "c" (0));
 	return pkru;
 }
 
 static inline void wrpkru(u32 pkru)
 {
-	u32 ecx = 0, edx = 0;
-
 	/*
 	 * "wrpkru" instruction.  Loads contents in EAX to PKRU,
 	 * requires that ecx = edx = 0.
 	 */
 	asm volatile(".byte 0x0f,0x01,0xef\n\t"
-		     : : "a" (pkru), "c"(ecx), "d"(edx));
+		     : : "a" (pkru), "c" (0), "d" (0));
 }
 
 #else
-- 
2.45.3

[PATCH 2/4] x86/asm: delete dummy variable in clwb()

[Alexey Dobriyan]

Cast to pointer-to-array instead.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 arch/x86/include/asm/special_insns.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 494a1aa19f05..d349aa0f0a83 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -179,17 +179,15 @@ static inline void clflushopt(volatile void *__p)
 		       "+m" (*(volatile char __force *)__p));
 }
 
-static inline void clwb(volatile void *__p)
+static inline void clwb(volatile void *p)
 {
-	volatile struct { char x[64]; } *p = __p;
-
 	asm volatile(ALTERNATIVE_2(
 		".byte 0x3e; clflush (%[pax])",
 		".byte 0x66; clflush (%[pax])", /* clflushopt (%%rax) */
 		X86_FEATURE_CLFLUSHOPT,
 		".byte 0x66, 0x0f, 0xae, 0x30",  /* clwb (%%rax) */
 		X86_FEATURE_CLWB)
-		: [p] "+m" (*p)
+		: [p] "+m" (*(volatile char(*)[64])p)
 		: [pax] "a" (p));
 }
 
-- 
2.45.3

[PATCH 3/4] x86/asm: delete dummy variables in movdir64b()

[Alexey Dobriyan]

Cast to pointer-to-array instead.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 arch/x86/include/asm/special_insns.h | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index d349aa0f0a83..b24c6c945c38 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -215,13 +215,10 @@ static __always_inline void serialize(void)
 /* The dst parameter must be 64-bytes aligned */
 static inline void movdir64b(void *dst, const void *src)
 {
-	const struct { char _[64]; } *__src = src;
-	struct { char _[64]; } *__dst = dst;
-
 	/*
 	 * MOVDIR64B %(rdx), rax.
 	 *
-	 * Both __src and __dst must be memory constraints in order to tell the
+	 * Both src and dst must be memory constraints in order to tell the
 	 * compiler that no other memory accesses should be reordered around
 	 * this one.
 	 *
@@ -230,8 +227,8 @@ static inline void movdir64b(void *dst, const void *src)
 	 * I.e., not the pointers but what they point to, thus the deref'ing '*'.
 	 */
 	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
-		     : "+m" (*__dst)
-		     :  "m" (*__src), "a" (__dst), "d" (__src));
+		     : "+m" (*(char(*)[64])dst)
+		     :  "m" (*(const char(*)[64])src), "a" (dst), "d" (src));
 }
 
 static inline void movdir64b_io(void __iomem *dst, const void *src)
-- 
2.45.3

Re: [PATCH 3/4] x86/asm: delete dummy variables in movdir64b()

[Ingo Molnar]

* Alexey Dobriyan <adobriyan@gmail.com> wrote:

> Cast to pointer-to-array instead.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>  arch/x86/include/asm/special_insns.h | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index d349aa0f0a83..b24c6c945c38 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -215,13 +215,10 @@ static __always_inline void serialize(void)
>  /* The dst parameter must be 64-bytes aligned */
>  static inline void movdir64b(void *dst, const void *src)
>  {
> -	const struct { char _[64]; } *__src = src;
> -	struct { char _[64]; } *__dst = dst;
> -
>  	/*
>  	 * MOVDIR64B %(rdx), rax.
>  	 *
> -	 * Both __src and __dst must be memory constraints in order to tell the
> +	 * Both src and dst must be memory constraints in order to tell the
>  	 * compiler that no other memory accesses should be reordered around
>  	 * this one.
>  	 *
> @@ -230,8 +227,8 @@ static inline void movdir64b(void *dst, const void *src)
>  	 * I.e., not the pointers but what they point to, thus the deref'ing '*'.
>  	 */
>  	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> -		     : "+m" (*__dst)
> -		     :  "m" (*__src), "a" (__dst), "d" (__src));
> +		     : "+m" (*(char(*)[64])dst)
> +		     :  "m" (*(const char(*)[64])src), "a" (dst), "d" (src));

In what world is putting type casts inside asm() statements an 
improvement to the code?

Thanks,

	Ingo

Re: [PATCH 3/4] x86/asm: delete dummy variables in movdir64b()

[H. Peter Anvin]

On March 7, 2025 3:49:26 AM PST, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
>> Cast to pointer-to-array instead.
>> 
>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>> ---
>>  arch/x86/include/asm/special_insns.h | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
>> index d349aa0f0a83..b24c6c945c38 100644
>> --- a/arch/x86/include/asm/special_insns.h
>> +++ b/arch/x86/include/asm/special_insns.h
>> @@ -215,13 +215,10 @@ static __always_inline void serialize(void)
>>  /* The dst parameter must be 64-bytes aligned */
>>  static inline void movdir64b(void *dst, const void *src)
>>  {
>> -	const struct { char _[64]; } *__src = src;
>> -	struct { char _[64]; } *__dst = dst;
>> -
>>  	/*
>>  	 * MOVDIR64B %(rdx), rax.
>>  	 *
>> -	 * Both __src and __dst must be memory constraints in order to tell the
>> +	 * Both src and dst must be memory constraints in order to tell the
>>  	 * compiler that no other memory accesses should be reordered around
>>  	 * this one.
>>  	 *
>> @@ -230,8 +227,8 @@ static inline void movdir64b(void *dst, const void *src)
>>  	 * I.e., not the pointers but what they point to, thus the deref'ing '*'.
>>  	 */
>>  	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
>> -		     : "+m" (*__dst)
>> -		     :  "m" (*__src), "a" (__dst), "d" (__src));
>> +		     : "+m" (*(char(*)[64])dst)
>> +		     :  "m" (*(const char(*)[64])src), "a" (dst), "d" (src));
>
>In what world is putting type casts inside asm() statements an 
>improvement to the code?
>
>Thanks,
>
>	Ingo

I second that sentiment. This is a net negative.

Re: [PATCH 3/4] x86/asm: delete dummy variables in movdir64b()

[Alexey Dobriyan]

On Fri, Mar 07, 2025 at 12:49:26PM +0100, Ingo Molnar wrote:
> 
> * Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > Cast to pointer-to-array instead.
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > ---
> >  arch/x86/include/asm/special_insns.h | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> > index d349aa0f0a83..b24c6c945c38 100644
> > --- a/arch/x86/include/asm/special_insns.h
> > +++ b/arch/x86/include/asm/special_insns.h
> > @@ -215,13 +215,10 @@ static __always_inline void serialize(void)
> >  /* The dst parameter must be 64-bytes aligned */
> >  static inline void movdir64b(void *dst, const void *src)
> >  {
> > -	const struct { char _[64]; } *__src = src;
> > -	struct { char _[64]; } *__dst = dst;
> > -
> >  	/*
> >  	 * MOVDIR64B %(rdx), rax.
> >  	 *
> > -	 * Both __src and __dst must be memory constraints in order to tell the
> > +	 * Both src and dst must be memory constraints in order to tell the
> >  	 * compiler that no other memory accesses should be reordered around
> >  	 * this one.
> >  	 *
> > @@ -230,8 +227,8 @@ static inline void movdir64b(void *dst, const void *src)
> >  	 * I.e., not the pointers but what they point to, thus the deref'ing '*'.
> >  	 */
> >  	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
> > -		     : "+m" (*__dst)
> > -		     :  "m" (*__src), "a" (__dst), "d" (__src));
> > +		     : "+m" (*(char(*)[64])dst)
> > +		     :  "m" (*(const char(*)[64])src), "a" (dst), "d" (src));
> 
> In what world is putting type casts inside asm() statements an 
> improvement to the code?

In the same world where creating distracting variable whose only purpose
is to make a cast is considered not good.

Notice the cast is shorter, there is not "struct", so it is positive in
both vertical and horizontal directions.

Re: [PATCH 3/4] x86/asm: delete dummy variables in movdir64b()

[H. Peter Anvin]

On March 7, 2025 8:15:42 AM PST, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>On Fri, Mar 07, 2025 at 12:49:26PM +0100, Ingo Molnar wrote:
>> 
>> * Alexey Dobriyan <adobriyan@gmail.com> wrote:
>> 
>> > Cast to pointer-to-array instead.
>> > 
>> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>> > ---
>> >  arch/x86/include/asm/special_insns.h | 9 +++------
>> >  1 file changed, 3 insertions(+), 6 deletions(-)
>> > 
>> > diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
>> > index d349aa0f0a83..b24c6c945c38 100644
>> > --- a/arch/x86/include/asm/special_insns.h
>> > +++ b/arch/x86/include/asm/special_insns.h
>> > @@ -215,13 +215,10 @@ static __always_inline void serialize(void)
>> >  /* The dst parameter must be 64-bytes aligned */
>> >  static inline void movdir64b(void *dst, const void *src)
>> >  {
>> > -	const struct { char _[64]; } *__src = src;
>> > -	struct { char _[64]; } *__dst = dst;
>> > -
>> >  	/*
>> >  	 * MOVDIR64B %(rdx), rax.
>> >  	 *
>> > -	 * Both __src and __dst must be memory constraints in order to tell the
>> > +	 * Both src and dst must be memory constraints in order to tell the
>> >  	 * compiler that no other memory accesses should be reordered around
>> >  	 * this one.
>> >  	 *
>> > @@ -230,8 +227,8 @@ static inline void movdir64b(void *dst, const void *src)
>> >  	 * I.e., not the pointers but what they point to, thus the deref'ing '*'.
>> >  	 */
>> >  	asm volatile(".byte 0x66, 0x0f, 0x38, 0xf8, 0x02"
>> > -		     : "+m" (*__dst)
>> > -		     :  "m" (*__src), "a" (__dst), "d" (__src));
>> > +		     : "+m" (*(char(*)[64])dst)
>> > +		     :  "m" (*(const char(*)[64])src), "a" (dst), "d" (src));
>> 
>> In what world is putting type casts inside asm() statements an 
>> improvement to the code?
>
>In the same world where creating distracting variable whose only purpose
>is to make a cast is considered not good.
>
>Notice the cast is shorter, there is not "struct", so it is positive in
>both vertical and horizontal directions.

Stuffing more into a single statement – and asm statements are invariably complex – is not good for clarity. Please, no.

[PATCH 4/4] x86/asm: delete dummy variable in enqcmds()

[Alexey Dobriyan]

Cast to pointer-to-array instead.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 arch/x86/include/asm/special_insns.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index b24c6c945c38..aa5f015ff2a2 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -257,8 +257,6 @@ static inline void movdir64b_io(void __iomem *dst, const void *src)
  */
 static inline int enqcmds(void __iomem *dst, const void *src)
 {
-	const struct { char _[64]; } *__src = src;
-	struct { char _[64]; } __iomem *__dst = dst;
 	bool zf;
 
 	/*
@@ -268,8 +266,8 @@ static inline int enqcmds(void __iomem *dst, const void *src)
 	 */
 	asm volatile(".byte 0xf3, 0x0f, 0x38, 0xf8, 0x02, 0x66, 0x90"
 		     CC_SET(z)
-		     : CC_OUT(z) (zf), "+m" (*__dst)
-		     : "m" (*__src), "a" (__dst), "d" (__src));
+		     : CC_OUT(z) (zf), "+m" (*(char __iomem(*)[64])dst)
+		     : "m" (*(const char(*)[64])src), "a" (dst), "d" (src));
 
 	/* Submission failure is indicated via EFLAGS.ZF=1 */
 	if (zf)
-- 
2.45.3

Re: [PATCH 1/4] x86/asm: inline constant inputs in rdpkru(), wrpkru()

[Dave Hansen]

On 3/6/25 22:12, Alexey Dobriyan wrote:
>  static inline u32 rdpkru(void)
>  {
> -	u32 ecx = 0;
>  	u32 edx, pkru;
>  
>  	/*
> @@ -88,20 +87,18 @@ static inline u32 rdpkru(void)
>  	 */
>  	asm volatile(".byte 0x0f,0x01,0xee\n\t"
>  		     : "=a" (pkru), "=d" (edx)
> -		     : "c" (ecx));
> +		     : "c" (0));
>  	return pkru;
>  }

Hey Alexey,

Again, thanks for the patch. I'll explain for a sec why I wrote it this way.

If you're looking at the RDPKRU spec, it literally says "ECX must be 0".
If you see "ecx = 0" in the code, any old dummy can tell that the
_intent_ is to set ecx=0. Anybody that can read C can at least
understand the intent.

But '"c" (0)', on the other hand, requires knowing inline asm and
specifically how x86 inline asm names its registers. It's not utterly
and blatantly obvious that "c" means "ecx".

I don't expect you to totally agree with me on this one. But I don't
think we can take your patch. Two reasons: we can't just be taking
patches for deeply personal style preferences. If we did, the code
history would just be filled with thrash. Second, I kinda like the code
as-is.