[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

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

[Dave Hansen]

On 3/6/25 13:07, Alexey Dobriyan wrote:
>  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));
>  }

Hey Alexey,

I appreciate the patch. But I do like how it's written currently. I
honestly kinda wish it went even further and did:

	u32 eax = pkru;

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

[Alexey Dobriyan]

On Thu, Mar 06, 2025 at 01:35:01PM -0800, Dave Hansen wrote:
> On 3/6/25 13:07, Alexey Dobriyan wrote:
> >  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));
> >  }
> 
> Hey Alexey,
> 
> I appreciate the patch. But I do like how it's written currently. I
> honestly kinda wish it went even further and did:
> 
> 	u32 eax = pkru;

I _think_ you can write   "eax" (pkru) .

I don't like the comment either. It just reiterates the SDM.
Trailing \n\t too -- they don't do anything in one-liners.

The function is basically:

	// wrpkru
	asm volatile (
		".byte 0x0f,0x01,0xef"
		:
		: "a" (pkru), "d" (0) , "c" (0)
	);

[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

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.