Re: [PATCH 12/27] x86/pkru: Provide .*_pkru_ins() functions

[Borislav Petkov <bp@alien8.de>]

On Wed, Apr 03, 2019 at 06:41:41PM +0200, Sebastian Andrzej Siewior wrote:
> Dave Hansen has asked for __read_pkru() and __write_pkru() to be symmetrical.
> As part of the series __write_pkru() will read back the value and only write it
> if it is different.
> In order to make both functions symmetrical move the function containing only
> the opcode into a function with _isn() suffix. __write_pkru() will just invoke
> __write_pkru_isn() but in a flowup patch will also read back the value.
> 
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/include/asm/pgtable.h       |  2 +-
>  arch/x86/include/asm/special_insns.h | 12 +++++++++---
>  arch/x86/kvm/vmx/vmx.c               |  2 +-
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 2779ace16d23f..64333e5222cd9 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -127,7 +127,7 @@ static inline int pte_dirty(pte_t pte)
>  static inline u32 read_pkru(void)
>  {
>  	if (boot_cpu_has(X86_FEATURE_OSPKE))
> -		return __read_pkru();
> +		return __read_pkru_ins();
>  	return 0;
>  }
>  
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 43c029cdc3fe8..27328606ff687 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -92,7 +92,7 @@ static inline void native_write_cr8(unsigned long val)
>  #endif
>  
>  #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> -static inline u32 __read_pkru(void)
> +static inline u32 __read_pkru_ins(void)
>  {
>  	u32 ecx = 0;
>  	u32 edx, pkru;
> @@ -107,7 +107,7 @@ static inline u32 __read_pkru(void)
>  	return pkru;
>  }
>  
> -static inline void __write_pkru(u32 pkru)
> +static inline void __write_pkru_ins(u32 pkru)
>  {
>  	u32 ecx = 0, edx = 0;
> 

Well, this is going in the wrong direction. The proper thing to do would
be to have:

rdpkru()
wrpkru()

which only do the inline asm with the respective opcodes. Just like
rdtsc(), clflush(), etc.

IOW, the functions which correspond to the instructions should be called
just like the respective instructions they implement in asm.

Then, all the helpers around them should have different names to denote
what they do. Like the current rdpkru() which does the assertion
checking should be renamed to something like read_pkey_user() or so.
Ditto for the current wrpkru().

Makes sense?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.