Messages in this thread Patch in this message | | | Date | Tue, 11 Aug 2020 09:41:27 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] x86/paravirt: Add missing noinstr to arch_local*() helpers |
| |
On Fri, Aug 07, 2020 at 05:19:03PM +0200, Marco Elver wrote:
> My hypothesis here is simply that kvm_wait() may be called in a place > where we get the same case I mentioned to Peter, > > raw_local_irq_save(); /* or other IRQs off without tracing */ > ... > kvm_wait() /* IRQ state tracing gets confused */ > ... > raw_local_irq_restore(); > > and therefore, using raw variants in kvm_wait() works. It's also safe > because it doesn't call any other libraries that would result in corrupt
Yes, this is definitely an issue.
Tracing, we also musn't call into tracing when using raw_local_irq_*(). Because then we re-intoduce this same issue all over again.
Both halt() and safe_halt() are more paravirt calls, but given we're in a KVM paravirt call already, I suppose we can directly use native_*() here.
Something like so then... I suppose, but then the Xen variants need TLC too.
--- arch/x86/include/asm/irqflags.h | 4 ++-- arch/x86/include/asm/kvm_para.h | 18 +++++++++--------- arch/x86/kernel/kvm.c | 14 +++++++------- 3 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h index 02a0cf547d7b..7c614db25274 100644 --- a/arch/x86/include/asm/irqflags.h +++ b/arch/x86/include/asm/irqflags.h @@ -54,13 +54,13 @@ static __always_inline void native_irq_enable(void) asm volatile("sti": : :"memory"); }
-static inline __cpuidle void native_safe_halt(void) +static __always_inline __cpuidle void native_safe_halt(void) { mds_idle_clear_cpu_buffers(); asm volatile("sti; hlt": : :"memory"); }
-static inline __cpuidle void native_halt(void) +static __always_inline __cpuidle void native_halt(void) { mds_idle_clear_cpu_buffers(); asm volatile("hlt": : :"memory"); diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h index 49d3a9edb06f..90f7ea58ebb0 100644 --- a/arch/x86/include/asm/kvm_para.h +++ b/arch/x86/include/asm/kvm_para.h @@ -30,7 +30,7 @@ static inline bool kvm_check_and_clear_guest_paused(void) * noted by the particular hypercall. */
-static inline long kvm_hypercall0(unsigned int nr) +static __always_inline long kvm_hypercall0(unsigned int nr) { long ret; asm volatile(KVM_HYPERCALL @@ -40,7 +40,7 @@ static inline long kvm_hypercall0(unsigned int nr) return ret; }
-static inline long kvm_hypercall1(unsigned int nr, unsigned long p1) +static __always_inline long kvm_hypercall1(unsigned int nr, unsigned long p1) { long ret; asm volatile(KVM_HYPERCALL @@ -50,8 +50,8 @@ static inline long kvm_hypercall1(unsigned int nr, unsigned long p1) return ret; }
-static inline long kvm_hypercall2(unsigned int nr, unsigned long p1, - unsigned long p2) +static __always_inline long kvm_hypercall2(unsigned int nr, unsigned long p1, + unsigned long p2) { long ret; asm volatile(KVM_HYPERCALL @@ -61,8 +61,8 @@ static inline long kvm_hypercall2(unsigned int nr, unsigned long p1, return ret; }
-static inline long kvm_hypercall3(unsigned int nr, unsigned long p1, - unsigned long p2, unsigned long p3) +static __always_inline long kvm_hypercall3(unsigned int nr, unsigned long p1, + unsigned long p2, unsigned long p3) { long ret; asm volatile(KVM_HYPERCALL @@ -72,9 +72,9 @@ static inline long kvm_hypercall3(unsigned int nr, unsigned long p1, return ret; }
-static inline long kvm_hypercall4(unsigned int nr, unsigned long p1, - unsigned long p2, unsigned long p3, - unsigned long p4) +static __always_inline long kvm_hypercall4(unsigned int nr, unsigned long p1, + unsigned long p2, unsigned long p3, + unsigned long p4) { long ret; asm volatile(KVM_HYPERCALL diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 233c77d056c9..15f8dfd8812d 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -779,7 +779,7 @@ arch_initcall(kvm_alloc_cpumask); #ifdef CONFIG_PARAVIRT_SPINLOCKS
/* Kick a cpu by its apicid. Used to wake up a halted vcpu */ -static void kvm_kick_cpu(int cpu) +static notrace kvm_kick_cpu(int cpu) { int apicid; unsigned long flags = 0; @@ -790,14 +790,14 @@ static void kvm_kick_cpu(int cpu)
#include <asm/qspinlock.h>
-static void kvm_wait(u8 *ptr, u8 val) +static notrace kvm_wait(u8 *ptr, u8 val) { unsigned long flags;
if (in_nmi()) return;
- local_irq_save(flags); + raw_local_irq_save(flags);
if (READ_ONCE(*ptr) != val) goto out; @@ -808,16 +808,16 @@ static void kvm_wait(u8 *ptr, u8 val) * in irq spinlock slowpath and no spurious interrupt occur to save us. */ if (arch_irqs_disabled_flags(flags)) - halt(); + native_halt(); else - safe_halt(); + native_safe_halt();
out: - local_irq_restore(flags); + raw_local_irq_restore(flags); }
#ifdef CONFIG_X86_32 -__visible bool __kvm_vcpu_is_preempted(long cpu) +__visible notrace bool __kvm_vcpu_is_preempted(long cpu) { struct kvm_steal_time *src = &per_cpu(steal_time, cpu);
| |