| Message ID | 20190403164156.19645-13-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/27] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig() | expand |
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?
On Wed, Apr 10, 2019 at 06:36:15PM +0200, Borislav Petkov wrote: > 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, like this: --- diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h index 2779ace16d23..e2948ce1376c 100644 --- a/arch/x86/include/asm/pgtable.h +++ b/arch/x86/include/asm/pgtable.h @@ -127,14 +127,14 @@ 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 rdpkru(); return 0; } static inline void write_pkru(u32 pkru) { if (boot_cpu_has(X86_FEATURE_OSPKE)) - __write_pkru(pkru); + wrpkru(pkru); } static inline int pte_young(pte_t pte) diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h index 43c029cdc3fe..396ff572d66a 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 rdpkru(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 wrpkru(u32 pkru) { u32 ecx = 0, edx = 0; @@ -119,12 +119,12 @@ static inline void __write_pkru(u32 pkru) : : "a" (pkru), "c"(ecx), "d"(edx)); } #else -static inline u32 __read_pkru(void) +static inline u32 rdpkru(void) { return 0; } -static inline void __write_pkru(u32 pkru) +static inline void wrpkru(u32 pkru) { } #endif diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index ab432a930ae8..85260aa2a920 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6413,7 +6413,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) if (static_cpu_has(X86_FEATURE_PKU) && kvm_read_cr4_bits(vcpu, X86_CR4_PKE) && vcpu->arch.pkru != vmx->host_pkru) - __write_pkru(vcpu->arch.pkru); + wrpkru(vcpu->arch.pkru); pt_guest_enter(vmx); @@ -6501,9 +6501,9 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) */ if (static_cpu_has(X86_FEATURE_PKU) && kvm_read_cr4_bits(vcpu, X86_CR4_PKE)) { - vcpu->arch.pkru = __read_pkru(); + vcpu->arch.pkru = rdpkru(); if (vcpu->arch.pkru != vmx->host_pkru) - __write_pkru(vmx->host_pkru); + wrpkru(vmx->host_pkru); } vmx->nested.nested_run_pending = 0;
On 2019-04-10 18:52:41 [+0200], Borislav Petkov wrote: > On Wed, Apr 10, 2019 at 06:36:15PM +0200, Borislav Petkov wrote: > > 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, like this: > > --- > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h > index 2779ace16d23..e2948ce1376c 100644 > --- a/arch/x86/include/asm/pgtable.h > +++ b/arch/x86/include/asm/pgtable.h > @@ -127,14 +127,14 @@ 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 rdpkru(); > return 0; > } > > static inline void write_pkru(u32 pkru) > { > if (boot_cpu_has(X86_FEATURE_OSPKE)) > - __write_pkru(pkru); > + wrpkru(pkru); I think if this is a simple 's@__write_pkru_ins@wrpkru@g' 's@__read_pkru_ins@rdpkru@g' then it should work just fine and match what Dave asked for. > } > > static inline int pte_young(pte_t pte) Sebastian
On 4/10/19 2:25 PM, Sebastian Andrzej Siewior wrote: >> static inline void write_pkru(u32 pkru) >> { >> if (boot_cpu_has(X86_FEATURE_OSPKE)) >> - __write_pkru(pkru); >> + wrpkru(pkru); > I think if this is a simple > > 's@__write_pkru_ins@wrpkru@g' > 's@__read_pkru_ins@rdpkru@g' > > then it should work just fine and match what Dave asked for. I'm fine with it either way, fwiw.
On Wed, Apr 10, 2019 at 02:29:26PM -0700, Dave Hansen wrote: > On 4/10/19 2:25 PM, Sebastian Andrzej Siewior wrote: > >> static inline void write_pkru(u32 pkru) > >> { > >> if (boot_cpu_has(X86_FEATURE_OSPKE)) > >> - __write_pkru(pkru); > >> + wrpkru(pkru); > > I think if this is a simple > > > > 's@__write_pkru_ins@wrpkru@g' > > 's@__read_pkru_ins@rdpkru@g' > > > > then it should work just fine and match what Dave asked for. > > I'm fine with it either way, fwiw. Final version: --- From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: Wed, 3 Apr 2019 18:41:41 +0200 Subject: [PATCH] x86/pkru: Provide *pkru() helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dave Hansen 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 asm into a function called like the instruction itself. __write_pkru() will just invoke wrpkru() but in a follow-up patch will also read back the value. [ bp: Convert asm opcode wrapper names to rd/wrpkru(). ] Suggested-by: Dave Hansen <dave.hansen@intel.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: Dave Hansen <dave.hansen@intel.com> Reviewed-by: Thomas Gleixner <tglx@linutronix.de> Cc: Andi Kleen <ak@linux.intel.com> Cc: Andy Lutomirski <luto@kernel.org> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Ingo Molnar <mingo@redhat.com> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com> Cc: Joerg Roedel <jroedel@suse.de> Cc: Juergen Gross <jgross@suse.com> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> Cc: kvm ML <kvm@vger.kernel.org> Cc: Michal Hocko <mhocko@suse.cz> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: "Radim Krčmář" <rkrcmar@redhat.com> Cc: Rik van Riel <riel@surriel.com> Cc: x86-ml <x86@kernel.org> Link: https://lkml.kernel.org/r/20190403164156.19645-13-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 2779ace16d23..e8875ca75623 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 rdpkru(); return 0; } diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h index 43c029cdc3fe..34897e2b52c9 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 rdpkru(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 wrpkru(u32 pkru) { u32 ecx = 0, edx = 0; @@ -118,8 +118,14 @@ static inline void __write_pkru(u32 pkru) asm volatile(".byte 0x0f,0x01,0xef\n\t" : : "a" (pkru), "c"(ecx), "d"(edx)); } + +static inline void __write_pkru(u32 pkru) +{ + wrpkru(pkru); +} + #else -static inline u32 __read_pkru(void) +static inline u32 rdpkru(void) { return 0; } diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index ab432a930ae8..dd1e1eea4f0b 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6501,7 +6501,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) */ if (static_cpu_has(X86_FEATURE_PKU) && kvm_read_cr4_bits(vcpu, X86_CR4_PKE)) { - vcpu->arch.pkru = __read_pkru(); + vcpu->arch.pkru = rdpkru(); if (vcpu->arch.pkru != vmx->host_pkru) __write_pkru(vmx->host_pkru); }
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; @@ -118,8 +118,14 @@ static inline void __write_pkru(u32 pkru) asm volatile(".byte 0x0f,0x01,0xef\n\t" : : "a" (pkru), "c"(ecx), "d"(edx)); } + +static inline void __write_pkru(u32 pkru) +{ + __write_pkru_ins(pkru); +} + #else -static inline u32 __read_pkru(void) +static inline u32 __read_pkru_ins(void) { return 0; } diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index ab432a930ae86..96dec81430970 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6501,7 +6501,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) */ if (static_cpu_has(X86_FEATURE_PKU) && kvm_read_cr4_bits(vcpu, X86_CR4_PKE)) { - vcpu->arch.pkru = __read_pkru(); + vcpu->arch.pkru = __read_pkru_ins(); if (vcpu->arch.pkru != vmx->host_pkru) __write_pkru(vmx->host_pkru); }
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(-)