* [PATCH 0/5] KVM: rename and extend vcpu->requests API
@ 2017-02-16 16:04 Radim Krčmář
2017-02-16 16:04 ` [PATCH 1/5] KVM: change API for requests to match bit operations Radim Krčmář
` (4 more replies)
0 siblings, 5 replies; 31+ messages in thread
From: Radim Krčmář @ 2017-02-16 16:04 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger,
Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall
KVM requests are not used with one API, but are a mix of open-coded
setting and kicking, which is what this series aims to normalize.
The main problem is described in comment of patch 1:
There are three main kinds of requests
1) requests from a VCPU to itself,
2) requests for a remote VCPU,
3) and requests for a remote VCPU that also expose some data
and we have just one function for setting requests.
The first type of requests leads to just set_bit. The other two need to
notify the remote VCPU -- often with kvm_vcpu_kick, but if the VCPU is
known to be halted, it can be just swait_activate(). And the third one
needs a memory barrier before setting the bit and after testing it.
The kvm_make_request function we have now is suboptimal in all of these
three cases -- it starts with a barrier, but then doesn't kick, so we
cannot use it for (1) or (2) due to performance issues and we need to
explicitly kick afterwards in (3).
Putting all these into one function would require runtime overhead as it
is not easy to tell if the request is local or remote, but the need for
a barrier can be decided at compile time.
The result of this series is still a mess as it does not include
kvm_vcpu_kick() into kvm_set_request(), but I hope to get an early
feedback about the idea.
(Series based on current kvm/queue.)
Radim Krčmář (5):
KVM: change API for requests to match bit operations
KVM: add KVM request variants without barrier
KVM: optimize kvm_make_all_cpus_request
KVM: add __kvm_request_needs_mb
KVM: add kvm_request_pending
arch/mips/kvm/emulate.c | 4 +-
arch/mips/kvm/trap_emul.c | 4 +-
arch/powerpc/kvm/book3s_pr.c | 4 +-
arch/powerpc/kvm/book3s_pr_papr.c | 2 +-
arch/powerpc/kvm/booke.c | 22 +++---
arch/powerpc/kvm/powerpc.c | 8 +--
arch/s390/kvm/kvm-s390.c | 26 +++----
arch/s390/kvm/kvm-s390.h | 4 +-
arch/s390/kvm/priv.c | 4 +-
arch/x86/kvm/hyperv.c | 14 ++--
arch/x86/kvm/i8259.c | 2 +-
arch/x86/kvm/lapic.c | 22 +++---
arch/x86/kvm/mmu.c | 14 ++--
arch/x86/kvm/pmu.c | 6 +-
arch/x86/kvm/svm.c | 12 ++--
arch/x86/kvm/vmx.c | 32 ++++-----
arch/x86/kvm/x86.c | 144 +++++++++++++++++++-------------------
include/linux/kvm_host.h | 96 +++++++++++++++++++++----
virt/kvm/kvm_main.c | 9 ++-
19 files changed, 253 insertions(+), 176 deletions(-)
--
2.11.1
^ permalink raw reply [flat|nested] 31+ messages in thread* [PATCH 1/5] KVM: change API for requests to match bit operations 2017-02-16 16:04 [PATCH 0/5] KVM: rename and extend vcpu->requests API Radim Krčmář @ 2017-02-16 16:04 ` Radim Krčmář 2017-02-17 9:30 ` Cornelia Huck 2017-02-16 16:04 ` [PATCH 2/5] KVM: add KVM request variants without barrier Radim Krčmář ` (3 subsequent siblings) 4 siblings, 1 reply; 31+ messages in thread From: Radim Krčmář @ 2017-02-16 16:04 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall kvm_make_request was a wrapper that added barriers to bit_set and kvm_check_request did the same for bit_test and bit_check, but the name was not very obvious and we were also lacking operations that cover bit_test and bit_clear, which resulted in an inconsistent use. The renaming: kvm_request_set <- kvm_make_request kvm_request_test_and_clear <- kvm_check_request Automated with coccinelle script: @@ expression VCPU, REQ; @@ -kvm_make_request(REQ, VCPU) +kvm_request_set(REQ, VCPU) @@ expression VCPU, REQ; @@ -kvm_check_request(REQ, VCPU) +kvm_request_test_and_clear(REQ, VCPU) --- arch/mips/kvm/emulate.c | 2 +- arch/mips/kvm/trap_emul.c | 2 +- arch/powerpc/kvm/book3s_pr.c | 2 +- arch/powerpc/kvm/booke.c | 16 +++--- arch/powerpc/kvm/powerpc.c | 2 +- arch/s390/kvm/kvm-s390.c | 22 ++++---- arch/s390/kvm/kvm-s390.h | 4 +- arch/s390/kvm/priv.c | 4 +- arch/x86/kvm/hyperv.c | 14 ++--- arch/x86/kvm/i8259.c | 2 +- arch/x86/kvm/lapic.c | 22 ++++---- arch/x86/kvm/mmu.c | 14 ++--- arch/x86/kvm/pmu.c | 6 +- arch/x86/kvm/svm.c | 12 ++-- arch/x86/kvm/vmx.c | 30 +++++----- arch/x86/kvm/x86.c | 128 +++++++++++++++++++++---------------------- include/linux/kvm_host.h | 30 ++++++++-- virt/kvm/kvm_main.c | 4 +- 18 files changed, 167 insertions(+), 149 deletions(-) diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c index d40cfaad4529..ee4af898bcf6 100644 --- a/arch/mips/kvm/emulate.c +++ b/arch/mips/kvm/emulate.c @@ -864,7 +864,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu) * We we are runnable, then definitely go off to user space to * check if any I/O interrupts are pending. */ - if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) { clear_bit(KVM_REQ_UNHALT, &vcpu->requests); vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN; } diff --git a/arch/mips/kvm/trap_emul.c b/arch/mips/kvm/trap_emul.c index b1fa53b252ea..35068823cde6 100644 --- a/arch/mips/kvm/trap_emul.c +++ b/arch/mips/kvm/trap_emul.c @@ -1032,7 +1032,7 @@ static void kvm_trap_emul_check_requests(struct kvm_vcpu *vcpu, int cpu, if (likely(!vcpu->requests)) return; - if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_TLB_FLUSH, vcpu)) { /* * Both kernel & user GVA mappings must be invalidated. The * caller is just about to check whether the ASID is stale diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c index d4dfc0ca2a44..7af5154e848b 100644 --- a/arch/powerpc/kvm/book3s_pr.c +++ b/arch/powerpc/kvm/book3s_pr.c @@ -241,7 +241,7 @@ static int kvmppc_core_check_requests_pr(struct kvm_vcpu *vcpu) /* We misuse TLB_FLUSH to indicate that we want to clear all shadow cache entries */ - if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_TLB_FLUSH, vcpu)) kvmppc_mmu_pte_flush(vcpu, 0, 0); return r; diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 0514cbd4e533..806caaf60e10 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -503,7 +503,7 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu, kvmppc_set_dar(vcpu, vcpu->arch.queued_dear); if (update_epr == true) { if (vcpu->arch.epr_flags & KVMPPC_EPR_USER) - kvm_make_request(KVM_REQ_EPR_EXIT, vcpu); + kvm_request_set(KVM_REQ_EPR_EXIT, vcpu); else if (vcpu->arch.epr_flags & KVMPPC_EPR_KERNEL) { BUG_ON(vcpu->arch.irq_type != KVMPPC_IRQ_MPIC); kvmppc_mpic_set_epr(vcpu); @@ -617,7 +617,7 @@ void kvmppc_watchdog_func(unsigned long data) if (new_tsr & TSR_WIS) { smp_wmb(); - kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); + kvm_request_set(KVM_REQ_PENDING_TIMER, vcpu); kvm_vcpu_kick(vcpu); } @@ -628,7 +628,7 @@ void kvmppc_watchdog_func(unsigned long data) if (final && (vcpu->arch.tcr & TCR_WRC_MASK) && vcpu->arch.watchdog_enabled) { smp_wmb(); - kvm_make_request(KVM_REQ_WATCHDOG, vcpu); + kvm_request_set(KVM_REQ_WATCHDOG, vcpu); kvm_vcpu_kick(vcpu); } @@ -704,19 +704,19 @@ int kvmppc_core_check_requests(struct kvm_vcpu *vcpu) { int r = 1; /* Indicate we want to get back into the guest */ - if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_PENDING_TIMER, vcpu)) update_timer_ints(vcpu); #if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC) - if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_TLB_FLUSH, vcpu)) kvmppc_core_flush_tlb(vcpu); #endif - if (kvm_check_request(KVM_REQ_WATCHDOG, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_WATCHDOG, vcpu)) { vcpu->run->exit_reason = KVM_EXIT_WATCHDOG; r = 0; } - if (kvm_check_request(KVM_REQ_EPR_EXIT, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_EPR_EXIT, vcpu)) { vcpu->run->epr.epr = 0; vcpu->arch.epr_needed = true; vcpu->run->exit_reason = KVM_EXIT_EPR; @@ -1830,7 +1830,7 @@ void kvmppc_set_tsr_bits(struct kvm_vcpu *vcpu, u32 tsr_bits) { set_bits(tsr_bits, &vcpu->arch.tsr); smp_wmb(); - kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); + kvm_request_set(KVM_REQ_PENDING_TIMER, vcpu); kvm_vcpu_kick(vcpu); } diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 1fe1391ba2c2..19e89419a843 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -190,7 +190,7 @@ int kvmppc_kvm_pv(struct kvm_vcpu *vcpu) * NX. If that's the case, remove !PR NX capability. */ vcpu->arch.disable_kernel_nx = true; - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); } vcpu->arch.magic_page_pa = param1 & ~0xfffULL; diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 99e35fe0dea8..db3c742a5dc9 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2097,7 +2097,7 @@ void exit_sie(struct kvm_vcpu *vcpu) /* Kick a guest cpu out of SIE to process a request synchronously */ void kvm_s390_sync_request(int req, struct kvm_vcpu *vcpu) { - kvm_make_request(req, vcpu); + kvm_request_set(req, vcpu); kvm_s390_vcpu_request(vcpu); } @@ -2402,24 +2402,24 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) * already finished. We might race against a second unmapper that * wants to set the blocking bit. Lets just retry the request loop. */ - if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_MMU_RELOAD, vcpu)) { int rc; rc = gmap_mprotect_notify(vcpu->arch.gmap, kvm_s390_get_prefix(vcpu), PAGE_SIZE * 2, PROT_WRITE); if (rc) { - kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); + kvm_request_set(KVM_REQ_MMU_RELOAD, vcpu); return rc; } goto retry; } - if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_TLB_FLUSH, vcpu)) { vcpu->arch.sie_block->ihcpu = 0xffff; goto retry; } - if (kvm_check_request(KVM_REQ_ENABLE_IBS, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_ENABLE_IBS, vcpu)) { if (!ibs_enabled(vcpu)) { trace_kvm_s390_enable_disable_ibs(vcpu->vcpu_id, 1); atomic_or(CPUSTAT_IBS, @@ -2428,7 +2428,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) goto retry; } - if (kvm_check_request(KVM_REQ_DISABLE_IBS, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_DISABLE_IBS, vcpu)) { if (ibs_enabled(vcpu)) { trace_kvm_s390_enable_disable_ibs(vcpu->vcpu_id, 0); atomic_andnot(CPUSTAT_IBS, @@ -2437,7 +2437,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) goto retry; } - if (kvm_check_request(KVM_REQ_ICPT_OPEREXC, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_ICPT_OPEREXC, vcpu)) { vcpu->arch.sie_block->ictl |= ICTL_OPEREXC; goto retry; } @@ -2723,7 +2723,7 @@ static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) if (kvm_run->kvm_dirty_regs & KVM_SYNC_CRS) { memcpy(&vcpu->arch.sie_block->gcr, &kvm_run->s.regs.crs, 128); /* some control register changes require a tlb flush */ - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); } if (kvm_run->kvm_dirty_regs & KVM_SYNC_ARCH0) { kvm_s390_set_cpu_timer(vcpu, kvm_run->s.regs.cputm); @@ -2923,7 +2923,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr) static void __disable_ibs_on_vcpu(struct kvm_vcpu *vcpu) { - kvm_check_request(KVM_REQ_ENABLE_IBS, vcpu); + kvm_request_test_and_clear(KVM_REQ_ENABLE_IBS, vcpu); kvm_s390_sync_request(KVM_REQ_DISABLE_IBS, vcpu); } @@ -2941,7 +2941,7 @@ static void __enable_ibs_on_vcpu(struct kvm_vcpu *vcpu) { if (!sclp.has_ibs) return; - kvm_check_request(KVM_REQ_DISABLE_IBS, vcpu); + kvm_request_test_and_clear(KVM_REQ_DISABLE_IBS, vcpu); kvm_s390_sync_request(KVM_REQ_ENABLE_IBS, vcpu); } @@ -2979,7 +2979,7 @@ void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu) * Another VCPU might have used IBS while we were offline. * Let's play safe and flush the VCPU at startup. */ - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); spin_unlock(&vcpu->kvm->arch.start_stop_lock); return; } diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h index af9fa91a0c91..ceb12cbcabaf 100644 --- a/arch/s390/kvm/kvm-s390.h +++ b/arch/s390/kvm/kvm-s390.h @@ -82,8 +82,8 @@ static inline void kvm_s390_set_prefix(struct kvm_vcpu *vcpu, u32 prefix) VCPU_EVENT(vcpu, 3, "set prefix of cpu %03u to 0x%x", vcpu->vcpu_id, prefix); vcpu->arch.sie_block->prefix = prefix >> GUEST_PREFIX_SHIFT; - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); - kvm_make_request(KVM_REQ_MMU_RELOAD, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_MMU_RELOAD, vcpu); } static inline u64 kvm_s390_get_base_disp_s(struct kvm_vcpu *vcpu, u8 *ar) diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c index 1ecc1cffdf7c..bf3cc6117ae5 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -998,7 +998,7 @@ int kvm_s390_handle_lctl(struct kvm_vcpu *vcpu) break; reg = (reg + 1) % 16; } while (1); - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); return 0; } @@ -1070,7 +1070,7 @@ static int handle_lctlg(struct kvm_vcpu *vcpu) break; reg = (reg + 1) % 16; } while (1); - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); return 0; } diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 08b27e0c7b71..851fa8a9a5bc 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -100,7 +100,7 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, __clear_bit(vector, synic->auto_eoi_bitmap); /* Load SynIC vectors into EOI exit bitmap */ - kvm_make_request(KVM_REQ_SCAN_IOAPIC, synic_to_vcpu(synic)); + kvm_request_set(KVM_REQ_SCAN_IOAPIC, synic_to_vcpu(synic)); return 0; } @@ -170,7 +170,7 @@ static void kvm_hv_notify_acked_sint(struct kvm_vcpu *vcpu, u32 sint) } } if (stimers_pending) - kvm_make_request(KVM_REQ_HV_STIMER, vcpu); + kvm_request_set(KVM_REQ_HV_STIMER, vcpu); idx = srcu_read_lock(&kvm->irq_srcu); gsi = atomic_read(&synic->sint_to_gsi[sint]); @@ -190,7 +190,7 @@ static void synic_exit(struct kvm_vcpu_hv_synic *synic, u32 msr) hv_vcpu->exit.u.synic.evt_page = synic->evt_page; hv_vcpu->exit.u.synic.msg_page = synic->msg_page; - kvm_make_request(KVM_REQ_HV_EXIT, vcpu); + kvm_request_set(KVM_REQ_HV_EXIT, vcpu); } static int synic_set_msr(struct kvm_vcpu_hv_synic *synic, @@ -410,7 +410,7 @@ static void stimer_mark_pending(struct kvm_vcpu_hv_stimer *stimer, set_bit(stimer->index, vcpu_to_hv_vcpu(vcpu)->stimer_pending_bitmap); - kvm_make_request(KVM_REQ_HV_STIMER, vcpu); + kvm_request_set(KVM_REQ_HV_STIMER, vcpu); if (vcpu_kick) kvm_vcpu_kick(vcpu); } @@ -752,7 +752,7 @@ static int kvm_hv_msr_set_crash_ctl(struct kvm_vcpu *vcpu, u64 data, bool host) hv->hv_crash_param[4]); /* Send notification about crash to user space */ - kvm_make_request(KVM_REQ_HV_CRASH, vcpu); + kvm_request_set(KVM_REQ_HV_CRASH, vcpu); } return 0; @@ -939,7 +939,7 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data, case HV_X64_MSR_REFERENCE_TSC: hv->hv_tsc_page = data; if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE) - kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); break; case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4: return kvm_hv_msr_set_crash_data(vcpu, @@ -950,7 +950,7 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data, case HV_X64_MSR_RESET: if (data == 1) { vcpu_debug(vcpu, "hyper-v reset requested\n"); - kvm_make_request(KVM_REQ_HV_RESET, vcpu); + kvm_request_set(KVM_REQ_HV_RESET, vcpu); } break; default: diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c index 73ea24d4f119..b85225d36aae 100644 --- a/arch/x86/kvm/i8259.c +++ b/arch/x86/kvm/i8259.c @@ -67,7 +67,7 @@ static void pic_unlock(struct kvm_pic *s) if (!found) return; - kvm_make_request(KVM_REQ_EVENT, found); + kvm_request_set(KVM_REQ_EVENT, found); kvm_vcpu_kick(found); } } diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index bad6a25067bc..0297eea0d47b 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -616,7 +616,7 @@ static void apic_update_ppr(struct kvm_lapic *apic) if (__apic_update_ppr(apic, &ppr) && apic_has_interrupt_for_ppr(apic, ppr) != -1) - kvm_make_request(KVM_REQ_EVENT, apic->vcpu); + kvm_request_set(KVM_REQ_EVENT, apic->vcpu); } void kvm_apic_update_ppr(struct kvm_vcpu *vcpu) @@ -978,7 +978,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, else { kvm_lapic_set_irr(vector, apic); - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); kvm_vcpu_kick(vcpu); } break; @@ -986,13 +986,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, case APIC_DM_REMRD: result = 1; vcpu->arch.pv.pv_unhalted = 1; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); kvm_vcpu_kick(vcpu); break; case APIC_DM_SMI: result = 1; - kvm_make_request(KVM_REQ_SMI, vcpu); + kvm_request_set(KVM_REQ_SMI, vcpu); kvm_vcpu_kick(vcpu); break; @@ -1010,7 +1010,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, /* make sure pending_events is visible before sending * the request */ smp_wmb(); - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); kvm_vcpu_kick(vcpu); } else { apic_debug("Ignoring de-assert INIT to vcpu %d\n", @@ -1026,7 +1026,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, /* make sure sipi_vector is visible for the receiver */ smp_wmb(); set_bit(KVM_APIC_SIPI, &apic->pending_events); - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); kvm_vcpu_kick(vcpu); break; @@ -1067,7 +1067,7 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector) /* Request a KVM exit to inform the userspace IOAPIC. */ if (irqchip_split(apic->vcpu->kvm)) { apic->vcpu->arch.pending_ioapic_eoi = vector; - kvm_make_request(KVM_REQ_IOAPIC_EOI_EXIT, apic->vcpu); + kvm_request_set(KVM_REQ_IOAPIC_EOI_EXIT, apic->vcpu); return; } @@ -1099,7 +1099,7 @@ static int apic_set_eoi(struct kvm_lapic *apic) kvm_hv_synic_send_eoi(apic->vcpu, vector); kvm_ioapic_send_eoi(apic, vector); - kvm_make_request(KVM_REQ_EVENT, apic->vcpu); + kvm_request_set(KVM_REQ_EVENT, apic->vcpu); return vector; } @@ -1114,7 +1114,7 @@ void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector) trace_kvm_eoi(apic, vector); kvm_ioapic_send_eoi(apic, vector); - kvm_make_request(KVM_REQ_EVENT, apic->vcpu); + kvm_request_set(KVM_REQ_EVENT, apic->vcpu); } EXPORT_SYMBOL_GPL(kvm_apic_set_eoi_accelerated); @@ -1179,7 +1179,7 @@ static void __report_tpr_access(struct kvm_lapic *apic, bool write) struct kvm_vcpu *vcpu = apic->vcpu; struct kvm_run *run = vcpu->run; - kvm_make_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu); + kvm_request_set(KVM_REQ_REPORT_TPR_ACCESS, vcpu); run->tpr_access.rip = kvm_rip_read(vcpu); run->tpr_access.is_write = write; } @@ -2217,7 +2217,7 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s) kvm_x86_ops->hwapic_isr_update(vcpu, apic_find_highest_isr(apic)); } - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); if (ioapic_in_kernel(vcpu->kvm)) kvm_rtc_eoi_tracking_restore_one(vcpu); diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 2fd7586aad4d..a588fce0be85 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -2075,7 +2075,7 @@ static void kvm_mmu_flush_or_zap(struct kvm_vcpu *vcpu, if (remote_flush) kvm_flush_remote_tlbs(vcpu->kvm); else if (local_flush) - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); } #ifdef CONFIG_KVM_MMU_AUDIT @@ -2281,11 +2281,11 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, break; WARN_ON(!list_empty(&invalid_list)); - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); } if (sp->unsync_children) - kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); + kvm_request_set(KVM_REQ_MMU_SYNC, vcpu); __clear_sp_write_flooding_count(sp); trace_kvm_mmu_get_page(sp, false); @@ -2769,7 +2769,7 @@ static bool mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep, unsigned pte_access, true, host_writable)) { if (write_fault) emulate = true; - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); } if (unlikely(is_mmio_spte(*sptep))) @@ -3303,7 +3303,7 @@ static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn) int ret = 0; if (!kvm_is_visible_gfn(vcpu->kvm, root_gfn)) { - kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, vcpu); ret = 1; } @@ -3707,7 +3707,7 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn, trace_kvm_try_async_get_page(gva, gfn); if (kvm_find_async_pf_gfn(vcpu, gfn)) { trace_kvm_async_pf_doublefault(gva, gfn); - kvm_make_request(KVM_REQ_APF_HALT, vcpu); + kvm_request_set(KVM_REQ_APF_HALT, vcpu); return true; } else if (kvm_arch_setup_async_pf(vcpu, gva, gfn)) return true; @@ -4765,7 +4765,7 @@ EXPORT_SYMBOL_GPL(kvm_mmu_page_fault); void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva) { vcpu->arch.mmu.invlpg(vcpu, gva); - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); ++vcpu->stat.invlpg; } EXPORT_SYMBOL_GPL(kvm_mmu_invlpg); diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c index 06ce377dcbc9..54fd50ad61c5 100644 --- a/arch/x86/kvm/pmu.c +++ b/arch/x86/kvm/pmu.c @@ -65,7 +65,7 @@ static void kvm_perf_overflow(struct perf_event *perf_event, if (!test_and_set_bit(pmc->idx, (unsigned long *)&pmu->reprogram_pmi)) { __set_bit(pmc->idx, (unsigned long *)&pmu->global_status); - kvm_make_request(KVM_REQ_PMU, pmc->vcpu); + kvm_request_set(KVM_REQ_PMU, pmc->vcpu); } } @@ -79,7 +79,7 @@ static void kvm_perf_overflow_intr(struct perf_event *perf_event, if (!test_and_set_bit(pmc->idx, (unsigned long *)&pmu->reprogram_pmi)) { __set_bit(pmc->idx, (unsigned long *)&pmu->global_status); - kvm_make_request(KVM_REQ_PMU, pmc->vcpu); + kvm_request_set(KVM_REQ_PMU, pmc->vcpu); /* * Inject PMI. If vcpu was in a guest mode during NMI PMI @@ -92,7 +92,7 @@ static void kvm_perf_overflow_intr(struct perf_event *perf_event, if (!kvm_is_in_guest()) irq_work_queue(&pmc_to_pmu(pmc)->irq_work); else - kvm_make_request(KVM_REQ_PMI, pmc->vcpu); + kvm_request_set(KVM_REQ_PMI, pmc->vcpu); } } diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index d1efe2c62b3f..57ea99d0ec30 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -2200,7 +2200,7 @@ static void svm_handle_mce(struct vcpu_svm *svm) */ pr_err("KVM: Guest triggered AMD Erratum 383\n"); - kvm_make_request(KVM_REQ_TRIPLE_FAULT, &svm->vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, &svm->vcpu); return; } @@ -3072,7 +3072,7 @@ static int stgi_interception(struct vcpu_svm *svm) svm->next_rip = kvm_rip_read(&svm->vcpu) + 3; skip_emulated_instruction(&svm->vcpu); - kvm_make_request(KVM_REQ_EVENT, &svm->vcpu); + kvm_request_set(KVM_REQ_EVENT, &svm->vcpu); enable_gif(svm); @@ -3220,7 +3220,7 @@ static int iret_interception(struct vcpu_svm *svm) clr_intercept(svm, INTERCEPT_IRET); svm->vcpu.arch.hflags |= HF_IRET_MASK; svm->nmi_iret_rip = kvm_rip_read(&svm->vcpu); - kvm_make_request(KVM_REQ_EVENT, &svm->vcpu); + kvm_request_set(KVM_REQ_EVENT, &svm->vcpu); return 1; } @@ -3659,7 +3659,7 @@ static int msr_interception(struct vcpu_svm *svm) static int interrupt_window_interception(struct vcpu_svm *svm) { - kvm_make_request(KVM_REQ_EVENT, &svm->vcpu); + kvm_request_set(KVM_REQ_EVENT, &svm->vcpu); svm_clear_vintr(svm); svm->vmcb->control.int_ctl &= ~V_IRQ_MASK; mark_dirty(svm->vmcb, VMCB_INTR); @@ -4693,7 +4693,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm) if ((svm->vcpu.arch.hflags & HF_IRET_MASK) && kvm_rip_read(&svm->vcpu) != svm->nmi_iret_rip) { svm->vcpu.arch.hflags &= ~(HF_NMI_MASK | HF_IRET_MASK); - kvm_make_request(KVM_REQ_EVENT, &svm->vcpu); + kvm_request_set(KVM_REQ_EVENT, &svm->vcpu); } svm->vcpu.arch.nmi_injected = false; @@ -4703,7 +4703,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm) if (!(exitintinfo & SVM_EXITINTINFO_VALID)) return; - kvm_make_request(KVM_REQ_EVENT, &svm->vcpu); + kvm_request_set(KVM_REQ_EVENT, &svm->vcpu); vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK; type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK; diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 9856b73a21ad..b183b4ac3ea5 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2288,7 +2288,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) struct desc_ptr *gdt = this_cpu_ptr(&host_gdt); unsigned long sysenter_esp; - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); /* * Linux uses per-cpu TSS and GDT, so set these when switching @@ -2468,7 +2468,7 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr, if (kvm_exception_is_soft(nr)) inc_eip = vcpu->arch.event_exit_inst_len; if (kvm_inject_realmode_interrupt(vcpu, nr, inc_eip) != EMULATE_DONE) - kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, vcpu); return; } @@ -4976,7 +4976,7 @@ static int vmx_deliver_nested_posted_interrupt(struct kvm_vcpu *vcpu, * we will accomplish it in the next vmentry. */ vmx->nested.pi_pending = true; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return 0; } return -1; @@ -5363,7 +5363,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) vmcs_write32(TPR_THRESHOLD, 0); } - kvm_make_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu); + kvm_request_set(KVM_REQ_APIC_PAGE_RELOAD, vcpu); if (kvm_vcpu_apicv_active(vcpu)) memset(&vmx->pi_desc, 0, sizeof(struct pi_desc)); @@ -5440,7 +5440,7 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu) if (vcpu->arch.interrupt.soft) inc_eip = vcpu->arch.event_exit_inst_len; if (kvm_inject_realmode_interrupt(vcpu, irq, inc_eip) != EMULATE_DONE) - kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, vcpu); return; } intr = irq | INTR_INFO_VALID_MASK; @@ -5477,7 +5477,7 @@ static void vmx_inject_nmi(struct kvm_vcpu *vcpu) if (vmx->rmode.vm86_active) { if (kvm_inject_realmode_interrupt(vcpu, NMI_VECTOR, 0) != EMULATE_DONE) - kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, vcpu); return; } @@ -6064,7 +6064,7 @@ static int handle_interrupt_window(struct kvm_vcpu *vcpu) vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL, CPU_BASED_VIRTUAL_INTR_PENDING); - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); ++vcpu->stat.irq_window_exits; return 1; @@ -6331,7 +6331,7 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu) vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL, CPU_BASED_VIRTUAL_NMI_PENDING); ++vcpu->stat.nmi_window_exits; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return 1; } @@ -6838,7 +6838,7 @@ static void nested_vmx_failValid(struct kvm_vcpu *vcpu, static void nested_vmx_abort(struct kvm_vcpu *vcpu, u32 indicator) { /* TODO: not to reset guest simply here. */ - kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, vcpu); pr_debug_ratelimited("kvm: nested vmx abort, indicator %d\n", indicator); } @@ -6848,7 +6848,7 @@ static enum hrtimer_restart vmx_preemption_timer_fn(struct hrtimer *timer) container_of(timer, struct vcpu_vmx, nested.preemption_timer); vmx->nested.preemption_timer_expired = true; - kvm_make_request(KVM_REQ_EVENT, &vmx->vcpu); + kvm_request_set(KVM_REQ_EVENT, &vmx->vcpu); kvm_vcpu_kick(&vmx->vcpu); return HRTIMER_NORESTART; @@ -7285,7 +7285,7 @@ static int handle_vmclear(struct kvm_vcpu *vcpu) * resulted in this case, so let's shut down before doing any * more damage: */ - kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, vcpu); return 1; } vmcs12 = kmap(page); @@ -7734,7 +7734,7 @@ static int handle_invept(struct kvm_vcpu *vcpu) */ case VMX_EPT_EXTENT_CONTEXT: kvm_mmu_sync_roots(vcpu); - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); nested_vmx_succeed(vcpu); break; default: @@ -8838,7 +8838,7 @@ static void __vmx_complete_interrupts(struct kvm_vcpu *vcpu, if (!idtv_info_valid) return; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); vector = idt_vectoring_info & VECTORING_INFO_VECTOR_MASK; type = idt_vectoring_info & VECTORING_INFO_TYPE_MASK; @@ -9136,7 +9136,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) * nested_run_pending, we need to re-enable this bit. */ if (vmx->nested.nested_run_pending) - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); vmx->nested.nested_run_pending = 0; @@ -11098,7 +11098,7 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason, * We are now running in L2, mmu_notifier will force to reload the * page's hpa for L2 vmcs. Need to reload it for L1 before entering L1. */ - kvm_make_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu); + kvm_request_set(KVM_REQ_APIC_PAGE_RELOAD, vcpu); /* * Exiting from L2 to L1, we're now back to L1 which thinks it just diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c48404017e4f..da125323682a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -388,7 +388,7 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu, u32 prev_nr; int class1, class2; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); if (!vcpu->arch.exception.pending) { queue: @@ -406,7 +406,7 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu, prev_nr = vcpu->arch.exception.nr; if (prev_nr == DF_VECTOR) { /* triple fault -> shutdown */ - kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, vcpu); return; } class1 = exception_class(prev_nr); @@ -469,7 +469,7 @@ static bool kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fau void kvm_inject_nmi(struct kvm_vcpu *vcpu) { atomic_inc(&vcpu->arch.nmi_queued); - kvm_make_request(KVM_REQ_NMI, vcpu); + kvm_request_set(KVM_REQ_NMI, vcpu); } EXPORT_SYMBOL_GPL(kvm_inject_nmi); @@ -805,7 +805,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) if (cr3 == kvm_read_cr3(vcpu) && !pdptrs_changed(vcpu)) { kvm_mmu_sync_roots(vcpu); - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); + kvm_request_set(KVM_REQ_TLB_FLUSH, vcpu); return 0; } @@ -1179,7 +1179,7 @@ void kvm_set_pending_timer(struct kvm_vcpu *vcpu) * vcpu_enter_guest. This function is only called from * the physical CPU that is running vcpu. */ - kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu); + kvm_request_set(KVM_REQ_PENDING_TIMER, vcpu); } static void kvm_write_wall_clock(struct kvm *kvm, gpa_t wall_clock) @@ -1375,7 +1375,7 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu) */ if (ka->use_master_clock || (gtod->clock.vclock_mode == VCLOCK_TSC && vcpus_matched)) - kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); trace_kvm_track_tsc(vcpu->vcpu_id, ka->nr_vcpus_matched_tsc, atomic_read(&vcpu->kvm->online_vcpus), @@ -1763,7 +1763,7 @@ static void kvm_gen_update_masterclock(struct kvm *kvm) pvclock_update_vm_gtod_copy(kvm); kvm_for_each_vcpu(i, vcpu, kvm) - kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, vcpu); /* guest entries allowed */ kvm_for_each_vcpu(i, vcpu, kvm) @@ -1890,7 +1890,7 @@ static int kvm_guest_time_update(struct kvm_vcpu *v) tgt_tsc_khz = __this_cpu_read(cpu_tsc_khz); if (unlikely(tgt_tsc_khz == 0)) { local_irq_restore(flags); - kvm_make_request(KVM_REQ_CLOCK_UPDATE, v); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, v); return 1; } if (!use_master_clock) { @@ -1976,7 +1976,7 @@ static void kvmclock_update_fn(struct work_struct *work) struct kvm_vcpu *vcpu; kvm_for_each_vcpu(i, vcpu, kvm) { - kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, vcpu); kvm_vcpu_kick(vcpu); } } @@ -1985,7 +1985,7 @@ static void kvm_gen_kvmclock_update(struct kvm_vcpu *v) { struct kvm *kvm = v->kvm; - kvm_make_request(KVM_REQ_CLOCK_UPDATE, v); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, v); schedule_delayed_work(&kvm->arch.kvmclock_update_work, KVMCLOCK_UPDATE_DELAY); } @@ -2229,13 +2229,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) if (ka->boot_vcpu_runs_old_kvmclock != tmp) set_bit(KVM_REQ_MASTERCLOCK_UPDATE, - &vcpu->requests); + &vcpu->requests); ka->boot_vcpu_runs_old_kvmclock = tmp; } vcpu->arch.time = data; - kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu); /* we verify if the enable bit is set... */ if (!(data & 1)) @@ -2272,7 +2272,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) if (!(data & KVM_MSR_ENABLED)) break; - kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); + kvm_request_set(KVM_REQ_STEAL_UPDATE, vcpu); break; case MSR_KVM_PV_EOI_EN: @@ -2836,7 +2836,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) if (unlikely(vcpu->arch.tsc_offset_adjustment)) { adjust_tsc_offset_host(vcpu, vcpu->arch.tsc_offset_adjustment); vcpu->arch.tsc_offset_adjustment = 0; - kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, vcpu); } if (unlikely(vcpu->cpu != cpu) || check_tsc_unstable()) { @@ -2860,13 +2860,13 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) * kvmclock on vcpu->cpu migration */ if (!vcpu->kvm->arch.use_master_clock || vcpu->cpu == -1) - kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu); if (vcpu->cpu != cpu) kvm_migrate_timers(vcpu); vcpu->cpu = cpu; } - kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); + kvm_request_set(KVM_REQ_STEAL_UPDATE, vcpu); } static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu) @@ -2957,7 +2957,7 @@ static int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu, if (!irqchip_in_kernel(vcpu->kvm)) { kvm_queue_interrupt(vcpu, irq->irq, false); - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return 0; } @@ -2972,7 +2972,7 @@ static int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu, return -EEXIST; vcpu->arch.pending_external_vector = irq->irq; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return 0; } @@ -2985,7 +2985,7 @@ static int kvm_vcpu_ioctl_nmi(struct kvm_vcpu *vcpu) static int kvm_vcpu_ioctl_smi(struct kvm_vcpu *vcpu) { - kvm_make_request(KVM_REQ_SMI, vcpu); + kvm_request_set(KVM_REQ_SMI, vcpu); return 0; } @@ -3051,7 +3051,7 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu, if (mce->status & MCI_STATUS_UC) { if ((vcpu->arch.mcg_status & MCG_STATUS_MCIP) || !kvm_read_cr4_bits(vcpu, X86_CR4_MCE)) { - kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + kvm_request_set(KVM_REQ_TRIPLE_FAULT, vcpu); return 0; } if (banks[1] & MCI_STATUS_VAL) @@ -3168,7 +3168,7 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, } } - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return 0; } @@ -3370,7 +3370,7 @@ static int kvm_set_guest_paused(struct kvm_vcpu *vcpu) if (!vcpu->arch.pv_time_enabled) return -EINVAL; vcpu->arch.pvclock_set_guest_stopped_request = true; - kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, vcpu); return 0; } @@ -5276,7 +5276,7 @@ static void toggle_interruptibility(struct kvm_vcpu *vcpu, u32 mask) if (unlikely(int_shadow || mask)) { kvm_x86_ops->set_interrupt_shadow(vcpu, mask); if (!mask) - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); } } @@ -5487,7 +5487,7 @@ static void kvm_smm_changed(struct kvm_vcpu *vcpu) trace_kvm_enter_smm(vcpu->vcpu_id, vcpu->arch.smbase, false); /* Process a latched INIT or SMI, if any. */ - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); } kvm_mmu_reset_context(vcpu); @@ -5731,7 +5731,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, * because POPF has no interrupt shadow. */ if (unlikely((ctxt->eflags & ~rflags) & X86_EFLAGS_IF)) - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); } else vcpu->arch.emulate_regs_need_sync_to_vcpu = true; @@ -5872,7 +5872,7 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va kvm_for_each_vcpu(i, vcpu, kvm) { if (vcpu->cpu != freq->cpu) continue; - kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, vcpu); if (vcpu->cpu != smp_processor_id()) send_ipi = 1; } @@ -6015,7 +6015,7 @@ static void pvclock_gtod_update_fn(struct work_struct *work) spin_lock(&kvm_lock); list_for_each_entry(kvm, &vm_list, vm_list) kvm_for_each_vcpu(i, vcpu, kvm) - kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); atomic_set(&kvm_guest_has_master_clock, 0); spin_unlock(&kvm_lock); } @@ -6405,7 +6405,7 @@ static void process_nmi(struct kvm_vcpu *vcpu) vcpu->arch.nmi_pending += atomic_xchg(&vcpu->arch.nmi_queued, 0); vcpu->arch.nmi_pending = min(vcpu->arch.nmi_pending, limit); - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); } #define put_smstate(type, buf, offset, val) \ @@ -6640,7 +6640,7 @@ static void enter_smm(struct kvm_vcpu *vcpu) static void process_smi(struct kvm_vcpu *vcpu) { vcpu->arch.smi_pending = true; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); } void kvm_make_scan_ioapic_request(struct kvm *kvm) @@ -6724,50 +6724,50 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) bool req_immediate_exit = false; if (vcpu->requests) { - if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_MMU_RELOAD, vcpu)) kvm_mmu_unload(vcpu); - if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_MIGRATE_TIMER, vcpu)) __kvm_migrate_timers(vcpu); - if (kvm_check_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_MASTERCLOCK_UPDATE, vcpu)) kvm_gen_update_masterclock(vcpu->kvm); - if (kvm_check_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu)) kvm_gen_kvmclock_update(vcpu); - if (kvm_check_request(KVM_REQ_CLOCK_UPDATE, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_CLOCK_UPDATE, vcpu)) { r = kvm_guest_time_update(vcpu); if (unlikely(r)) goto out; } - if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_MMU_SYNC, vcpu)) kvm_mmu_sync_roots(vcpu); - if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_TLB_FLUSH, vcpu)) kvm_vcpu_flush_tlb(vcpu); - if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) { vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS; r = 0; goto out; } - if (kvm_check_request(KVM_REQ_TRIPLE_FAULT, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_TRIPLE_FAULT, vcpu)) { vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN; r = 0; goto out; } - if (kvm_check_request(KVM_REQ_APF_HALT, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_APF_HALT, vcpu)) { /* Page is swapped out. Do synthetic halt */ vcpu->arch.apf.halted = true; r = 1; goto out; } - if (kvm_check_request(KVM_REQ_STEAL_UPDATE, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_STEAL_UPDATE, vcpu)) record_steal_time(vcpu); - if (kvm_check_request(KVM_REQ_SMI, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_SMI, vcpu)) process_smi(vcpu); - if (kvm_check_request(KVM_REQ_NMI, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_NMI, vcpu)) process_nmi(vcpu); - if (kvm_check_request(KVM_REQ_PMU, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_PMU, vcpu)) kvm_pmu_handle_event(vcpu); - if (kvm_check_request(KVM_REQ_PMI, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_PMI, vcpu)) kvm_pmu_deliver_pmi(vcpu); - if (kvm_check_request(KVM_REQ_IOAPIC_EOI_EXIT, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_IOAPIC_EOI_EXIT, vcpu)) { BUG_ON(vcpu->arch.pending_ioapic_eoi > 255); if (test_bit(vcpu->arch.pending_ioapic_eoi, vcpu->arch.ioapic_handled_vectors)) { @@ -6778,23 +6778,23 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) goto out; } } - if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_SCAN_IOAPIC, vcpu)) vcpu_scan_ioapic(vcpu); - if (kvm_check_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_APIC_PAGE_RELOAD, vcpu)) kvm_vcpu_reload_apic_access_page(vcpu); - if (kvm_check_request(KVM_REQ_HV_CRASH, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_HV_CRASH, vcpu)) { vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT; vcpu->run->system_event.type = KVM_SYSTEM_EVENT_CRASH; r = 0; goto out; } - if (kvm_check_request(KVM_REQ_HV_RESET, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_HV_RESET, vcpu)) { vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT; vcpu->run->system_event.type = KVM_SYSTEM_EVENT_RESET; r = 0; goto out; } - if (kvm_check_request(KVM_REQ_HV_EXIT, vcpu)) { + if (kvm_request_test_and_clear(KVM_REQ_HV_EXIT, vcpu)) { vcpu->run->exit_reason = KVM_EXIT_HYPERV; vcpu->run->hyperv = vcpu->arch.hyperv.exit; r = 0; @@ -6806,11 +6806,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) * KVM_REQ_CLOCK_UPDATE, because Hyper-V SynIC timers * depend on the guest clock being up-to-date */ - if (kvm_check_request(KVM_REQ_HV_STIMER, vcpu)) + if (kvm_request_test_and_clear(KVM_REQ_HV_STIMER, vcpu)) kvm_hv_process_stimers(vcpu); } - if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) { + if (kvm_request_test_and_clear(KVM_REQ_EVENT, vcpu) || req_int_win) { ++vcpu->stat.req_event; kvm_apic_accept_events(vcpu); if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) { @@ -6901,7 +6901,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) kvm_load_guest_xcr0(vcpu); if (req_immediate_exit) { - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); smp_send_reschedule(vcpu->cpu); } @@ -6973,7 +6973,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) } if (unlikely(vcpu->arch.tsc_always_catchup)) - kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, vcpu); if (vcpu->arch.apic_attention) kvm_lapic_sync_from_vapic(vcpu); @@ -7000,7 +7000,7 @@ static inline int vcpu_block(struct kvm *kvm, struct kvm_vcpu *vcpu) if (kvm_x86_ops->post_block) kvm_x86_ops->post_block(vcpu); - if (!kvm_check_request(KVM_REQ_UNHALT, vcpu)) + if (!kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) return 1; } @@ -7278,7 +7278,7 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) vcpu->arch.exception.pending = false; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return 0; } @@ -7357,7 +7357,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu, set_bit(KVM_APIC_SIPI, &vcpu->arch.apic->pending_events); } else vcpu->arch.mp_state = mp_state->mp_state; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return 0; } @@ -7377,7 +7377,7 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index, kvm_rip_write(vcpu, ctxt->eip); kvm_set_rflags(vcpu, ctxt->eflags); - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return EMULATE_DONE; } EXPORT_SYMBOL_GPL(kvm_task_switch); @@ -7458,7 +7458,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, !is_protmode(vcpu)) vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); return 0; } @@ -7708,7 +7708,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) vcpu->arch.cr2 = 0; - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); vcpu->arch.apf.msr_val = 0; vcpu->arch.st.msr_val = 0; @@ -7761,7 +7761,7 @@ int kvm_arch_hardware_enable(void) list_for_each_entry(kvm, &vm_list, vm_list) { kvm_for_each_vcpu(i, vcpu, kvm) { if (!stable && vcpu->cpu == smp_processor_id()) - kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_CLOCK_UPDATE, vcpu); if (stable && vcpu->arch.last_host_tsc > local_tsc) { backwards_tsc = true; if (vcpu->arch.last_host_tsc > max_tsc) @@ -7815,7 +7815,7 @@ int kvm_arch_hardware_enable(void) kvm_for_each_vcpu(i, vcpu, kvm) { vcpu->arch.tsc_offset_adjustment += delta_cyc; vcpu->arch.last_host_tsc = local_tsc; - kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); + kvm_request_set(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); } /* @@ -8446,7 +8446,7 @@ static void __kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) void kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) { __kvm_set_rflags(vcpu, rflags); - kvm_make_request(KVM_REQ_EVENT, vcpu); + kvm_request_set(KVM_REQ_EVENT, vcpu); } EXPORT_SYMBOL_GPL(kvm_set_rflags); @@ -8547,7 +8547,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, if (!(vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED) || (vcpu->arch.apf.send_user_only && kvm_x86_ops->get_cpl(vcpu) == 0)) - kvm_make_request(KVM_REQ_APF_HALT, vcpu); + kvm_request_set(KVM_REQ_APF_HALT, vcpu); else if (!apf_put_user(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT)) { fault.vector = PF_VECTOR; fault.error_code_valid = true; diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 8d69d5150748..21f91de3098b 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1084,24 +1084,42 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) #endif /* CONFIG_HAVE_KVM_EVENTFD */ -static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu) +/* + * An API for setting KVM requests. + * The general API design is inspired by bit_* API. + * + * A request can be set either to itself or to a remote VCPU. If the request + * is set to a remote VCPU, then the VCPU needs to be notified, which is + * usually done with kvm_vcpu_kick(). + * The request can also mean that some data is ready, so a remote requests + * needs a smp_wmb(). i.e. there are three types of requests: + * 1) local request + * 2) remote request with no data (= kick) + * 3) remote request with data (= kick + mb) + * + * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but + * forces smp_wmb() for all requests. + */ +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) { /* - * Ensure the rest of the request is published to kvm_check_request's - * caller. Paired with the smp_mb__after_atomic in kvm_check_request. + * Ensure the rest of the request is published to + * kvm_request_test_and_clear's caller. + * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. */ smp_wmb(); set_bit(req, &vcpu->requests); } -static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu) +static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu) { if (test_bit(req, &vcpu->requests)) { clear_bit(req, &vcpu->requests); /* - * Ensure the rest of the request is visible to kvm_check_request's - * caller. Paired with the smp_wmb in kvm_make_request. + * Ensure the rest of the request is visible to + * kvm_request_test_and_clear's caller. + * Paired with the smp_wmb in kvm_request_set. */ smp_mb__after_atomic(); return true; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 263a80513ad9..934f135c0d23 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -178,7 +178,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) me = get_cpu(); kvm_for_each_vcpu(i, vcpu, kvm) { - kvm_make_request(req, vcpu); + kvm_request_set(req, vcpu); cpu = vcpu->cpu; /* Set ->requests bit before we read ->mode. */ @@ -2140,7 +2140,7 @@ static void shrink_halt_poll_ns(struct kvm_vcpu *vcpu) static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu) { if (kvm_arch_vcpu_runnable(vcpu)) { - kvm_make_request(KVM_REQ_UNHALT, vcpu); + kvm_request_set(KVM_REQ_UNHALT, vcpu); return -EINTR; } if (kvm_cpu_has_pending_timer(vcpu)) -- 2.11.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/5] KVM: change API for requests to match bit operations 2017-02-16 16:04 ` [PATCH 1/5] KVM: change API for requests to match bit operations Radim Krčmář @ 2017-02-17 9:30 ` Cornelia Huck 2017-02-17 9:49 ` Andrew Jones 2017-02-17 15:01 ` Radim Krčmář 0 siblings, 2 replies; 31+ messages in thread From: Cornelia Huck @ 2017-02-17 9:30 UTC (permalink / raw) To: Radim Krčmář Cc: linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, James Hogan, Paul Mackerras, Christoffer Dall On Thu, 16 Feb 2017 17:04:45 +0100 Radim Krčmář <rkrcmar@redhat.com> wrote: > kvm_make_request was a wrapper that added barriers to bit_set and > kvm_check_request did the same for bit_test and bit_check, but the name > was not very obvious and we were also lacking operations that cover > bit_test and bit_clear, which resulted in an inconsistent use. > > The renaming: > kvm_request_set <- kvm_make_request > kvm_request_test_and_clear <- kvm_check_request > > Automated with coccinelle script: > @@ > expression VCPU, REQ; > @@ > -kvm_make_request(REQ, VCPU) > +kvm_request_set(REQ, VCPU) > > @@ > expression VCPU, REQ; > @@ > -kvm_check_request(REQ, VCPU) > +kvm_request_test_and_clear(REQ, VCPU) Forgot your s-o-b? > --- > arch/mips/kvm/emulate.c | 2 +- > arch/mips/kvm/trap_emul.c | 2 +- > arch/powerpc/kvm/book3s_pr.c | 2 +- > arch/powerpc/kvm/booke.c | 16 +++--- > arch/powerpc/kvm/powerpc.c | 2 +- > arch/s390/kvm/kvm-s390.c | 22 ++++---- > arch/s390/kvm/kvm-s390.h | 4 +- > arch/s390/kvm/priv.c | 4 +- > arch/x86/kvm/hyperv.c | 14 ++--- > arch/x86/kvm/i8259.c | 2 +- > arch/x86/kvm/lapic.c | 22 ++++---- > arch/x86/kvm/mmu.c | 14 ++--- > arch/x86/kvm/pmu.c | 6 +- > arch/x86/kvm/svm.c | 12 ++-- > arch/x86/kvm/vmx.c | 30 +++++----- > arch/x86/kvm/x86.c | 128 +++++++++++++++++++++---------------------- > include/linux/kvm_host.h | 30 ++++++++-- > virt/kvm/kvm_main.c | 4 +- > 18 files changed, 167 insertions(+), 149 deletions(-) (...lots of coccinelle changes...) > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 8d69d5150748..21f91de3098b 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1084,24 +1084,42 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) > > #endif /* CONFIG_HAVE_KVM_EVENTFD */ > > -static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu) > +/* > + * An API for setting KVM requests. > + * The general API design is inspired by bit_* API. > + * > + * A request can be set either to itself or to a remote VCPU. If the request > + * is set to a remote VCPU, then the VCPU needs to be notified, which is > + * usually done with kvm_vcpu_kick(). > + * The request can also mean that some data is ready, so a remote requests > + * needs a smp_wmb(). i.e. there are three types of requests: > + * 1) local request > + * 2) remote request with no data (= kick) > + * 3) remote request with data (= kick + mb) > + * > + * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but > + * forces smp_wmb() for all requests. > + */ > +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) Should we make req unsigned long as well, so that it matches the bit api even more? > { > /* > - * Ensure the rest of the request is published to kvm_check_request's > - * caller. Paired with the smp_mb__after_atomic in kvm_check_request. > + * Ensure the rest of the request is published to > + * kvm_request_test_and_clear's caller. > + * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. > */ > smp_wmb(); > set_bit(req, &vcpu->requests); > } > > -static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu) > +static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu) > { > if (test_bit(req, &vcpu->requests)) { > clear_bit(req, &vcpu->requests); > > /* > - * Ensure the rest of the request is visible to kvm_check_request's > - * caller. Paired with the smp_wmb in kvm_make_request. > + * Ensure the rest of the request is visible to > + * kvm_request_test_and_clear's caller. > + * Paired with the smp_wmb in kvm_request_set. > */ > smp_mb__after_atomic(); > return true; ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/5] KVM: change API for requests to match bit operations 2017-02-17 9:30 ` Cornelia Huck @ 2017-02-17 9:49 ` Andrew Jones 2017-02-17 9:52 ` Cornelia Huck 2017-02-17 15:01 ` Radim Krčmář 1 sibling, 1 reply; 31+ messages in thread From: Andrew Jones @ 2017-02-17 9:49 UTC (permalink / raw) To: Cornelia Huck Cc: Radim Krčmář, linux-kernel, kvm, Paolo Bonzini, Marc Zyngier, Christian Borntraeger, James Hogan, Paul Mackerras, Christoffer Dall On Fri, Feb 17, 2017 at 10:30:14AM +0100, Cornelia Huck wrote: > On Thu, 16 Feb 2017 17:04:45 +0100 > Radim Krčmář <rkrcmar@redhat.com> wrote: > > +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) > > Should we make req unsigned long as well, so that it matches the bit > api even more? The bitops API is inconsistent among architectures; some are int, some are unsigned int, some are unsigned long, and x86 is long. If we want to be consistent with something, then, IMO, we should be consistent with asm-generic/bitops, which is int, but actually unsigned makes more sense to me... Thanks, drew ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/5] KVM: change API for requests to match bit operations 2017-02-17 9:49 ` Andrew Jones @ 2017-02-17 9:52 ` Cornelia Huck 0 siblings, 0 replies; 31+ messages in thread From: Cornelia Huck @ 2017-02-17 9:52 UTC (permalink / raw) To: Andrew Jones Cc: Radim Krčmář, linux-kernel, kvm, Paolo Bonzini, Marc Zyngier, Christian Borntraeger, James Hogan, Paul Mackerras, Christoffer Dall On Fri, 17 Feb 2017 10:49:35 +0100 Andrew Jones <drjones@redhat.com> wrote: > On Fri, Feb 17, 2017 at 10:30:14AM +0100, Cornelia Huck wrote: > > On Thu, 16 Feb 2017 17:04:45 +0100 > > Radim Krčmář <rkrcmar@redhat.com> wrote: > > > +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) > > > > Should we make req unsigned long as well, so that it matches the bit > > api even more? > > The bitops API is inconsistent among architectures; some are int, some > are unsigned int, some are unsigned long, and x86 is long. If we want > to be consistent with something, then, IMO, we should be consistent with > asm-generic/bitops, which is int, but actually unsigned makes more sense > to me... Inconsistent interfaces are great :/ Having (any) unsigned value makes the most sense to me as well. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/5] KVM: change API for requests to match bit operations 2017-02-17 9:30 ` Cornelia Huck 2017-02-17 9:49 ` Andrew Jones @ 2017-02-17 15:01 ` Radim Krčmář 1 sibling, 0 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-17 15:01 UTC (permalink / raw) To: Cornelia Huck Cc: linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, James Hogan, Paul Mackerras, Christoffer Dall 2017-02-17 10:30+0100, Cornelia Huck: > On Thu, 16 Feb 2017 17:04:45 +0100 > Radim Krčmář <rkrcmar@redhat.com> wrote: > >> kvm_make_request was a wrapper that added barriers to bit_set and >> kvm_check_request did the same for bit_test and bit_check, but the name >> was not very obvious and we were also lacking operations that cover >> bit_test and bit_clear, which resulted in an inconsistent use. >> >> The renaming: >> kvm_request_set <- kvm_make_request >> kvm_request_test_and_clear <- kvm_check_request >> >> Automated with coccinelle script: >> @@ >> expression VCPU, REQ; >> @@ >> -kvm_make_request(REQ, VCPU) >> +kvm_request_set(REQ, VCPU) >> >> @@ >> expression VCPU, REQ; >> @@ >> -kvm_check_request(REQ, VCPU) >> +kvm_request_test_and_clear(REQ, VCPU) > > Forgot your s-o-b? Oops, thanks. >> +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) > > Should we make req unsigned long as well, so that it matches the bit > api even more? >From the discussion that followed, I'll keep unsigned. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/5] KVM: add KVM request variants without barrier 2017-02-16 16:04 [PATCH 0/5] KVM: rename and extend vcpu->requests API Radim Krčmář 2017-02-16 16:04 ` [PATCH 1/5] KVM: change API for requests to match bit operations Radim Krčmář @ 2017-02-16 16:04 ` Radim Krčmář 2017-02-23 10:57 ` Paolo Bonzini 2017-02-16 16:04 ` [PATCH 3/5] KVM: optimize kvm_make_all_cpus_request Radim Krčmář ` (2 subsequent siblings) 4 siblings, 1 reply; 31+ messages in thread From: Radim Krčmář @ 2017-02-16 16:04 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall The leading underscores mean that the call is just a bitop wrapper. Switch all users of open-coded set/check/test to kvm_request ones. Automated by coccinelle script: @@ expression VCPU, REQ; @@ -set_bit(REQ, &VCPU->requests) +__kvm_request_set(REQ, VCPU) @@ expression VCPU, REQ; @@ -clear_bit(REQ, &VCPU->requests) +__kvm_request_clear(REQ, VCPU) @@ expression VCPU, REQ; @@ -test_bit(REQ, &VCPU->requests) +__kvm_request_test(REQ, VCPU) Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- arch/mips/kvm/emulate.c | 2 +- arch/powerpc/kvm/book3s_pr.c | 2 +- arch/powerpc/kvm/book3s_pr_papr.c | 2 +- arch/powerpc/kvm/booke.c | 4 ++-- arch/powerpc/kvm/powerpc.c | 2 +- arch/s390/kvm/kvm-s390.c | 2 +- arch/x86/kvm/vmx.c | 2 +- arch/x86/kvm/x86.c | 14 +++++++------- include/linux/kvm_host.h | 22 +++++++++++++++++++--- 9 files changed, 34 insertions(+), 18 deletions(-) diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c index ee4af898bcf6..552ae2b5e911 100644 --- a/arch/mips/kvm/emulate.c +++ b/arch/mips/kvm/emulate.c @@ -865,7 +865,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu) * check if any I/O interrupts are pending. */ if (kvm_request_test_and_clear(KVM_REQ_UNHALT, vcpu)) { - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN; } } diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c index 7af5154e848b..f5894d27a8a9 100644 --- a/arch/powerpc/kvm/book3s_pr.c +++ b/arch/powerpc/kvm/book3s_pr.c @@ -349,7 +349,7 @@ static void kvmppc_set_msr_pr(struct kvm_vcpu *vcpu, u64 msr) if (msr & MSR_POW) { if (!vcpu->arch.pending_exceptions) { kvm_vcpu_block(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); vcpu->stat.halt_wakeup++; /* Unset POW bit after we woke up */ diff --git a/arch/powerpc/kvm/book3s_pr_papr.c b/arch/powerpc/kvm/book3s_pr_papr.c index f102616febc7..86847220811a 100644 --- a/arch/powerpc/kvm/book3s_pr_papr.c +++ b/arch/powerpc/kvm/book3s_pr_papr.c @@ -344,7 +344,7 @@ int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd) case H_CEDE: kvmppc_set_msr_fast(vcpu, kvmppc_get_msr(vcpu) | MSR_EE); kvm_vcpu_block(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); vcpu->stat.halt_wakeup++; return EMULATE_DONE; case H_LOGICAL_CI_LOAD: diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 806caaf60e10..e9098af0ab2a 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -579,7 +579,7 @@ static void arm_next_watchdog(struct kvm_vcpu *vcpu) * userspace, so clear the KVM_REQ_WATCHDOG request. */ if ((vcpu->arch.tsr & (TSR_ENW | TSR_WIS)) != (TSR_ENW | TSR_WIS)) - clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests); + __kvm_request_clear(KVM_REQ_WATCHDOG, vcpu); spin_lock_irqsave(&vcpu->arch.wdt_lock, flags); nr_jiffies = watchdog_next_timeout(vcpu); @@ -690,7 +690,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu) if (vcpu->arch.shared->msr & MSR_WE) { local_irq_enable(); kvm_vcpu_block(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); hard_irq_disable(); kvmppc_set_exit_type(vcpu, EMULATED_MTMSRWE_EXITS); diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 19e89419a843..f570ca9cd8c6 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -231,7 +231,7 @@ int kvmppc_kvm_pv(struct kvm_vcpu *vcpu) case EV_HCALL_TOKEN(EV_IDLE): r = EV_SUCCESS; kvm_vcpu_block(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); break; default: r = EV_UNIMPLEMENTED; diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index db3c742a5dc9..0fa77e6fdfaf 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2443,7 +2443,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) } /* nothing to do, just clear the request */ - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); return 0; } diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index b183b4ac3ea5..7ab638f2189b 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -6352,7 +6352,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) if (intr_window_requested && vmx_interrupt_allowed(vcpu)) return handle_interrupt_window(&vmx->vcpu); - if (test_bit(KVM_REQ_EVENT, &vcpu->requests)) + if (__kvm_request_test(KVM_REQ_EVENT, vcpu)) return 1; err = emulate_instruction(vcpu, EMULTYPE_NO_REEXECUTE); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index da125323682a..6e8c62c29a5a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1767,7 +1767,7 @@ static void kvm_gen_update_masterclock(struct kvm *kvm) /* guest entries allowed */ kvm_for_each_vcpu(i, vcpu, kvm) - clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests); + __kvm_request_clear(KVM_REQ_MCLOCK_INPROGRESS, vcpu); spin_unlock(&ka->pvclock_gtod_sync_lock); #endif @@ -2228,8 +2228,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) bool tmp = (msr == MSR_KVM_SYSTEM_TIME); if (ka->boot_vcpu_runs_old_kvmclock != tmp) - set_bit(KVM_REQ_MASTERCLOCK_UPDATE, - &vcpu->requests); + __kvm_request_set(KVM_REQ_MASTERCLOCK_UPDATE, + vcpu); ka->boot_vcpu_runs_old_kvmclock = tmp; } @@ -2816,7 +2816,7 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu) static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu) { - set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests); + __kvm_request_set(KVM_REQ_MIGRATE_TIMER, vcpu); } void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) @@ -7048,7 +7048,7 @@ static int vcpu_run(struct kvm_vcpu *vcpu) if (r <= 0) break; - clear_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests); + __kvm_request_clear(KVM_REQ_PENDING_TIMER, vcpu); if (kvm_cpu_has_pending_timer(vcpu)) kvm_inject_pending_timer_irqs(vcpu); @@ -7176,7 +7176,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) { kvm_vcpu_block(vcpu); kvm_apic_accept_events(vcpu); - clear_bit(KVM_REQ_UNHALT, &vcpu->requests); + __kvm_request_clear(KVM_REQ_UNHALT, vcpu); r = -EAGAIN; goto out; } @@ -8381,7 +8381,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu) if (atomic_read(&vcpu->arch.nmi_queued)) return true; - if (test_bit(KVM_REQ_SMI, &vcpu->requests)) + if (__kvm_request_test(KVM_REQ_SMI, vcpu)) return true; if (kvm_arch_interrupt_allowed(vcpu) && diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 21f91de3098b..d899473859d3 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1100,6 +1100,12 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but * forces smp_wmb() for all requests. */ + +static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) +{ + set_bit(req, &vcpu->requests); +} + static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) { /* @@ -1108,13 +1114,23 @@ static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. */ smp_wmb(); - set_bit(req, &vcpu->requests); + __kvm_request_set(req, vcpu); +} + +static inline bool __kvm_request_test(unsigned req, struct kvm_vcpu *vcpu) +{ + return test_bit(req, &vcpu->requests); +} + +static inline void __kvm_request_clear(unsigned req, struct kvm_vcpu *vcpu) +{ + test_bit(req, &vcpu->requests); } static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu) { - if (test_bit(req, &vcpu->requests)) { - clear_bit(req, &vcpu->requests); + if (__kvm_request_test(req, vcpu)) { + __kvm_request_clear(req, vcpu); /* * Ensure the rest of the request is visible to -- 2.11.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/5] KVM: add KVM request variants without barrier 2017-02-16 16:04 ` [PATCH 2/5] KVM: add KVM request variants without barrier Radim Krčmář @ 2017-02-23 10:57 ` Paolo Bonzini 2017-02-23 15:50 ` Radim Krčmář 0 siblings, 1 reply; 31+ messages in thread From: Paolo Bonzini @ 2017-02-23 10:57 UTC (permalink / raw) To: Radim Krčmář, linux-kernel, kvm Cc: Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On 16/02/2017 17:04, Radim Krčmář wrote: > + > +static inline void __kvm_request_clear(unsigned req, struct kvm_vcpu *vcpu) > +{ > + test_bit(req, &vcpu->requests); > } Are you sure? :) Paolo > static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu) > { > - if (test_bit(req, &vcpu->requests)) { > - clear_bit(req, &vcpu->requests); > + if (__kvm_request_test(req, vcpu)) { > + __kvm_request_clear(req, vcpu); > > /* > * Ensure the rest of the request is visible to > -- ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/5] KVM: add KVM request variants without barrier 2017-02-23 10:57 ` Paolo Bonzini @ 2017-02-23 15:50 ` Radim Krčmář 0 siblings, 0 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-23 15:50 UTC (permalink / raw) To: Paolo Bonzini Cc: linux-kernel, kvm, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall 2017-02-23 11:57+0100, Paolo Bonzini: > On 16/02/2017 17:04, Radim Krčmář wrote: >> + >> +static inline void __kvm_request_clear(unsigned req, struct kvm_vcpu *vcpu) >> +{ >> + test_bit(req, &vcpu->requests); >> } > > Are you sure? :) No, as always. :) ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/5] KVM: optimize kvm_make_all_cpus_request 2017-02-16 16:04 [PATCH 0/5] KVM: rename and extend vcpu->requests API Radim Krčmář 2017-02-16 16:04 ` [PATCH 1/5] KVM: change API for requests to match bit operations Radim Krčmář 2017-02-16 16:04 ` [PATCH 2/5] KVM: add KVM request variants without barrier Radim Krčmář @ 2017-02-16 16:04 ` Radim Krčmář 2017-02-16 16:04 ` [PATCH 4/5] KVM: add __kvm_request_needs_mb Radim Krčmář 2017-02-16 16:04 ` [PATCH 5/5] KVM: add kvm_request_pending Radim Krčmář 4 siblings, 0 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-16 16:04 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall Use __kvm_request_set to avoid repeated use of wmb(). kvm_make_all_cpus_request is also a candidate for renaming. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- virt/kvm/kvm_main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 934f135c0d23..2250920ec965 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -177,8 +177,12 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) zalloc_cpumask_var(&cpus, GFP_ATOMIC); me = get_cpu(); + + /* Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. */ + smp_wmb(); + kvm_for_each_vcpu(i, vcpu, kvm) { - kvm_request_set(req, vcpu); + __kvm_request_set(req, vcpu); cpu = vcpu->cpu; /* Set ->requests bit before we read ->mode. */ -- 2.11.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-16 16:04 [PATCH 0/5] KVM: rename and extend vcpu->requests API Radim Krčmář ` (2 preceding siblings ...) 2017-02-16 16:04 ` [PATCH 3/5] KVM: optimize kvm_make_all_cpus_request Radim Krčmář @ 2017-02-16 16:04 ` Radim Krčmář 2017-02-16 19:49 ` David Hildenbrand 2017-02-23 11:01 ` Paolo Bonzini 2017-02-16 16:04 ` [PATCH 5/5] KVM: add kvm_request_pending Radim Krčmář 4 siblings, 2 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-16 16:04 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall A macro to optimize requests that do not need a memory barrier because they have no dependencies. An architecture can implement a function that says which requests do not need memory barriers when handling them. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- include/linux/kvm_host.h | 41 +++++++++++++++++++++++++++++++++++++---- virt/kvm/kvm_main.c | 3 ++- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index d899473859d3..2cc438685af8 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1097,8 +1097,8 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) * 2) remote request with no data (= kick) * 3) remote request with data (= kick + mb) * - * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but - * forces smp_wmb() for all requests. + * TODO: the API does not distinguish local and remote requests -- remote + * should contain kvm_vcpu_kick(). */ static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) @@ -1106,6 +1106,37 @@ static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) set_bit(req, &vcpu->requests); } +/* + * __kvm_request_needs_mb is used to improve performance, so it should have no + * runtime overhead. + */ +static inline bool __kvm_request_needs_mb(int req) +{ + /* + * This barrier lets callers avoid the following pattern: + * if (__kvm_request_needs_mb(req)) + * ... + * else + * barrier(); + */ + barrier(); + + if (!__builtin_constant_p(req)) + return true; + +#ifdef kvm_arch_request_needs_mb + /* + * GCC optimizes pure kvm_arch_request_needs_mb() with a constant input + * into a contant, but __builtin_constant_p() is not so clever, so we + * cannot ensure that with: + * BUILD_BUG_ON(!__builtin_constant_p(kvm_arch_request_needs_mb(req))); + */ + return kvm_arch_request_needs_mb(req); +#else + return true; +#endif +} + static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) { /* @@ -1113,7 +1144,8 @@ static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) * kvm_request_test_and_clear's caller. * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. */ - smp_wmb(); + if (__kvm_request_needs_mb(req)) + smp_wmb(); __kvm_request_set(req, vcpu); } @@ -1137,7 +1169,8 @@ static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcp * kvm_request_test_and_clear's caller. * Paired with the smp_wmb in kvm_request_set. */ - smp_mb__after_atomic(); + if (__kvm_request_needs_mb(req)) + smp_mb__after_atomic(); return true; } else { return false; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2250920ec965..ced3e4cb1df0 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -179,7 +179,8 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) me = get_cpu(); /* Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. */ - smp_wmb(); + if (__kvm_request_needs_mb(req)) + smp_wmb(); kvm_for_each_vcpu(i, vcpu, kvm) { __kvm_request_set(req, vcpu); -- 2.11.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-16 16:04 ` [PATCH 4/5] KVM: add __kvm_request_needs_mb Radim Krčmář @ 2017-02-16 19:49 ` David Hildenbrand 2017-02-16 21:31 ` Radim Krčmář 2017-02-17 8:46 ` Christian Borntraeger 2017-02-23 11:01 ` Paolo Bonzini 1 sibling, 2 replies; 31+ messages in thread From: David Hildenbrand @ 2017-02-16 19:49 UTC (permalink / raw) To: Radim Krčmář, linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall, Christian Borntraeger Am 16.02.2017 um 17:04 schrieb Radim Krčmář: > A macro to optimize requests that do not need a memory barrier because > they have no dependencies. An architecture can implement a function > that says which requests do not need memory barriers when handling them. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > include/linux/kvm_host.h | 41 +++++++++++++++++++++++++++++++++++++---- > virt/kvm/kvm_main.c | 3 ++- > 2 files changed, 39 insertions(+), 5 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index d899473859d3..2cc438685af8 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1097,8 +1097,8 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) > * 2) remote request with no data (= kick) > * 3) remote request with data (= kick + mb) > * > - * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but > - * forces smp_wmb() for all requests. > + * TODO: the API does not distinguish local and remote requests -- remote > + * should contain kvm_vcpu_kick(). > */ Just for your info, kvm_vcpu_kick() and kvm_make_all_cpus_request() do not work on s390x (and in its current form never will). I tried to make it work once, but I gave up. s390x uses kvm_s390_sync_request()->kvm_s390_vcpu_request() to kick a guest out of guest mode. A special bit in the SIE control block is used to perform the kick (exit_sie(), STOP request), and another bit to prevent the guest from reentering the SIE, until the request has been handled (to avoid races). This is really complicated stuff, and the basic reason for it (if I remember correctly) is that s390x does reenable all interrupts when entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based kicks don't work (as it is otherwise just racy), and if I remember correctly, SMP reschedule signals (s390x external calls) would be slower. (Christian, please correct me if I'm wrong) So this statement, is at least from a s390x point of view wrong. The kvm_vcpu_kick() function would have to be rerouted to an appropriate s390x implementation (or that whole smp and OUTSIDE_GUEST_MODE stuff would have to be factored out). -- Thanks, David ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-16 19:49 ` David Hildenbrand @ 2017-02-16 21:31 ` Radim Krčmář 2017-02-17 8:46 ` Christian Borntraeger 1 sibling, 0 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-16 21:31 UTC (permalink / raw) To: David Hildenbrand Cc: linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall 2017-02-16 20:49+0100, David Hildenbrand: > Am 16.02.2017 um 17:04 schrieb Radim Krčmář: >> A macro to optimize requests that do not need a memory barrier because >> they have no dependencies. An architecture can implement a function >> that says which requests do not need memory barriers when handling them. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> --- >> include/linux/kvm_host.h | 41 +++++++++++++++++++++++++++++++++++++---- >> virt/kvm/kvm_main.c | 3 ++- >> 2 files changed, 39 insertions(+), 5 deletions(-) >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index d899473859d3..2cc438685af8 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -1097,8 +1097,8 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) >> * 2) remote request with no data (= kick) >> * 3) remote request with data (= kick + mb) >> * >> - * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but >> - * forces smp_wmb() for all requests. >> + * TODO: the API does not distinguish local and remote requests -- remote >> + * should contain kvm_vcpu_kick(). >> */ > > Just for your info, kvm_vcpu_kick() and kvm_make_all_cpus_request() do > not work on s390x (and in its current form never will). I tried to make > it work once, but I gave up. > > s390x uses kvm_s390_sync_request()->kvm_s390_vcpu_request() to kick a > guest out of guest mode. A special bit in the SIE control block is used > to perform the kick (exit_sie(), STOP request), and another bit to > prevent the guest from reentering the SIE, until the request has been > handled (to avoid races). Hm, kvm_s390_vcpu_wakeup() looks more suitable as the s390 implementation of kvm_vcpu_kick() (which is what we want to be connected with kvm_request). I think that kvm_s390_sync_request() is a different idea as it does not call swait_active(), so the request is delayed if the VCPU is halted. And kvm_s390_sync_request() it also waits for the VCPU to actually exit, which pushes it even further away from what other requests do. :) I would rather use bitops/barriers/kicks directly if the use of kvm_request helpers is too diverse. > This is really complicated stuff, and the basic reason for it (if I > remember correctly) is that s390x does reenable all interrupts when > entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based > kicks don't work (as it is otherwise just racy), and if I remember > correctly, SMP reschedule signals (s390x external calls) would be > slower. (Christian, please correct me if I'm wrong) Having a different mechanism for the exit itself is ok, just the general behavior has to stay the same -- kvm_vcpu_kick() does whatever is needed to let the VCPU notice possible changes in state as soon as possible and doesn't care about the VCPU any further. If other architectures had a fast mechanism that forced an immediate VCPU exit, they would gladly use it as well. > So this statement, is at least from a s390x point of view wrong. The > kvm_vcpu_kick() function would have to be rerouted to an appropriate > s390x implementation (or that whole smp and OUTSIDE_GUEST_MODE stuff > would have to be factored out). I agree. What about starting by adding __weak on functions that are currently "#ifndef CONFIG_S390" and letting s390 completely reimplement them? Thanks for the info! ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-16 19:49 ` David Hildenbrand 2017-02-16 21:31 ` Radim Krčmář @ 2017-02-17 8:46 ` Christian Borntraeger 2017-02-17 10:13 ` David Hildenbrand 1 sibling, 1 reply; 31+ messages in thread From: Christian Borntraeger @ 2017-02-17 8:46 UTC (permalink / raw) To: David Hildenbrand, Radim Krčmář, linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On 02/16/2017 08:49 PM, David Hildenbrand wrote: > Am 16.02.2017 um 17:04 schrieb Radim Krčmář: >> A macro to optimize requests that do not need a memory barrier because >> they have no dependencies. An architecture can implement a function >> that says which requests do not need memory barriers when handling them. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> --- >> include/linux/kvm_host.h | 41 +++++++++++++++++++++++++++++++++++++---- >> virt/kvm/kvm_main.c | 3 ++- >> 2 files changed, 39 insertions(+), 5 deletions(-) >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index d899473859d3..2cc438685af8 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -1097,8 +1097,8 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) >> * 2) remote request with no data (= kick) >> * 3) remote request with data (= kick + mb) >> * >> - * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but >> - * forces smp_wmb() for all requests. >> + * TODO: the API does not distinguish local and remote requests -- remote >> + * should contain kvm_vcpu_kick(). >> */ > > Just for your info, kvm_vcpu_kick() and kvm_make_all_cpus_request() do > not work on s390x (and in its current form never will). I tried to make > it work once, but I gave up. > > s390x uses kvm_s390_sync_request()->kvm_s390_vcpu_request() to kick a > guest out of guest mode. A special bit in the SIE control block is used > to perform the kick (exit_sie(), STOP request), and another bit to > prevent the guest from reentering the SIE, until the request has been > handled (to avoid races). > > This is really complicated stuff, and the basic reason for it (if I > remember correctly) is that s390x does reenable all interrupts when > entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based > kicks don't work (as it is otherwise just racy), and if I remember > correctly, SMP reschedule signals (s390x external calls) would be > slower. (Christian, please correct me if I'm wrong) No the reason was that there are some requests that need to be handled outside run SIE. For example one reason was the guest prefix page. This must be mapped read/write ALL THE TIME when a guest is running, otherwise the host might crash. So we have to exit SIE and make sure that it does not reenter, therefore we use the RELOAD_MMU request from a notifier that is called from page table functions, whenever memory management decides to unmap/write protect (dirty pages tracking, reference tracking, page migration or compaction...) SMP-based request wills kick out the guest, but for some thing like the one above it will be too late. > So this statement, is at least from a s390x point of view wrong. The > kvm_vcpu_kick() function would have to be rerouted to an appropriate > s390x implementation (or that whole smp and OUTSIDE_GUEST_MODE stuff > would have to be factored out). > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-17 8:46 ` Christian Borntraeger @ 2017-02-17 10:13 ` David Hildenbrand 2017-02-17 10:19 ` Christian Borntraeger ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: David Hildenbrand @ 2017-02-17 10:13 UTC (permalink / raw) To: Christian Borntraeger, Radim Krčmář, linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall >> This is really complicated stuff, and the basic reason for it (if I >> remember correctly) is that s390x does reenable all interrupts when >> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based >> kicks don't work (as it is otherwise just racy), and if I remember >> correctly, SMP reschedule signals (s390x external calls) would be >> slower. (Christian, please correct me if I'm wrong) > > No the reason was that there are some requests that need to be handled > outside run SIE. For example one reason was the guest prefix page. > This must be mapped read/write ALL THE TIME when a guest is running, > otherwise the host might crash. So we have to exit SIE and make sure that > it does not reenter, therefore we use the RELOAD_MMU request from a notifier > that is called from page table functions, whenever memory management decides > to unmap/write protect (dirty pages tracking, reference tracking, page migration > or compaction...) > > SMP-based request wills kick out the guest, but for some thing like the > one above it will be too late. While what you said is 100% correct, I had something else in mind that hindered using vcpu_kick() and especially kvm_make_all_cpus_request(). And I remember that being related to how preemption and OUTSIDE_GUEST_MODE is handled. I think this boils down to what would have to be implemented in kvm_arch_vcpu_should_kick(). x86 can track the guest state using vcpu->mode, because they can be sure that the guest can't reschedule while in the critical guest entry/exit section. This is not true for s390x, as preemption is enabled. That's why vcpu->mode cannot be used in its current form to track if a VCPU is in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently relies on this setting. For now, calling vcpu_kick() on s390x will result in a BUG(). On s390x, there are 3 use cases I see for requests: 1. Remote requests that need a sync Make a request, wait until SIE has been left and make sure the request will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a candidate. 2. Remote requests that don't need a sync E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while KVM_REQ_DISABLE_IBS does. 3. local requests E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() Of course, having a unified interface would be better. /* set the request and kick the CPU out of guest mode */ kvm_set_request(req, vcpu); /* set the request, kick the CPU out of guest mode, wait until guest mode has been left and make sure the request will be handled before reentering guest mode */ kvm_set_sync_request(req, vcpu); Same maybe even for multiple VCPUs (as there are then ways to speed it up, e.g. first kick all, then wait for all) This would require arch specific callbacks to 1. pre announce the request (e.g. set PROG_REQUEST on s390x) 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later kvm_s390_vsie_kick(vcpu) on s390x) 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) This would only make sense if there are other use cases for sync requests. At least I remember that Power also has a faster way for kicking VCPUs, not involving SMP rescheds. I can't judge if this is a s390x only thing and is better be left as is :) At least vcpu_kick() could be quite easily made to work on s390x. Radim, are there also other users that need something like sync requests? > > >> So this statement, is at least from a s390x point of view wrong. The >> kvm_vcpu_kick() function would have to be rerouted to an appropriate >> s390x implementation (or that whole smp and OUTSIDE_GUEST_MODE stuff >> would have to be factored out). >> > -- Thanks, David ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-17 10:13 ` David Hildenbrand @ 2017-02-17 10:19 ` Christian Borntraeger 2017-02-17 11:28 ` Christian Borntraeger 2017-02-22 15:17 ` Radim Krčmář 2 siblings, 0 replies; 31+ messages in thread From: Christian Borntraeger @ 2017-02-17 10:19 UTC (permalink / raw) To: David Hildenbrand, Radim Krčmář, linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On 02/17/2017 11:13 AM, David Hildenbrand wrote: > >>> This is really complicated stuff, and the basic reason for it (if I >>> remember correctly) is that s390x does reenable all interrupts when >>> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based >>> kicks don't work (as it is otherwise just racy), and if I remember >>> correctly, SMP reschedule signals (s390x external calls) would be >>> slower. (Christian, please correct me if I'm wrong) >> >> No the reason was that there are some requests that need to be handled >> outside run SIE. For example one reason was the guest prefix page. >> This must be mapped read/write ALL THE TIME when a guest is running, >> otherwise the host might crash. So we have to exit SIE and make sure that >> it does not reenter, therefore we use the RELOAD_MMU request from a notifier >> that is called from page table functions, whenever memory management decides >> to unmap/write protect (dirty pages tracking, reference tracking, page migration >> or compaction...) >> >> SMP-based request wills kick out the guest, but for some thing like the >> one above it will be too late. > > While what you said is 100% correct, I had something else in mind that > hindered using vcpu_kick() and especially kvm_make_all_cpus_request(). > And I remember that being related to how preemption and > OUTSIDE_GUEST_MODE is handled. I think this boils down to what would > have to be implemented in kvm_arch_vcpu_should_kick(). > > x86 can track the guest state using vcpu->mode, because they can be sure > that the guest can't reschedule while in the critical guest entry/exit > section. This is not true for s390x, as preemption is enabled. That's > why vcpu->mode cannot be used in its current form to track if a VCPU is > in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently > relies on this setting. > > For now, calling vcpu_kick() on s390x will result in a BUG(). Yes, you are right, preemption and interrupt handling certainly made it hard to switch to the common code function. The prefix page oddity was the original reason for implementing the s390 special code since we realized that even mlocking and pinning did not guarantee that the page table stays writable in the page table. > > > On s390x, there are 3 use cases I see for requests: > > 1. Remote requests that need a sync > > Make a request, wait until SIE has been left and make sure the request > will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU > notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a > candidate. > > 2. Remote requests that don't need a sync > > E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while > KVM_REQ_DISABLE_IBS does. > > 3. local requests > > E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() > > > Of course, having a unified interface would be better. Yes, we tried to be as common as possible. This enabled us to use things like halt polling. Radims idea of a weak function could work out, have not checked yet. > > /* set the request and kick the CPU out of guest mode */ > kvm_set_request(req, vcpu); > > /* set the request, kick the CPU out of guest mode, wait until guest > mode has been left and make sure the request will be handled before > reentering guest mode */ > kvm_set_sync_request(req, vcpu); > > > Same maybe even for multiple VCPUs (as there are then ways to speed it > up, e.g. first kick all, then wait for all) > > This would require arch specific callbacks to > 1. pre announce the request (e.g. set PROG_REQUEST on s390x) > 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later > kvm_s390_vsie_kick(vcpu) on s390x) > 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) > > This would only make sense if there are other use cases for sync > requests. At least I remember that Power also has a faster way for > kicking VCPUs, not involving SMP rescheds. I can't judge if this is a > s390x only thing and is better be left as is :) > > At least vcpu_kick() could be quite easily made to work on s390x. > > Radim, are there also other users that need something like sync requests? > >> >> >>> So this statement, is at least from a s390x point of view wrong. The >>> kvm_vcpu_kick() function would have to be rerouted to an appropriate >>> s390x implementation (or that whole smp and OUTSIDE_GUEST_MODE stuff >>> would have to be factored out). >>> >> > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-17 10:13 ` David Hildenbrand 2017-02-17 10:19 ` Christian Borntraeger @ 2017-02-17 11:28 ` Christian Borntraeger 2017-02-22 15:17 ` Radim Krčmář 2 siblings, 0 replies; 31+ messages in thread From: Christian Borntraeger @ 2017-02-17 11:28 UTC (permalink / raw) To: David Hildenbrand, Radim Krčmář, linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On 02/17/2017 11:13 AM, David Hildenbrand wrote: > >>> This is really complicated stuff, and the basic reason for it (if I >>> remember correctly) is that s390x does reenable all interrupts when >>> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based >>> kicks don't work (as it is otherwise just racy), and if I remember >>> correctly, SMP reschedule signals (s390x external calls) would be >>> slower. (Christian, please correct me if I'm wrong) >> >> No the reason was that there are some requests that need to be handled >> outside run SIE. For example one reason was the guest prefix page. >> This must be mapped read/write ALL THE TIME when a guest is running, >> otherwise the host might crash. So we have to exit SIE and make sure that >> it does not reenter, therefore we use the RELOAD_MMU request from a notifier >> that is called from page table functions, whenever memory management decides >> to unmap/write protect (dirty pages tracking, reference tracking, page migration >> or compaction...) >> >> SMP-based request wills kick out the guest, but for some thing like the >> one above it will be too late. > > While what you said is 100% correct, I had something else in mind that > hindered using vcpu_kick() and especially kvm_make_all_cpus_request(). > And I remember that being related to how preemption and > OUTSIDE_GUEST_MODE is handled. I think this boils down to what would > have to be implemented in kvm_arch_vcpu_should_kick(). > > x86 can track the guest state using vcpu->mode, because they can be sure > that the guest can't reschedule while in the critical guest entry/exit > section. This is not true for s390x, as preemption is enabled. That's > why vcpu->mode cannot be used in its current form to track if a VCPU is > in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently > relies on this setting. > > For now, calling vcpu_kick() on s390x will result in a BUG(). > > > On s390x, there are 3 use cases I see for requests: > > 1. Remote requests that need a sync > > Make a request, wait until SIE has been left and make sure the request > will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU > notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a > candidate. > > 2. Remote requests that don't need a sync > > E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while > KVM_REQ_DISABLE_IBS does. > > 3. local requests > > E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() > > > Of course, having a unified interface would be better. > > /* set the request and kick the CPU out of guest mode */ > kvm_set_request(req, vcpu); > > /* set the request, kick the CPU out of guest mode, wait until guest > mode has been left and make sure the request will be handled before > reentering guest mode */ > kvm_set_sync_request(req, vcpu); > > > Same maybe even for multiple VCPUs (as there are then ways to speed it > up, e.g. first kick all, then wait for all) > > This would require arch specific callbacks to > 1. pre announce the request (e.g. set PROG_REQUEST on s390x) > 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later > kvm_s390_vsie_kick(vcpu) on s390x) > 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) > > This would only make sense if there are other use cases for sync > requests. At least I remember that Power also has a faster way for > kicking VCPUs, not involving SMP rescheds. I can't judge if this is a > s390x only thing and is better be left as is :) Hmmm, maybe we should simply enable kvm_vcpu_kick and kvm_vcpu_wake_up in common code. They should work for the cases from common code. We must still keep the s390 specific functions and we will call those from within s390 code when necessary. Back then you tried to replace our functions and that had issues (functionality wise and speed wise) - maybe just keeping those is the easiest solution. For kvm_make_all_cpus_request we already have the slow path of waking all CPUs, so that would be ok. So why not have vcpu->mode = IN_GUEST_MODE for the whole inner loop of s390? I will try to come up with a patch. Christian ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-17 10:13 ` David Hildenbrand 2017-02-17 10:19 ` Christian Borntraeger 2017-02-17 11:28 ` Christian Borntraeger @ 2017-02-22 15:17 ` Radim Krčmář 2017-02-22 19:23 ` Christian Borntraeger ` (2 more replies) 2 siblings, 3 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-22 15:17 UTC (permalink / raw) To: David Hildenbrand Cc: Christian Borntraeger, linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall [Oops, the end of this thread got dragged into a mark-as-read spree ...] 2017-02-17 11:13+0100, David Hildenbrand: >>> This is really complicated stuff, and the basic reason for it (if I >>> remember correctly) is that s390x does reenable all interrupts when >>> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based >>> kicks don't work (as it is otherwise just racy), and if I remember >>> correctly, SMP reschedule signals (s390x external calls) would be >>> slower. (Christian, please correct me if I'm wrong) >> >> No the reason was that there are some requests that need to be handled >> outside run SIE. For example one reason was the guest prefix page. >> This must be mapped read/write ALL THE TIME when a guest is running, >> otherwise the host might crash. So we have to exit SIE and make sure that >> it does not reenter, therefore we use the RELOAD_MMU request from a notifier >> that is called from page table functions, whenever memory management decides >> to unmap/write protect (dirty pages tracking, reference tracking, page migration >> or compaction...) >> >> SMP-based request wills kick out the guest, but for some thing like the >> one above it will be too late. > > While what you said is 100% correct, I had something else in mind that > hindered using vcpu_kick() and especially kvm_make_all_cpus_request(). > And I remember that being related to how preemption and > OUTSIDE_GUEST_MODE is handled. I think this boils down to what would > have to be implemented in kvm_arch_vcpu_should_kick(). > > x86 can track the guest state using vcpu->mode, because they can be sure > that the guest can't reschedule while in the critical guest entry/exit > section. This is not true for s390x, as preemption is enabled. That's > why vcpu->mode cannot be used in its current form to track if a VCPU is > in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently > relies on this setting. > > For now, calling vcpu_kick() on s390x will result in a BUG(). > > > On s390x, there are 3 use cases I see for requests: > > 1. Remote requests that need a sync > > Make a request, wait until SIE has been left and make sure the request > will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU > notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a > candidate. Btw. aren't those requests racy? void exit_sie(struct kvm_vcpu *vcpu) { atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags); If you get stalled here and the target VCPU handles the request and reenters SIE in the meantime, then you'll wait until its next exit. (And miss an unbounded amount of exits in the worst case.) while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE) cpu_relax(); } And out of curiosity -- how many cycles does this loop usually take? > 2. Remote requests that don't need a sync > > E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while > KVM_REQ_DISABLE_IBS does. A usual KVM request would kick the VCPU out of nested virt as well. Shouldn't it be done for these as well? > 3. local requests > > E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() > > > Of course, having a unified interface would be better. > > /* set the request and kick the CPU out of guest mode */ > kvm_set_request(req, vcpu); > > /* set the request, kick the CPU out of guest mode, wait until guest > mode has been left and make sure the request will be handled before > reentering guest mode */ > kvm_set_sync_request(req, vcpu); Sounds good, I'll also add kvm_set_self_request(req, vcpu); > Same maybe even for multiple VCPUs (as there are then ways to speed it > up, e.g. first kick all, then wait for all) > > This would require arch specific callbacks to > 1. pre announce the request (e.g. set PROG_REQUEST on s390x) > 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later > kvm_s390_vsie_kick(vcpu) on s390x) > 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) > > This would only make sense if there are other use cases for sync > requests. At least I remember that Power also has a faster way for > kicking VCPUs, not involving SMP rescheds. I can't judge if this is a > s390x only thing and is better be left as is :) > > At least vcpu_kick() could be quite easily made to work on s390x. > > Radim, are there also other users that need something like sync requests? I think that ARM has a similar need when updating vgic, but relies on an asumption that VCPUs are going to be out after kicking them with kvm_make_all_cpus_request(). (vgic_change_active_prepare in virt/kvm/arm/vgic/vgic-mmio.c) Having synchronous requests in a common API should probably wait for the completion of the request, not just for the kick, which would make race handling simpler. I'm not going to worry about them in this pass, though. Thanks. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-22 15:17 ` Radim Krčmář @ 2017-02-22 19:23 ` Christian Borntraeger 2017-02-23 15:43 ` Radim Krčmář 2017-02-22 19:57 ` Christian Borntraeger 2017-02-24 11:34 ` Christoffer Dall 2 siblings, 1 reply; 31+ messages in thread From: Christian Borntraeger @ 2017-02-22 19:23 UTC (permalink / raw) To: Radim Krčmář, David Hildenbrand Cc: linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On 02/22/2017 04:17 PM, Radim Krčmář wrote: > [Oops, the end of this thread got dragged into a mark-as-read spree ...] > > 2017-02-17 11:13+0100, David Hildenbrand: >>>> This is really complicated stuff, and the basic reason for it (if I >>>> remember correctly) is that s390x does reenable all interrupts when >>>> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based >>>> kicks don't work (as it is otherwise just racy), and if I remember >>>> correctly, SMP reschedule signals (s390x external calls) would be >>>> slower. (Christian, please correct me if I'm wrong) >>> >>> No the reason was that there are some requests that need to be handled >>> outside run SIE. For example one reason was the guest prefix page. >>> This must be mapped read/write ALL THE TIME when a guest is running, >>> otherwise the host might crash. So we have to exit SIE and make sure that >>> it does not reenter, therefore we use the RELOAD_MMU request from a notifier >>> that is called from page table functions, whenever memory management decides >>> to unmap/write protect (dirty pages tracking, reference tracking, page migration >>> or compaction...) >>> >>> SMP-based request wills kick out the guest, but for some thing like the >>> one above it will be too late. >> >> While what you said is 100% correct, I had something else in mind that >> hindered using vcpu_kick() and especially kvm_make_all_cpus_request(). >> And I remember that being related to how preemption and >> OUTSIDE_GUEST_MODE is handled. I think this boils down to what would >> have to be implemented in kvm_arch_vcpu_should_kick(). >> >> x86 can track the guest state using vcpu->mode, because they can be sure >> that the guest can't reschedule while in the critical guest entry/exit >> section. This is not true for s390x, as preemption is enabled. That's >> why vcpu->mode cannot be used in its current form to track if a VCPU is >> in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently >> relies on this setting. >> >> For now, calling vcpu_kick() on s390x will result in a BUG(). >> >> >> On s390x, there are 3 use cases I see for requests: >> >> 1. Remote requests that need a sync >> >> Make a request, wait until SIE has been left and make sure the request >> will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU >> notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a >> candidate. > > Btw. aren't those requests racy? > > void exit_sie(struct kvm_vcpu *vcpu) > { > atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags); > > If you get stalled here and the target VCPU handles the request and > reenters SIE in the meantime, then you'll wait until its next exit. > (And miss an unbounded amount of exits in the worst case.) > > while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE) > cpu_relax(); > } > Its not racy for the purpose it was originally made for (get the vcpu out of SIE before we unmap a guest prefix page) as the MMU_RELOAD handler will wait for the pte lock which is held by the code that called kvm_s390_sync_request(KVM_REQ_MMU_RELOAD, vcpu). We also have the guarantee that after returning from kvm_s390_sync_request we will have that request be handled before we reenter the guest, which is all we need for DISABLE_IBS. But yes, all non MMU_RELOAD users might wait longer, possibly several guest exits. We never noticed that as requests are really a seldom event. Basically unmapping of the guest prefix page due to paging and migration, switching between 1 and more guest cpus and some other seldom events. > And out of curiosity -- how many cycles does this loop usually take? > >> 2. Remote requests that don't need a sync >> >> E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while >> KVM_REQ_DISABLE_IBS does. > > A usual KVM request would kick the VCPU out of nested virt as well. > Shouldn't it be done for these as well? > >> 3. local requests >> >> E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() >> >> >> Of course, having a unified interface would be better. >> >> /* set the request and kick the CPU out of guest mode */ >> kvm_set_request(req, vcpu); >> >> /* set the request, kick the CPU out of guest mode, wait until guest >> mode has been left and make sure the request will be handled before >> reentering guest mode */ >> kvm_set_sync_request(req, vcpu); > > Sounds good, I'll also add > > kvm_set_self_request(req, vcpu); > >> Same maybe even for multiple VCPUs (as there are then ways to speed it >> up, e.g. first kick all, then wait for all) >> >> This would require arch specific callbacks to >> 1. pre announce the request (e.g. set PROG_REQUEST on s390x) >> 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later >> kvm_s390_vsie_kick(vcpu) on s390x) >> 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) >> >> This would only make sense if there are other use cases for sync >> requests. At least I remember that Power also has a faster way for >> kicking VCPUs, not involving SMP rescheds. I can't judge if this is a >> s390x only thing and is better be left as is :) >> >> At least vcpu_kick() could be quite easily made to work on s390x. >> >> Radim, are there also other users that need something like sync requests? > > I think that ARM has a similar need when updating vgic, but relies on an > asumption that VCPUs are going to be out after kicking them with > kvm_make_all_cpus_request(). > (vgic_change_active_prepare in virt/kvm/arm/vgic/vgic-mmio.c) > > Having synchronous requests in a common API should probably wait for the > completion of the request, not just for the kick, which would make race > handling simpler. > > I'm not going to worry about them in this pass, though. > > Thanks. > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-22 19:23 ` Christian Borntraeger @ 2017-02-23 15:43 ` Radim Krčmář 0 siblings, 0 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-23 15:43 UTC (permalink / raw) To: Christian Borntraeger Cc: David Hildenbrand, linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall 2017-02-22 20:23+0100, Christian Borntraeger: > On 02/22/2017 04:17 PM, Radim Krčmář wrote: >> [Oops, the end of this thread got dragged into a mark-as-read spree ...] >> >> 2017-02-17 11:13+0100, David Hildenbrand: >>>>> This is really complicated stuff, and the basic reason for it (if I >>>>> remember correctly) is that s390x does reenable all interrupts when >>>>> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based >>>>> kicks don't work (as it is otherwise just racy), and if I remember >>>>> correctly, SMP reschedule signals (s390x external calls) would be >>>>> slower. (Christian, please correct me if I'm wrong) >>>> >>>> No the reason was that there are some requests that need to be handled >>>> outside run SIE. For example one reason was the guest prefix page. >>>> This must be mapped read/write ALL THE TIME when a guest is running, >>>> otherwise the host might crash. So we have to exit SIE and make sure that >>>> it does not reenter, therefore we use the RELOAD_MMU request from a notifier >>>> that is called from page table functions, whenever memory management decides >>>> to unmap/write protect (dirty pages tracking, reference tracking, page migration >>>> or compaction...) >>>> >>>> SMP-based request wills kick out the guest, but for some thing like the >>>> one above it will be too late. >>> >>> While what you said is 100% correct, I had something else in mind that >>> hindered using vcpu_kick() and especially kvm_make_all_cpus_request(). >>> And I remember that being related to how preemption and >>> OUTSIDE_GUEST_MODE is handled. I think this boils down to what would >>> have to be implemented in kvm_arch_vcpu_should_kick(). >>> >>> x86 can track the guest state using vcpu->mode, because they can be sure >>> that the guest can't reschedule while in the critical guest entry/exit >>> section. This is not true for s390x, as preemption is enabled. That's >>> why vcpu->mode cannot be used in its current form to track if a VCPU is >>> in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently >>> relies on this setting. >>> >>> For now, calling vcpu_kick() on s390x will result in a BUG(). >>> >>> >>> On s390x, there are 3 use cases I see for requests: >>> >>> 1. Remote requests that need a sync >>> >>> Make a request, wait until SIE has been left and make sure the request >>> will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU >>> notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a >>> candidate. >> >> Btw. aren't those requests racy? >> >> void exit_sie(struct kvm_vcpu *vcpu) >> { >> atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags); >> >> If you get stalled here and the target VCPU handles the request and >> reenters SIE in the meantime, then you'll wait until its next exit. >> (And miss an unbounded amount of exits in the worst case.) >> >> while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE) >> cpu_relax(); >> } >> > > Its not racy for the purpose it was originally made for (get the vcpu > out of SIE before we unmap a guest prefix page) as the MMU_RELOAD handler > will wait for the pte lock which is held by the code that called > kvm_s390_sync_request(KVM_REQ_MMU_RELOAD, vcpu). > > We also have the guarantee that after returning from kvm_s390_sync_request > we will have that request be handled before we reenter the guest, which is > all we need for DISABLE_IBS. > > But yes, all non MMU_RELOAD users might wait longer, possibly several guest > exits. We never noticed that as requests are really a seldom event. Basically > unmapping of the guest prefix page due to paging and migration, switching > between 1 and more guest cpus and some other seldom events. Ok, thanks for the info. I don't think that we'll find too many use-cases to demand inclusion into a generic kick/request API, so having a function that waits until a VCPU is out of guest mode would be more suited for generic code. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-22 15:17 ` Radim Krčmář 2017-02-22 19:23 ` Christian Borntraeger @ 2017-02-22 19:57 ` Christian Borntraeger 2017-02-23 10:20 ` David Hildenbrand 2017-02-24 11:34 ` Christoffer Dall 2 siblings, 1 reply; 31+ messages in thread From: Christian Borntraeger @ 2017-02-22 19:57 UTC (permalink / raw) To: Radim Krčmář, David Hildenbrand Cc: linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On 02/22/2017 04:17 PM, Radim Krčmář wrote: > [...] > while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE) > cpu_relax(); > } > And out of curiosity -- how many cycles does this loop usually take? A quick hack indicates something between 3 and 700ns. >> 2. Remote requests that don't need a sync >> >> E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while >> KVM_REQ_DISABLE_IBS does. > > A usual KVM request would kick the VCPU out of nested virt as well. > Shouldn't it be done for these as well? A common code function probably should. For some of the cases (again prefix page handling) we do not need it. For example if we unmap the guest prefix page, but guest^2 is running this causes no trouble as long as we handle the request before reentering guest^1. So not an easy answer. > >> 3. local requests >> >> E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() >> >> >> Of course, having a unified interface would be better. >> >> /* set the request and kick the CPU out of guest mode */ >> kvm_set_request(req, vcpu); >> >> /* set the request, kick the CPU out of guest mode, wait until guest >> mode has been left and make sure the request will be handled before >> reentering guest mode */ >> kvm_set_sync_request(req, vcpu); > > Sounds good, I'll also add > > kvm_set_self_request(req, vcpu); > >> Same maybe even for multiple VCPUs (as there are then ways to speed it >> up, e.g. first kick all, then wait for all) >> >> This would require arch specific callbacks to >> 1. pre announce the request (e.g. set PROG_REQUEST on s390x) >> 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later >> kvm_s390_vsie_kick(vcpu) on s390x) >> 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) >> >> This would only make sense if there are other use cases for sync >> requests. At least I remember that Power also has a faster way for >> kicking VCPUs, not involving SMP rescheds. I can't judge if this is a >> s390x only thing and is better be left as is :) >> >> At least vcpu_kick() could be quite easily made to work on s390x. >> >> Radim, are there also other users that need something like sync requests? > > I think that ARM has a similar need when updating vgic, but relies on an > asumption that VCPUs are going to be out after kicking them with > kvm_make_all_cpus_request(). > (vgic_change_active_prepare in virt/kvm/arm/vgic/vgic-mmio.c) > > Having synchronous requests in a common API should probably wait for the > completion of the request, not just for the kick, which would make race > handling simpler. This would be problematic for our prefix page handling due to locking. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-22 19:57 ` Christian Borntraeger @ 2017-02-23 10:20 ` David Hildenbrand 2017-02-23 15:39 ` Radim Krčmář 0 siblings, 1 reply; 31+ messages in thread From: David Hildenbrand @ 2017-02-23 10:20 UTC (permalink / raw) To: Christian Borntraeger, Radim Krčmář Cc: linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall Am 22.02.2017 um 20:57 schrieb Christian Borntraeger: > On 02/22/2017 04:17 PM, Radim Krčmář wrote: >> > [...] >> while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE) >> cpu_relax(); >> } > >> And out of curiosity -- how many cycles does this loop usually take? > > A quick hack indicates something between 3 and 700ns. > >>> 2. Remote requests that don't need a sync >>> >>> E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while >>> KVM_REQ_DISABLE_IBS does. >> >> A usual KVM request would kick the VCPU out of nested virt as well. >> Shouldn't it be done for these as well? > > A common code function probably should. For some of the cases (again > prefix page handling) we do not need it. For example if we unmap > the guest prefix page, but guest^2 is running this causes no trouble > as long as we handle the request before reentering guest^1. So > not an easy answer. The problem is, that doing synchronous requests on two sie control blocks (vsie and "ordinary") is really really hard to implement. I had a prototype, but it was just ugly. And there was no reason to do it, because all current requests can live with nested guest being executed (as it is really all just a matter of coordinating SIE control block changes for our guest, not involving nested guests). Also note, that VSIE uses these special request bits in the SIE control block for its own purposes (to catch unmappings of the prefix page, but this time on the nested address space). We don't want to replace this by an ordinary request bit (because then we have to exit the VSIE loop much more often). I think the VSIE code should not care too much about request bits until there is really a need for it (meaning: VCPU requests that cannot tolerate the VSIE running). Kicking the VSIE from time to time cannot harm. But there are no guarantees about requests. > >> >>> 3. local requests >>> >>> E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() >>> >>> >>> Of course, having a unified interface would be better. >>> >>> /* set the request and kick the CPU out of guest mode */ >>> kvm_set_request(req, vcpu); >>> >>> /* set the request, kick the CPU out of guest mode, wait until guest >>> mode has been left and make sure the request will be handled before >>> reentering guest mode */ >>> kvm_set_sync_request(req, vcpu); >> >> Sounds good, I'll also add >> >> kvm_set_self_request(req, vcpu); or introduce some lightweight check (e.g. against (vcpu->cpu)) internally, that simply skips kicking/other stuff in case the vcpu is currently loaded. >> >>> Same maybe even for multiple VCPUs (as there are then ways to speed it >>> up, e.g. first kick all, then wait for all) >>> >>> This would require arch specific callbacks to >>> 1. pre announce the request (e.g. set PROG_REQUEST on s390x) >>> 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later >>> kvm_s390_vsie_kick(vcpu) on s390x) >>> 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) >>> >>> This would only make sense if there are other use cases for sync >>> requests. At least I remember that Power also has a faster way for >>> kicking VCPUs, not involving SMP rescheds. I can't judge if this is a >>> s390x only thing and is better be left as is :) >>> >>> At least vcpu_kick() could be quite easily made to work on s390x. >>> >>> Radim, are there also other users that need something like sync requests? >> >> I think that ARM has a similar need when updating vgic, but relies on an >> asumption that VCPUs are going to be out after kicking them with >> kvm_make_all_cpus_request(). >> (vgic_change_active_prepare in virt/kvm/arm/vgic/vgic-mmio.c) >> >> Having synchronous requests in a common API should probably wait for the >> completion of the request, not just for the kick, which would make race >> handling simpler. > > This would be problematic for our prefix page handling due to locking. > And if I am not wrong, waiting for a request to be handled might take forever (thinking about VCPUs in user space - maybe even stopped ones that won't run again). I think the general notion of synchronous VCPU requests should be: Make sure that VCPU does not go into guest mode before handling the request. This includes waiting for it to exit guest mode. -- Thanks, David ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-23 10:20 ` David Hildenbrand @ 2017-02-23 15:39 ` Radim Krčmář 0 siblings, 0 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-23 15:39 UTC (permalink / raw) To: David Hildenbrand Cc: Christian Borntraeger, linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall 2017-02-23 11:20+0100, David Hildenbrand: > Am 22.02.2017 um 20:57 schrieb Christian Borntraeger: >> On 02/22/2017 04:17 PM, Radim Krčmář wrote: >>> >> [...] >>> while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE) >>> cpu_relax(); >>> } >> >>> And out of curiosity -- how many cycles does this loop usually take? >> >> A quick hack indicates something between 3 and 700ns. >> >>>> 2. Remote requests that don't need a sync >>>> >>>> E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while >>>> KVM_REQ_DISABLE_IBS does. >>> >>> A usual KVM request would kick the VCPU out of nested virt as well. >>> Shouldn't it be done for these as well? >> >> A common code function probably should. For some of the cases (again >> prefix page handling) we do not need it. For example if we unmap >> the guest prefix page, but guest^2 is running this causes no trouble >> as long as we handle the request before reentering guest^1. So >> not an easy answer. > > The problem is, that doing synchronous requests on two sie control > blocks (vsie and "ordinary") is really really hard to implement. I had a > prototype, but it was just ugly. And there was no reason to do it, > because all current requests can live with nested guest being executed > (as it is really all just a matter of coordinating SIE control block > changes for our guest, not involving nested guests). > > Also note, that VSIE uses these special request bits in the SIE control > block for its own purposes (to catch unmappings of the prefix page, but > this time on the nested address space). We don't want to replace this by > an ordinary request bit (because then we have to exit the VSIE loop much > more often). > > I think the VSIE code should not care too much about request bits until > there is really a need for it (meaning: VCPU requests that cannot > tolerate the VSIE running). Sure, request-based changes to L1 can be delayed if they cannot affect L2. > Kicking the VSIE from time to time cannot harm. But there are no > guarantees about requests. > >> >>> >>>> 3. local requests >>>> >>>> E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() >>>> >>>> >>>> Of course, having a unified interface would be better. >>>> >>>> /* set the request and kick the CPU out of guest mode */ >>>> kvm_set_request(req, vcpu); >>>> >>>> /* set the request, kick the CPU out of guest mode, wait until guest >>>> mode has been left and make sure the request will be handled before >>>> reentering guest mode */ >>>> kvm_set_sync_request(req, vcpu); >>> >>> Sounds good, I'll also add >>> >>> kvm_set_self_request(req, vcpu); > > or introduce some lightweight check (e.g. against (vcpu->cpu)) > internally, that simply skips kicking/other stuff in case the vcpu is > currently loaded. Sounds interesting, I'll benchmark it on x86. I was planning on adding a check inside CONFIG_DEBUG WARN_ON_ONCE, but it might be cheap enough and definitely simplifies the API. The check is already performed in kvm_vcpu_kick() and I hope that it won't need a big refactoring to avoid copy-paste. >>>> Same maybe even for multiple VCPUs (as there are then ways to speed it >>>> up, e.g. first kick all, then wait for all) >>>> >>>> This would require arch specific callbacks to >>>> 1. pre announce the request (e.g. set PROG_REQUEST on s390x) >>>> 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later >>>> kvm_s390_vsie_kick(vcpu) on s390x) >>>> 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) >>>> >>>> This would only make sense if there are other use cases for sync >>>> requests. At least I remember that Power also has a faster way for >>>> kicking VCPUs, not involving SMP rescheds. I can't judge if this is a >>>> s390x only thing and is better be left as is :) >>>> >>>> At least vcpu_kick() could be quite easily made to work on s390x. >>>> >>>> Radim, are there also other users that need something like sync requests? >>> >>> I think that ARM has a similar need when updating vgic, but relies on an >>> asumption that VCPUs are going to be out after kicking them with >>> kvm_make_all_cpus_request(). >>> (vgic_change_active_prepare in virt/kvm/arm/vgic/vgic-mmio.c) >>> >>> Having synchronous requests in a common API should probably wait for the >>> completion of the request, not just for the kick, which would make race >>> handling simpler. >> >> This would be problematic for our prefix page handling due to locking. >> > > And if I am not wrong, waiting for a request to be handled might take > forever (thinking about VCPUs in user space - maybe even stopped ones > that won't run again). I agree. > I think the general notion of synchronous VCPU requests should be: Make > sure that VCPU does not go into guest mode before handling the request. > This includes waiting for it to exit guest mode. I'll keep synchronous requests out for the moment. There is nothing similar in other architectures and the idea is very specific even in s390. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-22 15:17 ` Radim Krčmář 2017-02-22 19:23 ` Christian Borntraeger 2017-02-22 19:57 ` Christian Borntraeger @ 2017-02-24 11:34 ` Christoffer Dall 2017-02-24 12:46 ` Andrew Jones 2 siblings, 1 reply; 31+ messages in thread From: Christoffer Dall @ 2017-02-24 11:34 UTC (permalink / raw) To: Radim Krčmář Cc: David Hildenbrand, Christian Borntraeger, linux-kernel, kvm, Paolo Bonzini, Andrew Jones, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On Wed, Feb 22, 2017 at 04:17:05PM +0100, Radim Krčmář wrote: > [Oops, the end of this thread got dragged into a mark-as-read spree ...] > > 2017-02-17 11:13+0100, David Hildenbrand: > >>> This is really complicated stuff, and the basic reason for it (if I > >>> remember correctly) is that s390x does reenable all interrupts when > >>> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based > >>> kicks don't work (as it is otherwise just racy), and if I remember > >>> correctly, SMP reschedule signals (s390x external calls) would be > >>> slower. (Christian, please correct me if I'm wrong) > >> > >> No the reason was that there are some requests that need to be handled > >> outside run SIE. For example one reason was the guest prefix page. > >> This must be mapped read/write ALL THE TIME when a guest is running, > >> otherwise the host might crash. So we have to exit SIE and make sure that > >> it does not reenter, therefore we use the RELOAD_MMU request from a notifier > >> that is called from page table functions, whenever memory management decides > >> to unmap/write protect (dirty pages tracking, reference tracking, page migration > >> or compaction...) > >> > >> SMP-based request wills kick out the guest, but for some thing like the > >> one above it will be too late. > > > > While what you said is 100% correct, I had something else in mind that > > hindered using vcpu_kick() and especially kvm_make_all_cpus_request(). > > And I remember that being related to how preemption and > > OUTSIDE_GUEST_MODE is handled. I think this boils down to what would > > have to be implemented in kvm_arch_vcpu_should_kick(). > > > > x86 can track the guest state using vcpu->mode, because they can be sure > > that the guest can't reschedule while in the critical guest entry/exit > > section. This is not true for s390x, as preemption is enabled. That's > > why vcpu->mode cannot be used in its current form to track if a VCPU is > > in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently > > relies on this setting. > > > > For now, calling vcpu_kick() on s390x will result in a BUG(). > > > > > > On s390x, there are 3 use cases I see for requests: > > > > 1. Remote requests that need a sync > > > > Make a request, wait until SIE has been left and make sure the request > > will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU > > notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a > > candidate. > > Btw. aren't those requests racy? > > void exit_sie(struct kvm_vcpu *vcpu) > { > atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags); > > If you get stalled here and the target VCPU handles the request and > reenters SIE in the meantime, then you'll wait until its next exit. > (And miss an unbounded amount of exits in the worst case.) > > while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE) > cpu_relax(); > } > > And out of curiosity -- how many cycles does this loop usually take? > > > 2. Remote requests that don't need a sync > > > > E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while > > KVM_REQ_DISABLE_IBS does. > > A usual KVM request would kick the VCPU out of nested virt as well. > Shouldn't it be done for these as well? > > > 3. local requests > > > > E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() > > > > > > Of course, having a unified interface would be better. > > > > /* set the request and kick the CPU out of guest mode */ > > kvm_set_request(req, vcpu); > > > > /* set the request, kick the CPU out of guest mode, wait until guest > > mode has been left and make sure the request will be handled before > > reentering guest mode */ > > kvm_set_sync_request(req, vcpu); > > Sounds good, I'll also add > > kvm_set_self_request(req, vcpu); > > > Same maybe even for multiple VCPUs (as there are then ways to speed it > > up, e.g. first kick all, then wait for all) > > > > This would require arch specific callbacks to > > 1. pre announce the request (e.g. set PROG_REQUEST on s390x) > > 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later > > kvm_s390_vsie_kick(vcpu) on s390x) > > 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) > > > > This would only make sense if there are other use cases for sync > > requests. At least I remember that Power also has a faster way for > > kicking VCPUs, not involving SMP rescheds. I can't judge if this is a > > s390x only thing and is better be left as is :) > > > > At least vcpu_kick() could be quite easily made to work on s390x. > > > > Radim, are there also other users that need something like sync requests? > > I think that ARM has a similar need when updating vgic, but relies on an > asumption that VCPUs are going to be out after kicking them with > kvm_make_all_cpus_request(). > (vgic_change_active_prepare in virt/kvm/arm/vgic/vgic-mmio.c) Yes, we have similar needs. We don't actually use the requests infrastructure in the moment (although I have plans to move to that following a long series of optimization patches I have stashed on my machine), but we reuse the kvm_make_all_cpus_request function to figure out which CPUs need a kick, and which don't, instead of duplicating this logic in the ARM tree. > > Having synchronous requests in a common API should probably wait for the > completion of the request, not just for the kick, which would make race > handling simpler. > > I'm not going to worry about them in this pass, though. > I'll be happy to help working on this or at least reviewing stuff to move our home-baked "stop all VCPUs and wait for something before entering the guest again" functionality to common functionality that uses requests. Thanks, -Christoffer ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-24 11:34 ` Christoffer Dall @ 2017-02-24 12:46 ` Andrew Jones 0 siblings, 0 replies; 31+ messages in thread From: Andrew Jones @ 2017-02-24 12:46 UTC (permalink / raw) To: Christoffer Dall Cc: Radim Krčmář, David Hildenbrand, Christian Borntraeger, linux-kernel, kvm, Paolo Bonzini, Marc Zyngier, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On Fri, Feb 24, 2017 at 12:34:07PM +0100, Christoffer Dall wrote: > On Wed, Feb 22, 2017 at 04:17:05PM +0100, Radim Krčmář wrote: > > [Oops, the end of this thread got dragged into a mark-as-read spree ...] > > > > 2017-02-17 11:13+0100, David Hildenbrand: > > >>> This is really complicated stuff, and the basic reason for it (if I > > >>> remember correctly) is that s390x does reenable all interrupts when > > >>> entering the sie (see kvm-s390.c:__vcpu_run()). So the fancy smp-based > > >>> kicks don't work (as it is otherwise just racy), and if I remember > > >>> correctly, SMP reschedule signals (s390x external calls) would be > > >>> slower. (Christian, please correct me if I'm wrong) > > >> > > >> No the reason was that there are some requests that need to be handled > > >> outside run SIE. For example one reason was the guest prefix page. > > >> This must be mapped read/write ALL THE TIME when a guest is running, > > >> otherwise the host might crash. So we have to exit SIE and make sure that > > >> it does not reenter, therefore we use the RELOAD_MMU request from a notifier > > >> that is called from page table functions, whenever memory management decides > > >> to unmap/write protect (dirty pages tracking, reference tracking, page migration > > >> or compaction...) > > >> > > >> SMP-based request wills kick out the guest, but for some thing like the > > >> one above it will be too late. > > > > > > While what you said is 100% correct, I had something else in mind that > > > hindered using vcpu_kick() and especially kvm_make_all_cpus_request(). > > > And I remember that being related to how preemption and > > > OUTSIDE_GUEST_MODE is handled. I think this boils down to what would > > > have to be implemented in kvm_arch_vcpu_should_kick(). > > > > > > x86 can track the guest state using vcpu->mode, because they can be sure > > > that the guest can't reschedule while in the critical guest entry/exit > > > section. This is not true for s390x, as preemption is enabled. That's > > > why vcpu->mode cannot be used in its current form to track if a VCPU is > > > in/oustide/exiting guest mode. And kvm_make_all_cpus_request() currently > > > relies on this setting. > > > > > > For now, calling vcpu_kick() on s390x will result in a BUG(). > > > > > > > > > On s390x, there are 3 use cases I see for requests: > > > > > > 1. Remote requests that need a sync > > > > > > Make a request, wait until SIE has been left and make sure the request > > > will be processed before re-entering the SIE. e.g. KVM_REQ_RELOAD_MMU > > > notifier in mmu notifier you mentioned. Also KVM_REQ_DISABLE_IBS is a > > > candidate. > > > > Btw. aren't those requests racy? > > > > void exit_sie(struct kvm_vcpu *vcpu) > > { > > atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags); > > > > If you get stalled here and the target VCPU handles the request and > > reenters SIE in the meantime, then you'll wait until its next exit. > > (And miss an unbounded amount of exits in the worst case.) > > > > while (vcpu->arch.sie_block->prog0c & PROG_IN_SIE) > > cpu_relax(); > > } > > > > And out of curiosity -- how many cycles does this loop usually take? > > > > > 2. Remote requests that don't need a sync > > > > > > E.g. KVM_REQ_ENABLE_IBS doesn't strictly need it, while > > > KVM_REQ_DISABLE_IBS does. > > > > A usual KVM request would kick the VCPU out of nested virt as well. > > Shouldn't it be done for these as well? > > > > > 3. local requests > > > > > > E.g. KVM_REQ_TLB_FLUSH from kvm_s390_set_prefix() > > > > > > > > > Of course, having a unified interface would be better. > > > > > > /* set the request and kick the CPU out of guest mode */ > > > kvm_set_request(req, vcpu); > > > > > > /* set the request, kick the CPU out of guest mode, wait until guest > > > mode has been left and make sure the request will be handled before > > > reentering guest mode */ > > > kvm_set_sync_request(req, vcpu); > > > > Sounds good, I'll also add > > > > kvm_set_self_request(req, vcpu); > > > > > Same maybe even for multiple VCPUs (as there are then ways to speed it > > > up, e.g. first kick all, then wait for all) > > > > > > This would require arch specific callbacks to > > > 1. pre announce the request (e.g. set PROG_REQUEST on s390x) > > > 2. kick the cpu (e.g. CPUSTAT_STOP_INT and later > > > kvm_s390_vsie_kick(vcpu) on s390x) > > > 3. check if still executing the guest (e.g. PROG_IN_SIE on s390x) > > > > > > This would only make sense if there are other use cases for sync > > > requests. At least I remember that Power also has a faster way for > > > kicking VCPUs, not involving SMP rescheds. I can't judge if this is a > > > s390x only thing and is better be left as is :) > > > > > > At least vcpu_kick() could be quite easily made to work on s390x. > > > > > > Radim, are there also other users that need something like sync requests? > > > > I think that ARM has a similar need when updating vgic, but relies on an > > asumption that VCPUs are going to be out after kicking them with > > kvm_make_all_cpus_request(). > > (vgic_change_active_prepare in virt/kvm/arm/vgic/vgic-mmio.c) > > Yes, we have similar needs. We don't actually use the requests > infrastructure in the moment (although I have plans to move to that > following a long series of optimization patches I have stashed on my > machine), but we reuse the kvm_make_all_cpus_request function to figure > out which CPUs need a kick, and which don't, instead of duplicating this > logic in the ARM tree. I also have patches (that I was going to post a week ago, but then decided to base on Radim's work) that bring vcpu-requests to arm. The patches I have are to plug a few races, most notably ones in PSCI emulation. You may recall an off-list discussion we had about those races that took place roughly one million years ago. Indeed it was your suggestion at the time to bring vcpu-requests to arm. I'll still post those patches shortly. I think Radim plans to post a v2 soon, so I'll rebase on that. > > > > > Having synchronous requests in a common API should probably wait for the > > completion of the request, not just for the kick, which would make race > > handling simpler. > > > > I'm not going to worry about them in this pass, though. > > > > I'll be happy to help working on this or at least reviewing stuff to > move our home-baked "stop all VCPUs and wait for something before > entering the guest again" functionality to common functionality that > uses requests. > Thanks, drew ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-16 16:04 ` [PATCH 4/5] KVM: add __kvm_request_needs_mb Radim Krčmář 2017-02-16 19:49 ` David Hildenbrand @ 2017-02-23 11:01 ` Paolo Bonzini 2017-02-23 15:52 ` Radim Krčmář 1 sibling, 1 reply; 31+ messages in thread From: Paolo Bonzini @ 2017-02-23 11:01 UTC (permalink / raw) To: Radim Krčmář, linux-kernel, kvm Cc: Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On 16/02/2017 17:04, Radim Krčmář wrote: > A macro to optimize requests that do not need a memory barrier because > they have no dependencies. An architecture can implement a function > that says which requests do not need memory barriers when handling them. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> I would leave this for a separate series, otherwise looks nice (though I was skeptical at first ;)). Paolo > --- > include/linux/kvm_host.h | 41 +++++++++++++++++++++++++++++++++++++---- > virt/kvm/kvm_main.c | 3 ++- > 2 files changed, 39 insertions(+), 5 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index d899473859d3..2cc438685af8 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1097,8 +1097,8 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) > * 2) remote request with no data (= kick) > * 3) remote request with data (= kick + mb) > * > - * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but > - * forces smp_wmb() for all requests. > + * TODO: the API does not distinguish local and remote requests -- remote > + * should contain kvm_vcpu_kick(). > */ > > static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) > @@ -1106,6 +1106,37 @@ static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) > set_bit(req, &vcpu->requests); > } > > +/* > + * __kvm_request_needs_mb is used to improve performance, so it should have no > + * runtime overhead. > + */ > +static inline bool __kvm_request_needs_mb(int req) > +{ > + /* > + * This barrier lets callers avoid the following pattern: > + * if (__kvm_request_needs_mb(req)) > + * ... > + * else > + * barrier(); > + */ > + barrier(); > + > + if (!__builtin_constant_p(req)) > + return true; > + > +#ifdef kvm_arch_request_needs_mb > + /* > + * GCC optimizes pure kvm_arch_request_needs_mb() with a constant input > + * into a contant, but __builtin_constant_p() is not so clever, so we > + * cannot ensure that with: > + * BUILD_BUG_ON(!__builtin_constant_p(kvm_arch_request_needs_mb(req))); > + */ > + return kvm_arch_request_needs_mb(req); > +#else > + return true; > +#endif > +} > + > static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) > { > /* > @@ -1113,7 +1144,8 @@ static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) > * kvm_request_test_and_clear's caller. > * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. > */ > - smp_wmb(); > + if (__kvm_request_needs_mb(req)) > + smp_wmb(); > __kvm_request_set(req, vcpu); > } > > @@ -1137,7 +1169,8 @@ static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcp > * kvm_request_test_and_clear's caller. > * Paired with the smp_wmb in kvm_request_set. > */ > - smp_mb__after_atomic(); > + if (__kvm_request_needs_mb(req)) > + smp_mb__after_atomic(); > return true; > } else { > return false; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2250920ec965..ced3e4cb1df0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -179,7 +179,8 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) > me = get_cpu(); > > /* Paired with the smp_mb__after_atomic in kvm_request_test_and_clear. */ > - smp_wmb(); > + if (__kvm_request_needs_mb(req)) > + smp_wmb(); > > kvm_for_each_vcpu(i, vcpu, kvm) { > __kvm_request_set(req, vcpu); > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] KVM: add __kvm_request_needs_mb 2017-02-23 11:01 ` Paolo Bonzini @ 2017-02-23 15:52 ` Radim Krčmář 0 siblings, 0 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-23 15:52 UTC (permalink / raw) To: Paolo Bonzini Cc: linux-kernel, kvm, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall 2017-02-23 12:01+0100, Paolo Bonzini: > On 16/02/2017 17:04, Radim Krčmář wrote: >> A macro to optimize requests that do not need a memory barrier because >> they have no dependencies. An architecture can implement a function >> that says which requests do not need memory barriers when handling them. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > I would leave this for a separate series, otherwise looks nice (though I > was skeptical at first ;)). Ok, I'll first post just the renaming and base other ideas from related thread on it. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 5/5] KVM: add kvm_request_pending 2017-02-16 16:04 [PATCH 0/5] KVM: rename and extend vcpu->requests API Radim Krčmář ` (3 preceding siblings ...) 2017-02-16 16:04 ` [PATCH 4/5] KVM: add __kvm_request_needs_mb Radim Krčmář @ 2017-02-16 16:04 ` Radim Krčmář 2017-02-16 19:50 ` David Hildenbrand 2017-02-17 9:51 ` Andrew Jones 4 siblings, 2 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-16 16:04 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall Just to complete the encapsulation. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- arch/mips/kvm/trap_emul.c | 2 +- arch/powerpc/kvm/booke.c | 2 +- arch/powerpc/kvm/powerpc.c | 4 ++-- arch/s390/kvm/kvm-s390.c | 2 +- arch/x86/kvm/x86.c | 4 ++-- include/linux/kvm_host.h | 5 +++++ 6 files changed, 12 insertions(+), 7 deletions(-) diff --git a/arch/mips/kvm/trap_emul.c b/arch/mips/kvm/trap_emul.c index 35068823cde6..59e121343170 100644 --- a/arch/mips/kvm/trap_emul.c +++ b/arch/mips/kvm/trap_emul.c @@ -1029,7 +1029,7 @@ static void kvm_trap_emul_check_requests(struct kvm_vcpu *vcpu, int cpu, struct mm_struct *mm; int i; - if (likely(!vcpu->requests)) + if (likely(!kvm_request_pending(vcpu))) return; if (kvm_request_test_and_clear(KVM_REQ_TLB_FLUSH, vcpu)) { diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index e9098af0ab2a..54d2d1cca514 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -682,7 +682,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu) kvmppc_core_check_exceptions(vcpu); - if (vcpu->requests) { + if (kvm_request_pending(vcpu)) { /* Exception delivery raised request; start over */ return 1; } diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index f570ca9cd8c6..f92d82382da3 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -52,7 +52,7 @@ EXPORT_SYMBOL_GPL(kvmppc_pr_ops); int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) { return !!(v->arch.pending_exceptions) || - v->requests; + kvm_request_pending(v); } int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu) @@ -104,7 +104,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu) */ smp_mb(); - if (vcpu->requests) { + if (kvm_request_pending(vcpu)) { /* Make sure we process requests preemptable */ local_irq_enable(); trace_kvm_check_requests(vcpu); diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 0fa77e6fdfaf..f2625da20e38 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2393,7 +2393,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) { retry: kvm_s390_vcpu_request_handled(vcpu); - if (!vcpu->requests) + if (!kvm_request_pending(vcpu)) return 0; /* * We use MMU_RELOAD just to re-arm the ipte notifier for the diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 6e8c62c29a5a..4f325b4eb2cd 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6723,7 +6723,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) bool req_immediate_exit = false; - if (vcpu->requests) { + if (kvm_request_pending(vcpu)) { if (kvm_request_test_and_clear(KVM_REQ_MMU_RELOAD, vcpu)) kvm_mmu_unload(vcpu); if (kvm_request_test_and_clear(KVM_REQ_MIGRATE_TIMER, vcpu)) @@ -6887,7 +6887,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) kvm_x86_ops->sync_pir_to_irr(vcpu); } - if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests + if (vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu) || need_resched() || signal_pending(current)) { vcpu->mode = OUTSIDE_GUEST_MODE; smp_wmb(); diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 2cc438685af8..563cf964dc5c 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1101,6 +1101,11 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) * should contain kvm_vcpu_kick(). */ +static inline bool kvm_request_pending(struct kvm_vcpu *vcpu) +{ + return vcpu->requests; +} + static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) { set_bit(req, &vcpu->requests); -- 2.11.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] KVM: add kvm_request_pending 2017-02-16 16:04 ` [PATCH 5/5] KVM: add kvm_request_pending Radim Krčmář @ 2017-02-16 19:50 ` David Hildenbrand 2017-02-17 9:51 ` Andrew Jones 1 sibling, 0 replies; 31+ messages in thread From: David Hildenbrand @ 2017-02-16 19:50 UTC (permalink / raw) To: Radim Krčmář, linux-kernel, kvm Cc: Paolo Bonzini, Andrew Jones, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall Am 16.02.2017 um 17:04 schrieb Radim Krčmář: > Just to complete the encapsulation. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- Reviewed-by: David Hildenbrand <david@redhat.com> -- Thanks, David ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] KVM: add kvm_request_pending 2017-02-16 16:04 ` [PATCH 5/5] KVM: add kvm_request_pending Radim Krčmář 2017-02-16 19:50 ` David Hildenbrand @ 2017-02-17 9:51 ` Andrew Jones 2017-02-17 14:59 ` Radim Krčmář 1 sibling, 1 reply; 31+ messages in thread From: Andrew Jones @ 2017-02-17 9:51 UTC (permalink / raw) To: Radim Krčmář Cc: linux-kernel, kvm, Paolo Bonzini, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall On Thu, Feb 16, 2017 at 05:04:49PM +0100, Radim Krčmář wrote: ... > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 2cc438685af8..563cf964dc5c 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1101,6 +1101,11 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) > * should contain kvm_vcpu_kick(). > */ > > +static inline bool kvm_request_pending(struct kvm_vcpu *vcpu) > +{ > + return vcpu->requests; How about wrapping this with READ_ONCE for good measure? > +} > + > static inline void __kvm_request_set(unsigned req, struct kvm_vcpu *vcpu) > { > set_bit(req, &vcpu->requests); > -- > 2.11.1 > Thanks, drew ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] KVM: add kvm_request_pending 2017-02-17 9:51 ` Andrew Jones @ 2017-02-17 14:59 ` Radim Krčmář 0 siblings, 0 replies; 31+ messages in thread From: Radim Krčmář @ 2017-02-17 14:59 UTC (permalink / raw) To: Andrew Jones Cc: linux-kernel, kvm, Paolo Bonzini, Marc Zyngier, Christian Borntraeger, Cornelia Huck, James Hogan, Paul Mackerras, Christoffer Dall 2017-02-17 10:51+0100, Andrew Jones: > On Thu, Feb 16, 2017 at 05:04:49PM +0100, Radim Krčmář wrote: > ... >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index 2cc438685af8..563cf964dc5c 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -1101,6 +1101,11 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args) >> * should contain kvm_vcpu_kick(). >> */ >> >> +static inline bool kvm_request_pending(struct kvm_vcpu *vcpu) >> +{ >> + return vcpu->requests; > > How about wrapping this with READ_ONCE for good measure? Sounds good; I don't think that callers would want stale values, thanks. ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2017-02-24 12:47 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-16 16:04 [PATCH 0/5] KVM: rename and extend vcpu->requests API Radim Krčmář 2017-02-16 16:04 ` [PATCH 1/5] KVM: change API for requests to match bit operations Radim Krčmář 2017-02-17 9:30 ` Cornelia Huck 2017-02-17 9:49 ` Andrew Jones 2017-02-17 9:52 ` Cornelia Huck 2017-02-17 15:01 ` Radim Krčmář 2017-02-16 16:04 ` [PATCH 2/5] KVM: add KVM request variants without barrier Radim Krčmář 2017-02-23 10:57 ` Paolo Bonzini 2017-02-23 15:50 ` Radim Krčmář 2017-02-16 16:04 ` [PATCH 3/5] KVM: optimize kvm_make_all_cpus_request Radim Krčmář 2017-02-16 16:04 ` [PATCH 4/5] KVM: add __kvm_request_needs_mb Radim Krčmář 2017-02-16 19:49 ` David Hildenbrand 2017-02-16 21:31 ` Radim Krčmář 2017-02-17 8:46 ` Christian Borntraeger 2017-02-17 10:13 ` David Hildenbrand 2017-02-17 10:19 ` Christian Borntraeger 2017-02-17 11:28 ` Christian Borntraeger 2017-02-22 15:17 ` Radim Krčmář 2017-02-22 19:23 ` Christian Borntraeger 2017-02-23 15:43 ` Radim Krčmář 2017-02-22 19:57 ` Christian Borntraeger 2017-02-23 10:20 ` David Hildenbrand 2017-02-23 15:39 ` Radim Krčmář 2017-02-24 11:34 ` Christoffer Dall 2017-02-24 12:46 ` Andrew Jones 2017-02-23 11:01 ` Paolo Bonzini 2017-02-23 15:52 ` Radim Krčmář 2017-02-16 16:04 ` [PATCH 5/5] KVM: add kvm_request_pending Radim Krčmář 2017-02-16 19:50 ` David Hildenbrand 2017-02-17 9:51 ` Andrew Jones 2017-02-17 14:59 ` Radim Krčmář
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.