[PATCH v2 0/9] KVM: towards maintainable kvm_make_all_cpus_request()

[PATCH v2 0/9] KVM: towards maintainable kvm_make_all_cpus_request()

[Radim Krčmář]

v2 doesn't use kvm_arch_vcpu_should_kick() and hence avoids the big bug
discovered by James.  Instead, the last just exposes the synchronization
behavior and prepares it for future merging with
kvm_arch_vcpu_should_kick().

v2 also constains two patches from Andrew that applied without any
changes and a simple optimization for wakeups.

v1: http://www.spinics.net/lists/kvm/msg147898.html


Andrew Jones (2):
  KVM: add explicit barrier to kvm_vcpu_kick
  KVM: improve arch vcpu request defining

Radim Krčmář (7):
  KVM: add kvm_{test,clear}_request to replace {test,clear}_bit
  KVM: x86: always use kvm_make_request instead of set_bit
  KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up
  KVM: mark requests that do not need a wakeup
  KVM: perform a wake_up in kvm_make_all_cpus_request
  KVM: return if kvm_vcpu_wake_up() did wake up the VCPU
  KVM: mark requests that need synchronization

 arch/arm/include/asm/kvm_host.h     |  2 +-
 arch/arm64/include/asm/kvm_host.h   |  2 +-
 arch/mips/kvm/emulate.c             |  2 +-
 arch/powerpc/include/asm/kvm_host.h |  4 ++--
 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/include/asm/kvm_host.h    |  6 ++---
 arch/s390/kvm/kvm-s390.c            |  2 +-
 arch/x86/include/asm/kvm_host.h     | 44 ++++++++++++++++++-------------------
 arch/x86/kvm/vmx.c                  |  2 +-
 arch/x86/kvm/x86.c                  | 20 ++++++-----------
 include/linux/kvm_host.h            | 42 ++++++++++++++++++++++++++++++-----
 virt/kvm/kvm_main.c                 | 30 +++++++++++++++++--------
 15 files changed, 101 insertions(+), 65 deletions(-)

-- 
2.12.2

[PATCH v2 1/9] KVM: add kvm_{test,clear}_request to replace {test,clear}_bit

[Radim Krčmář]

Users were expected to use kvm_check_request() for testing and clearing,
but request have expanded their use since then and some users want to
only test or do a faster clear.

Make sure that requests are not directly accessed with bit operations.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
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                |  8 ++++----
 include/linux/kvm_host.h          | 14 ++++++++++++--
 9 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
index 34e78a3ee9d7..4144bfaef137 100644
--- a/arch/mips/kvm/emulate.c
+++ b/arch/mips/kvm/emulate.c
@@ -982,7 +982,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu)
 		 * check if any I/O interrupts are pending.
 		 */
 		if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
-			clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+			kvm_clear_request(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 f026b062c0ed..69a09444d46e 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_clear_request(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..bcbeeb62dd13 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_clear_request(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 3c296c2eacf8..3eaac3809977 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -584,7 +584,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_clear_request(KVM_REQ_WATCHDOG, vcpu);
 
 	spin_lock_irqsave(&vcpu->arch.wdt_lock, flags);
 	nr_jiffies = watchdog_next_timeout(vcpu);
@@ -695,7 +695,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_clear_request(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 cf725c580fc5..1ee22a910074 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -233,7 +233,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_clear_request(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 8771fef112a1..71a5f4023f3f 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2492,7 +2492,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_clear_request(KVM_REQ_UNHALT, vcpu);
 
 	return 0;
 }
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a4ef63718101..b003b8dfb20c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6299,7 +6299,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_test_request(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 f68c5b2ba627..2de54c20fa9e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1753,7 +1753,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_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
 
 	spin_unlock(&ka->pvclock_gtod_sync_lock);
 #endif
@@ -7041,7 +7041,7 @@ static int vcpu_run(struct kvm_vcpu *vcpu)
 		if (r <= 0)
 			break;
 
-		clear_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests);
+		kvm_clear_request(KVM_REQ_PENDING_TIMER, vcpu);
 		if (kvm_cpu_has_pending_timer(vcpu))
 			kvm_inject_pending_timer_irqs(vcpu);
 
@@ -7169,7 +7169,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_clear_request(KVM_REQ_UNHALT, vcpu);
 		r = -EAGAIN;
 		goto out;
 	}
@@ -8382,7 +8382,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_test_request(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 397b7b5b1933..374fa92c7657 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1079,10 +1079,20 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
 	set_bit(req, &vcpu->requests);
 }
 
+static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
+{
+	return test_bit(req, &vcpu->requests);
+}
+
+static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
+{
+	clear_bit(req, &vcpu->requests);
+}
+
 static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
 {
-	if (test_bit(req, &vcpu->requests)) {
-		clear_bit(req, &vcpu->requests);
+	if (kvm_test_request(req, vcpu)) {
+		kvm_clear_request(req, vcpu);
 
 		/*
 		 * Ensure the rest of the request is visible to kvm_check_request's
-- 
2.12.2

[PATCH v2 2/9] KVM: x86: always use kvm_make_request instead of set_bit

[Radim Krčmář]

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/x86.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2de54c20fa9e..0936c3e2e51c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2203,8 +2203,7 @@ 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_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
 
 			ka->boot_vcpu_runs_old_kvmclock = tmp;
 		}
@@ -2802,11 +2801,6 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 	return kvm_arch_has_noncoherent_dma(vcpu->kvm);
 }
 
-static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu)
-{
-	set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests);
-}
-
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 	/* Address WBINVD may be executed by guest */
@@ -2850,7 +2844,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		if (!vcpu->kvm->arch.use_master_clock || vcpu->cpu == -1)
 			kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
 		if (vcpu->cpu != cpu)
-			kvm_migrate_timers(vcpu);
+			kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu);
 		vcpu->cpu = cpu;
 	}
 
-- 
2.12.2

[PATCH v2 3/9] KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up

[Radim Krčmář]

The #ifndef was protecting a missing halt_wakeup stat, but that is no
longer necessary.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 virt/kvm/kvm_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 357e67cba32e..e5d52b46b531 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2195,7 +2195,6 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_block);
 
-#ifndef CONFIG_S390
 void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wqp;
@@ -2225,7 +2224,6 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 	put_cpu();
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
-#endif /* !CONFIG_S390 */
 
 int kvm_vcpu_yield_to(struct kvm_vcpu *target)
 {
-- 
2.12.2

[PATCH v2 4/9] KVM: mark requests that do not need a wakeup

[Radim Krčmář]

Some operations must ensure that the guest is not running with stale
data, but if the guest is halted, then the update can wait until another
event happens.  kvm_make_all_requests() currently doesn't wake up, so we
can mark all requests used with it.

First 8 bits were arbitrarily reserved for request numbers.

Most uses of requests have the request type as a constant, so a compiler
will optimize the '&'.

An alternative would be to have an inline function that would return
whether the request needs a wake-up or not, but I like this one better
even though it might produce worse assembly.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: use GENMASK [Marc]
---
 arch/arm/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/kvm_host.h |  2 +-
 arch/x86/include/asm/kvm_host.h   |  6 +++---
 include/linux/kvm_host.h          | 12 +++++++-----
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index de67ce647501..49358f20d36f 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -44,7 +44,7 @@
 #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
 #endif
 
-#define KVM_REQ_VCPU_EXIT	8
+#define KVM_REQ_VCPU_EXIT	(8 | KVM_REQUEST_NO_WAKEUP)
 
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
 int __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 522e4f60976e..1c9458a7ec92 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -41,7 +41,7 @@
 
 #define KVM_VCPU_MAX_FEATURES 4
 
-#define KVM_REQ_VCPU_EXIT	8
+#define KVM_REQ_VCPU_EXIT	(8 | KVM_REQUEST_NO_WAKEUP)
 
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f5c942edbc86..19219826bed6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -61,10 +61,10 @@
 #define KVM_REQ_PMI               19
 #define KVM_REQ_SMI               20
 #define KVM_REQ_MASTERCLOCK_UPDATE 21
-#define KVM_REQ_MCLOCK_INPROGRESS 22
-#define KVM_REQ_SCAN_IOAPIC       23
+#define KVM_REQ_MCLOCK_INPROGRESS (22 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_SCAN_IOAPIC       (23 | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_GLOBAL_CLOCK_UPDATE 24
-#define KVM_REQ_APIC_PAGE_RELOAD  25
+#define KVM_REQ_APIC_PAGE_RELOAD  (25 | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_HV_CRASH          26
 #define KVM_REQ_IOAPIC_EOI_EXIT   27
 #define KVM_REQ_HV_RESET          28
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 374fa92c7657..a805ddcb7eb0 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -115,12 +115,14 @@ static inline bool is_error_page(struct page *page)
 	return IS_ERR(page);
 }
 
+#define KVM_REQUEST_MASK           GENMASK(7,0)
+#define KVM_REQUEST_NO_WAKEUP      BIT(8)
 /*
  * Architecture-independent vcpu->requests bit members
  * Bits 4-7 are reserved for more arch-independent bits.
  */
-#define KVM_REQ_TLB_FLUSH          0
-#define KVM_REQ_MMU_RELOAD         1
+#define KVM_REQ_TLB_FLUSH          (0 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_MMU_RELOAD         (1 | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_PENDING_TIMER      2
 #define KVM_REQ_UNHALT             3
 
@@ -1076,17 +1078,17 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
 	 * caller.  Paired with the smp_mb__after_atomic in kvm_check_request.
 	 */
 	smp_wmb();
-	set_bit(req, &vcpu->requests);
+	set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
 }
 
 static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
 {
-	return test_bit(req, &vcpu->requests);
+	return test_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
 }
 
 static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
 {
-	clear_bit(req, &vcpu->requests);
+	clear_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
 }
 
 static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
-- 
2.12.2

[PATCH v2 5/9] KVM: perform a wake_up in kvm_make_all_cpus_request

[Radim Krčmář]

We want to have kvm_make_all_cpus_request() to be an optmized version of

  kvm_for_each_vcpu(i, vcpu, kvm) {
    kvm_make_request(vcpu, request);
    kvm_vcpu_kick(vcpu);
  }

and kvm_vcpu_kick() wakes up the target vcpu.  We know which requests do
not need the wake up and use it to optimize the loop.

Thanks to that, this patch doesn't change the behavior of current users
(the all don't need the wake up) and only prepares for future where the
wake up is going to be needed.

I think that most requests do not need the wake up, so we would flip the
bit then.

kvm_vcpu_kick() will get this condition after it is merged with
kvm_make_request() because we currently don't know which request is being
kicked.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 virt/kvm/kvm_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e5d52b46b531..3772f7dcc72d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		/* Set ->requests bit before we read ->mode. */
 		smp_mb__after_atomic();
 
+		if (!(req & KVM_REQUEST_NO_WAKEUP))
+			kvm_vcpu_wake_up(vcpu);
+
 		if (cpus != NULL && cpu != -1 && cpu != me &&
 		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
 			cpumask_set_cpu(cpu, cpus);
-- 
2.12.2

[PATCH v2 6/9] KVM: add explicit barrier to kvm_vcpu_kick

[Radim Krčmář]

From: Andrew Jones <drjones@redhat.com>

kvm_vcpu_kick() must issue a general memory barrier prior to reading
vcpu->mode in order to ensure correctness of the mutual-exclusion
memory barrier pattern used with vcpu->requests.  While the cmpxchg
called from kvm_vcpu_kick():

 kvm_vcpu_kick
   kvm_arch_vcpu_should_kick
     kvm_vcpu_exiting_guest_mode
       cmpxchg

implies general memory barriers before and after the operation, that
implication is only valid when cmpxchg succeeds.  We need an explicit
barrier for when it fails, otherwise a VCPU thread on its entry path
that reads zero for vcpu->requests does not exclude the possibility
the requesting thread sees !IN_GUEST_MODE when it reads vcpu->mode.

kvm_make_all_cpus_request already had a barrier, so we remove it, as
now it would be redundant.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/x86/kvm/x86.c       | 2 +-
 include/linux/kvm_host.h | 6 ++++++
 virt/kvm/kvm_main.c      | 3 ---
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0936c3e2e51c..69fcee26f4da 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6853,7 +6853,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	/*
 	 * 1) We should set ->mode before checking ->requests.  Please see
-	 * the comment in kvm_make_all_cpus_request.
+	 * the comment in kvm_vcpu_exiting_guest_mode().
 	 *
 	 * 2) For APICv, we should set ->mode before checking PIR.ON.  This
 	 * pairs with the memory barrier implicit in pi_test_and_set_on
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a805ddcb7eb0..84c5396564f7 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -270,6 +270,12 @@ struct kvm_vcpu {
 
 static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
 {
+	/*
+	 * The memory barrier ensures a previous write to vcpu->requests cannot
+	 * be reordered with the read of vcpu->mode.  It pairs with the general
+	 * memory barrier following the write of vcpu->mode in VCPU RUN.
+	 */
+	smp_mb__before_atomic();
 	return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE);
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3772f7dcc72d..1efb07643035 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -183,9 +183,6 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		kvm_make_request(req, vcpu);
 		cpu = vcpu->cpu;
 
-		/* Set ->requests bit before we read ->mode. */
-		smp_mb__after_atomic();
-
 		if (!(req & KVM_REQUEST_NO_WAKEUP))
 			kvm_vcpu_wake_up(vcpu);
 
-- 
2.12.2

[PATCH v2 7/9] KVM: improve arch vcpu request defining

[Radim Krčmář]

From: Andrew Jones <drjones@redhat.com>

Marc Zyngier suggested that we define the arch specific VCPU request
base, rather than requiring each arch to remember to start from 8.
That suggestion, along with Radim Krčmář's recent VCPU request flag
addition, snowballed into defining something of an arch VCPU request
defining API.

No functional change.

(Looks like x86 is running out of arch VCPU request bits.  Maybe
 someday we'll need to extend to 64.)

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/arm/include/asm/kvm_host.h     |  2 +-
 arch/arm64/include/asm/kvm_host.h   |  2 +-
 arch/powerpc/include/asm/kvm_host.h |  4 ++--
 arch/s390/include/asm/kvm_host.h    |  6 ++---
 arch/x86/include/asm/kvm_host.h     | 44 ++++++++++++++++++-------------------
 include/linux/kvm_host.h            |  8 +++++++
 6 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 49358f20d36f..1d48a4b65b86 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -44,7 +44,7 @@
 #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
 #endif
 
-#define KVM_REQ_VCPU_EXIT	(8 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_NO_WAKEUP(0)
 
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
 int __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1c9458a7ec92..d3370b79660e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -41,7 +41,7 @@
 
 #define KVM_VCPU_MAX_FEATURES 4
 
-#define KVM_REQ_VCPU_EXIT	(8 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_NO_WAKEUP(0)
 
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 77c60826d145..51b4fda269c9 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -52,8 +52,8 @@
 #define KVM_IRQCHIP_NUM_PINS     256
 
 /* PPC-specific vcpu->requests bit members */
-#define KVM_REQ_WATCHDOG           8
-#define KVM_REQ_EPR_EXIT           9
+#define KVM_REQ_WATCHDOG	KVM_ARCH_REQ(0)
+#define KVM_REQ_EPR_EXIT	KVM_ARCH_REQ(1)
 
 #include <linux/mmu_notifier.h>
 
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 426614a882a9..9c3bd94204ac 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -42,9 +42,9 @@
 #define KVM_HALT_POLL_NS_DEFAULT 80000
 
 /* s390-specific vcpu->requests bit members */
-#define KVM_REQ_ENABLE_IBS         8
-#define KVM_REQ_DISABLE_IBS        9
-#define KVM_REQ_ICPT_OPEREXC       10
+#define KVM_REQ_ENABLE_IBS	KVM_ARCH_REQ(0)
+#define KVM_REQ_DISABLE_IBS	KVM_ARCH_REQ(1)
+#define KVM_REQ_ICPT_OPEREXC	KVM_ARCH_REQ(2)
 
 #define SIGP_CTRL_C		0x80
 #define SIGP_CTRL_SCN_MASK	0x3f
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 19219826bed6..15eb7d3837e3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -48,28 +48,28 @@
 #define KVM_IRQCHIP_NUM_PINS  KVM_IOAPIC_NUM_PINS
 
 /* x86-specific vcpu->requests bit members */
-#define KVM_REQ_MIGRATE_TIMER      8
-#define KVM_REQ_REPORT_TPR_ACCESS  9
-#define KVM_REQ_TRIPLE_FAULT      10
-#define KVM_REQ_MMU_SYNC          11
-#define KVM_REQ_CLOCK_UPDATE      12
-#define KVM_REQ_EVENT             14
-#define KVM_REQ_APF_HALT          15
-#define KVM_REQ_STEAL_UPDATE      16
-#define KVM_REQ_NMI               17
-#define KVM_REQ_PMU               18
-#define KVM_REQ_PMI               19
-#define KVM_REQ_SMI               20
-#define KVM_REQ_MASTERCLOCK_UPDATE 21
-#define KVM_REQ_MCLOCK_INPROGRESS (22 | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_SCAN_IOAPIC       (23 | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_GLOBAL_CLOCK_UPDATE 24
-#define KVM_REQ_APIC_PAGE_RELOAD  (25 | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_HV_CRASH          26
-#define KVM_REQ_IOAPIC_EOI_EXIT   27
-#define KVM_REQ_HV_RESET          28
-#define KVM_REQ_HV_EXIT           29
-#define KVM_REQ_HV_STIMER         30
+#define KVM_REQ_MIGRATE_TIMER		KVM_ARCH_REQ(0)
+#define KVM_REQ_REPORT_TPR_ACCESS	KVM_ARCH_REQ(1)
+#define KVM_REQ_TRIPLE_FAULT		KVM_ARCH_REQ(2)
+#define KVM_REQ_MMU_SYNC		KVM_ARCH_REQ(3)
+#define KVM_REQ_CLOCK_UPDATE		KVM_ARCH_REQ(4)
+#define KVM_REQ_EVENT			KVM_ARCH_REQ(6)
+#define KVM_REQ_APF_HALT		KVM_ARCH_REQ(7)
+#define KVM_REQ_STEAL_UPDATE		KVM_ARCH_REQ(8)
+#define KVM_REQ_NMI			KVM_ARCH_REQ(9)
+#define KVM_REQ_PMU			KVM_ARCH_REQ(10)
+#define KVM_REQ_PMI			KVM_ARCH_REQ(11)
+#define KVM_REQ_SMI			KVM_ARCH_REQ(12)
+#define KVM_REQ_MASTERCLOCK_UPDATE	KVM_ARCH_REQ(13)
+#define KVM_REQ_MCLOCK_INPROGRESS	KVM_ARCH_REQ_NO_WAKEUP(14)
+#define KVM_REQ_SCAN_IOAPIC		KVM_ARCH_REQ_NO_WAKEUP(15)
+#define KVM_REQ_GLOBAL_CLOCK_UPDATE	KVM_ARCH_REQ(16)
+#define KVM_REQ_APIC_PAGE_RELOAD	KVM_ARCH_REQ_NO_WAKEUP(17)
+#define KVM_REQ_HV_CRASH		KVM_ARCH_REQ(18)
+#define KVM_REQ_IOAPIC_EOI_EXIT		KVM_ARCH_REQ(19)
+#define KVM_REQ_HV_RESET		KVM_ARCH_REQ(20)
+#define KVM_REQ_HV_EXIT			KVM_ARCH_REQ(21)
+#define KVM_REQ_HV_STIMER		KVM_ARCH_REQ(22)
 
 #define CR0_RESERVED_BITS                                               \
 	(~(unsigned long)(X86_CR0_PE | X86_CR0_MP | X86_CR0_EM | X86_CR0_TS \
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 84c5396564f7..955debd82cf2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -125,6 +125,14 @@ static inline bool is_error_page(struct page *page)
 #define KVM_REQ_MMU_RELOAD         (1 | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_PENDING_TIMER      2
 #define KVM_REQ_UNHALT             3
+#define KVM_REQUEST_ARCH_BASE      8
+
+#define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
+	BUILD_BUG_ON((unsigned)(nr) >= 32 - KVM_REQUEST_ARCH_BASE); \
+	(unsigned)(((nr) + KVM_REQUEST_ARCH_BASE) | (flags)); \
+})
+#define KVM_ARCH_REQ(nr)           KVM_ARCH_REQ_FLAGS(nr, 0)
+#define KVM_ARCH_REQ_NO_WAKEUP(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP)
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID		0
 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
-- 
2.12.2

[PATCH v2 8/9] KVM: return if kvm_vcpu_wake_up() did wake up the VCPU

[Radim Krčmář]

No need to kick a VCPU that we have just woken up.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 include/linux/kvm_host.h |  2 +-
 virt/kvm/kvm_main.c      | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 955debd82cf2..38cfe372918c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -698,7 +698,7 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn);
 void kvm_vcpu_block(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu);
-void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
+bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
 int kvm_vcpu_yield_to(struct kvm_vcpu *target);
 void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1efb07643035..632f7b3e198c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -183,8 +183,8 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		kvm_make_request(req, vcpu);
 		cpu = vcpu->cpu;
 
-		if (!(req & KVM_REQUEST_NO_WAKEUP))
-			kvm_vcpu_wake_up(vcpu);
+		if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu))
+			continue;
 
 		if (cpus != NULL && cpu != -1 && cpu != me &&
 		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
@@ -2195,7 +2195,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_block);
 
-void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
+bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wqp;
 
@@ -2203,8 +2203,10 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
 	if (swait_active(wqp)) {
 		swake_up(wqp);
 		++vcpu->stat.halt_wakeup;
+		return true;
 	}
 
+	return false;
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
 
@@ -2216,7 +2218,9 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 	int me;
 	int cpu = vcpu->cpu;
 
-	kvm_vcpu_wake_up(vcpu);
+	if (kvm_vcpu_wake_up(vcpu))
+		return;
+
 	me = get_cpu();
 	if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
 		if (kvm_arch_vcpu_should_kick(vcpu))
-- 
2.12.2

[PATCH v2 9/9] KVM: mark requests that need synchronization

[Radim Krčmář]

kvm_make_all_requests() provides a synchronization that waits until all
kicked VCPUs have acknowledged the kick.  This is important for
KVM_REQ_MMU_RELOAD as it prevents freeing while lockless paging is
underway.

This patch adds the synchronization property into all requests that are
currently being used with kvm_make_all_requests() in order to preserve
the current behavior and only introduce a new framework.  Removing it
from requests where it is not necessary is left for future patches.

A question is whether this propertly isn't better expressed as a
function of the caller.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: replaces [v1 1/6]
     Ugh, KVM_ARCH_REQ_WAIT_NO_WAKEUP looks a weird ...
---
 arch/arm/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/kvm_host.h |  2 +-
 arch/x86/include/asm/kvm_host.h   |  6 +++---
 include/linux/kvm_host.h          |  8 ++++++--
 virt/kvm/kvm_main.c               | 16 +++++++++++++---
 5 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 1d48a4b65b86..2190a7ddd515 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -44,7 +44,7 @@
 #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
 #endif
 
-#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_NO_WAKEUP(0)
+#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_WAIT_NO_WAKEUP(0)
 
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
 int __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d3370b79660e..98f3d01ae91b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -41,7 +41,7 @@
 
 #define KVM_VCPU_MAX_FEATURES 4
 
-#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_NO_WAKEUP(0)
+#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_WAIT_NO_WAKEUP(0)
 
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 15eb7d3837e3..77083d7e9540 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -61,10 +61,10 @@
 #define KVM_REQ_PMI			KVM_ARCH_REQ(11)
 #define KVM_REQ_SMI			KVM_ARCH_REQ(12)
 #define KVM_REQ_MASTERCLOCK_UPDATE	KVM_ARCH_REQ(13)
-#define KVM_REQ_MCLOCK_INPROGRESS	KVM_ARCH_REQ_NO_WAKEUP(14)
-#define KVM_REQ_SCAN_IOAPIC		KVM_ARCH_REQ_NO_WAKEUP(15)
+#define KVM_REQ_MCLOCK_INPROGRESS	KVM_ARCH_REQ_WAIT_NO_WAKEUP(14)
+#define KVM_REQ_SCAN_IOAPIC		KVM_ARCH_REQ_WAIT_NO_WAKEUP(15)
 #define KVM_REQ_GLOBAL_CLOCK_UPDATE	KVM_ARCH_REQ(16)
-#define KVM_REQ_APIC_PAGE_RELOAD	KVM_ARCH_REQ_NO_WAKEUP(17)
+#define KVM_REQ_APIC_PAGE_RELOAD	KVM_ARCH_REQ_WAIT_NO_WAKEUP(17)
 #define KVM_REQ_HV_CRASH		KVM_ARCH_REQ(18)
 #define KVM_REQ_IOAPIC_EOI_EXIT		KVM_ARCH_REQ(19)
 #define KVM_REQ_HV_RESET		KVM_ARCH_REQ(20)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 38cfe372918c..a668f33b20dc 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -117,12 +117,13 @@ static inline bool is_error_page(struct page *page)
 
 #define KVM_REQUEST_MASK           GENMASK(7,0)
 #define KVM_REQUEST_NO_WAKEUP      BIT(8)
+#define KVM_REQUEST_WAIT           BIT(9)
 /*
  * Architecture-independent vcpu->requests bit members
  * Bits 4-7 are reserved for more arch-independent bits.
  */
-#define KVM_REQ_TLB_FLUSH          (0 | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_MMU_RELOAD         (1 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_TLB_FLUSH          (0 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)
+#define KVM_REQ_MMU_RELOAD         (1 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)
 #define KVM_REQ_PENDING_TIMER      2
 #define KVM_REQ_UNHALT             3
 #define KVM_REQUEST_ARCH_BASE      8
@@ -133,6 +134,9 @@ static inline bool is_error_page(struct page *page)
 })
 #define KVM_ARCH_REQ(nr)           KVM_ARCH_REQ_FLAGS(nr, 0)
 #define KVM_ARCH_REQ_NO_WAKEUP(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP)
+#define KVM_ARCH_REQ_WAIT(nr)      KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP)
+#define KVM_ARCH_REQ_WAIT_NO_WAKEUP(nr) \
+	KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID		0
 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 632f7b3e198c..dbf0410cd8b2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -165,6 +165,15 @@ void vcpu_put(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(vcpu_put);
 
+/* TODO: merge with kvm_arch_vcpu_should_kick */
+static bool kvm_should_kick_request(struct kvm_vcpu *vcpu, unsigned req)
+{
+	int mode = kvm_vcpu_exiting_guest_mode(vcpu);
+
+	return req & KVM_REQUEST_WAIT ?
+		mode != OUTSIDE_GUEST_MODE : mode == IN_GUEST_MODE;
+}
+
 static void ack_flush(void *_completed)
 {
 }
@@ -174,6 +183,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 	int i, cpu, me;
 	cpumask_var_t cpus;
 	bool called = true;
+	bool wait = req & KVM_REQUEST_WAIT;
 	struct kvm_vcpu *vcpu;
 
 	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
@@ -187,13 +197,13 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 			continue;
 
 		if (cpus != NULL && cpu != -1 && cpu != me &&
-		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
+		    kvm_should_kick_request(vcpu, req))
 			cpumask_set_cpu(cpu, cpus);
 	}
 	if (unlikely(cpus == NULL))
-		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+		smp_call_function_many(cpu_online_mask, ack_flush, NULL, wait);
 	else if (!cpumask_empty(cpus))
-		smp_call_function_many(cpus, ack_flush, NULL, 1);
+		smp_call_function_many(cpus, ack_flush, NULL, wait);
 	else
 		called = false;
 	put_cpu();
-- 
2.12.2

Re: [PATCH v2 1/9] KVM: add kvm_{test,clear}_request to replace {test,clear}_bit

[Andrew Jones]

On Wed, Apr 26, 2017 at 10:32:19PM +0200, Radim Krčmář wrote:
> Users were expected to use kvm_check_request() for testing and clearing,
> but request have expanded their use since then and some users want to
> only test or do a faster clear.
> 
> Make sure that requests are not directly accessed with bit operations.

This isn't 100% accurate, as the set_bit changes are made in the next
patch.

> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Otherwise
Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 2/9] KVM: x86: always use kvm_make_request instead of set_bit

[Andrew Jones]

On Wed, Apr 26, 2017 at 10:32:20PM +0200, Radim Krčmář wrote:
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 4/9] KVM: mark requests that do not need a wakeup

[Andrew Jones]

On Wed, Apr 26, 2017 at 10:32:22PM +0200, Radim Krčmář wrote:
> Some operations must ensure that the guest is not running with stale
> data, but if the guest is halted, then the update can wait until another
> event happens.  kvm_make_all_requests() currently doesn't wake up, so we
> can mark all requests used with it.
> 
> First 8 bits were arbitrarily reserved for request numbers.
> 
> Most uses of requests have the request type as a constant, so a compiler
> will optimize the '&'.
> 
> An alternative would be to have an inline function that would return
> whether the request needs a wake-up or not, but I like this one better
> even though it might produce worse assembly.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: use GENMASK [Marc]
> ---
>  arch/arm/include/asm/kvm_host.h   |  2 +-
>  arch/arm64/include/asm/kvm_host.h |  2 +-
>  arch/x86/include/asm/kvm_host.h   |  6 +++---
>  include/linux/kvm_host.h          | 12 +++++++-----
>  4 files changed, 12 insertions(+), 10 deletions(-)

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2 5/9] KVM: perform a wake_up in kvm_make_all_cpus_request

[Andrew Jones]

On Wed, Apr 26, 2017 at 10:32:23PM +0200, Radim Krčmář wrote:
> We want to have kvm_make_all_cpus_request() to be an optmized version of
> 
>   kvm_for_each_vcpu(i, vcpu, kvm) {
>     kvm_make_request(vcpu, request);
>     kvm_vcpu_kick(vcpu);
>   }
> 
> and kvm_vcpu_kick() wakes up the target vcpu.  We know which requests do
> not need the wake up and use it to optimize the loop.
> 
> Thanks to that, this patch doesn't change the behavior of current users
> (the all don't need the wake up) and only prepares for future where the
> wake up is going to be needed.
> 
> I think that most requests do not need the wake up, so we would flip the
> bit then.
> 
> kvm_vcpu_kick() will get this condition after it is merged with
> kvm_make_request() because we currently don't know which request is being
> kicked.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e5d52b46b531..3772f7dcc72d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  		/* Set ->requests bit before we read ->mode. */
>  		smp_mb__after_atomic();
>  
> +		if (!(req & KVM_REQUEST_NO_WAKEUP))
> +			kvm_vcpu_wake_up(vcpu);
> +
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
>  		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
>  			cpumask_set_cpu(cpu, cpus);
> -- 
> 2.12.2
>

Reviewed-by: Andrew Jones <drjones@redhat.com> 

Re: [PATCH v2 1/9] KVM: add kvm_{test,clear}_request to replace {test,clear}_bit

[Cornelia Huck]

On Wed, 26 Apr 2017 22:32:19 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> Users were expected to use kvm_check_request() for testing and clearing,
> but request have expanded their use since then and some users want to
> only test or do a faster clear.
> 
> Make sure that requests are not directly accessed with bit operations.
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 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                |  8 ++++----
>  include/linux/kvm_host.h          | 14 ++++++++++++--
>  9 files changed, 24 insertions(+), 14 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH v2 8/9] KVM: return if kvm_vcpu_wake_up() did wake up the VCPU

[Andrew Jones]

On Wed, Apr 26, 2017 at 10:32:26PM +0200, Radim Krčmář wrote:
> No need to kick a VCPU that we have just woken up.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  include/linux/kvm_host.h |  2 +-
>  virt/kvm/kvm_main.c      | 12 ++++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 955debd82cf2..38cfe372918c 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -698,7 +698,7 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn);
>  void kvm_vcpu_block(struct kvm_vcpu *vcpu);
>  void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu);
>  void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu);
> -void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
> +bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
>  void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
>  int kvm_vcpu_yield_to(struct kvm_vcpu *target);
>  void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 1efb07643035..632f7b3e198c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -183,8 +183,8 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  		kvm_make_request(req, vcpu);
>  		cpu = vcpu->cpu;
>  
> -		if (!(req & KVM_REQUEST_NO_WAKEUP))
> -			kvm_vcpu_wake_up(vcpu);
> +		if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu))
> +			continue;
>  
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
>  		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
> @@ -2195,7 +2195,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_block);
>  
> -void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
> +bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>  {
>  	struct swait_queue_head *wqp;
>  
> @@ -2203,8 +2203,10 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>  	if (swait_active(wqp)) {
>  		swake_up(wqp);
>  		++vcpu->stat.halt_wakeup;
> +		return true;
>  	}
>  
> +	return false;
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
>  
> @@ -2216,7 +2218,9 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>  	int me;
>  	int cpu = vcpu->cpu;
>  
> -	kvm_vcpu_wake_up(vcpu);
> +	if (kvm_vcpu_wake_up(vcpu))
> +		return;
> +
>  	me = get_cpu();
>  	if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>  		if (kvm_arch_vcpu_should_kick(vcpu))
> -- 
> 2.12.2
>

Reviewed-by: Andrew Jones <drjones@redhat.com> 

Re: [PATCH v2 3/9] KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up

[Cornelia Huck]

On Wed, 26 Apr 2017 22:32:21 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> The #ifndef was protecting a missing halt_wakeup stat, but that is no
> longer necessary.
> 
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH v2 9/9] KVM: mark requests that need synchronization

[Andrew Jones]

On Wed, Apr 26, 2017 at 10:32:27PM +0200, Radim Krčmář wrote:
> kvm_make_all_requests() provides a synchronization that waits until all
> kicked VCPUs have acknowledged the kick.  This is important for
> KVM_REQ_MMU_RELOAD as it prevents freeing while lockless paging is
> underway.
> 
> This patch adds the synchronization property into all requests that are
> currently being used with kvm_make_all_requests() in order to preserve
> the current behavior and only introduce a new framework.  Removing it
> from requests where it is not necessary is left for future patches.
> 
> A question is whether this propertly isn't better expressed as a
> function of the caller.

Yes, IMO, knowing that waiting is needed, and the expectation that a wait
will occur, should be explicit to the caller.  I guess we need to extend
the VCPU request API in some way to do that.

> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: replaces [v1 1/6]
>      Ugh, KVM_ARCH_REQ_WAIT_NO_WAKEUP looks a weird ...

Assuming we stick with the request wait flag, rather than an API change,
then maybe we should drop KVM_ARCH_REQ_NO_WAKEUP() and just use, e.g.
KVM_ARCH_REQ_FLAGS(14, KVM_REQUEST_NO_WAKEUP) or
KVM_ARCH_REQ_FLAGS(14, KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)

> ---
>  arch/arm/include/asm/kvm_host.h   |  2 +-
>  arch/arm64/include/asm/kvm_host.h |  2 +-
>  arch/x86/include/asm/kvm_host.h   |  6 +++---
>  include/linux/kvm_host.h          |  8 ++++++--
>  virt/kvm/kvm_main.c               | 16 +++++++++++++---
>  5 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 1d48a4b65b86..2190a7ddd515 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -44,7 +44,7 @@
>  #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
>  #endif
>  
> -#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_NO_WAKEUP(0)
> +#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_WAIT_NO_WAKEUP(0)
>  
>  u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
>  int __attribute_const__ kvm_target_cpu(void);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d3370b79660e..98f3d01ae91b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -41,7 +41,7 @@
>  
>  #define KVM_VCPU_MAX_FEATURES 4
>  
> -#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_NO_WAKEUP(0)
> +#define KVM_REQ_VCPU_EXIT	KVM_ARCH_REQ_WAIT_NO_WAKEUP(0)
>  
>  int __attribute_const__ kvm_target_cpu(void);
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 15eb7d3837e3..77083d7e9540 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -61,10 +61,10 @@
>  #define KVM_REQ_PMI			KVM_ARCH_REQ(11)
>  #define KVM_REQ_SMI			KVM_ARCH_REQ(12)
>  #define KVM_REQ_MASTERCLOCK_UPDATE	KVM_ARCH_REQ(13)
> -#define KVM_REQ_MCLOCK_INPROGRESS	KVM_ARCH_REQ_NO_WAKEUP(14)
> -#define KVM_REQ_SCAN_IOAPIC		KVM_ARCH_REQ_NO_WAKEUP(15)
> +#define KVM_REQ_MCLOCK_INPROGRESS	KVM_ARCH_REQ_WAIT_NO_WAKEUP(14)
> +#define KVM_REQ_SCAN_IOAPIC		KVM_ARCH_REQ_WAIT_NO_WAKEUP(15)
>  #define KVM_REQ_GLOBAL_CLOCK_UPDATE	KVM_ARCH_REQ(16)
> -#define KVM_REQ_APIC_PAGE_RELOAD	KVM_ARCH_REQ_NO_WAKEUP(17)
> +#define KVM_REQ_APIC_PAGE_RELOAD	KVM_ARCH_REQ_WAIT_NO_WAKEUP(17)
>  #define KVM_REQ_HV_CRASH		KVM_ARCH_REQ(18)
>  #define KVM_REQ_IOAPIC_EOI_EXIT		KVM_ARCH_REQ(19)
>  #define KVM_REQ_HV_RESET		KVM_ARCH_REQ(20)
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 38cfe372918c..a668f33b20dc 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -117,12 +117,13 @@ static inline bool is_error_page(struct page *page)
>  
>  #define KVM_REQUEST_MASK           GENMASK(7,0)
>  #define KVM_REQUEST_NO_WAKEUP      BIT(8)
> +#define KVM_REQUEST_WAIT           BIT(9)
>  /*
>   * Architecture-independent vcpu->requests bit members
>   * Bits 4-7 are reserved for more arch-independent bits.
>   */
> -#define KVM_REQ_TLB_FLUSH          (0 | KVM_REQUEST_NO_WAKEUP)
> -#define KVM_REQ_MMU_RELOAD         (1 | KVM_REQUEST_NO_WAKEUP)
> +#define KVM_REQ_TLB_FLUSH          (0 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)
> +#define KVM_REQ_MMU_RELOAD         (1 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)
>  #define KVM_REQ_PENDING_TIMER      2
>  #define KVM_REQ_UNHALT             3
>  #define KVM_REQUEST_ARCH_BASE      8
> @@ -133,6 +134,9 @@ static inline bool is_error_page(struct page *page)
>  })
>  #define KVM_ARCH_REQ(nr)           KVM_ARCH_REQ_FLAGS(nr, 0)
>  #define KVM_ARCH_REQ_NO_WAKEUP(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP)
> +#define KVM_ARCH_REQ_WAIT(nr)      KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP)

copy+paste mistake: s/KVM_REQUEST_NO_WAKEUP/KVM_REQUEST_WAIT/

> +#define KVM_ARCH_REQ_WAIT_NO_WAKEUP(nr) \
> +	KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)
>  
>  #define KVM_USERSPACE_IRQ_SOURCE_ID		0
>  #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 632f7b3e198c..dbf0410cd8b2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -165,6 +165,15 @@ void vcpu_put(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(vcpu_put);
>  
> +/* TODO: merge with kvm_arch_vcpu_should_kick */
> +static bool kvm_should_kick_request(struct kvm_vcpu *vcpu, unsigned req)
> +{
> +	int mode = kvm_vcpu_exiting_guest_mode(vcpu);
> +
> +	return req & KVM_REQUEST_WAIT ?
> +		mode != OUTSIDE_GUEST_MODE : mode == IN_GUEST_MODE;
> +}
> +
>  static void ack_flush(void *_completed)
>  {
>  }
> @@ -174,6 +183,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  	int i, cpu, me;
>  	cpumask_var_t cpus;
>  	bool called = true;
> +	bool wait = req & KVM_REQUEST_WAIT;
>  	struct kvm_vcpu *vcpu;
>  
>  	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
> @@ -187,13 +197,13 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  			continue;
>  
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
> -		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
> +		    kvm_should_kick_request(vcpu, req))
>  			cpumask_set_cpu(cpu, cpus);
>  	}
>  	if (unlikely(cpus == NULL))
> -		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
> +		smp_call_function_many(cpu_online_mask, ack_flush, NULL, wait);
>  	else if (!cpumask_empty(cpus))
> -		smp_call_function_many(cpus, ack_flush, NULL, 1);
> +		smp_call_function_many(cpus, ack_flush, NULL, wait);
>  	else
>  		called = false;
>  	put_cpu();
> -- 
> 2.12.2
>

Thanks,
drew

Re: [PATCH v2 4/9] KVM: mark requests that do not need a wakeup

[Cornelia Huck]

On Wed, 26 Apr 2017 22:32:22 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> Some operations must ensure that the guest is not running with stale
> data, but if the guest is halted, then the update can wait until another
> event happens.  kvm_make_all_requests() currently doesn't wake up, so we
> can mark all requests used with it.
> 
> First 8 bits were arbitrarily reserved for request numbers.
> 
> Most uses of requests have the request type as a constant, so a compiler
> will optimize the '&'.
> 
> An alternative would be to have an inline function that would return
> whether the request needs a wake-up or not, but I like this one better
> even though it might produce worse assembly.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: use GENMASK [Marc]
> ---
>  arch/arm/include/asm/kvm_host.h   |  2 +-
>  arch/arm64/include/asm/kvm_host.h |  2 +-
>  arch/x86/include/asm/kvm_host.h   |  6 +++---
>  include/linux/kvm_host.h          | 12 +++++++-----
>  4 files changed, 12 insertions(+), 10 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH v2 5/9] KVM: perform a wake_up in kvm_make_all_cpus_request

[Cornelia Huck]

On Wed, 26 Apr 2017 22:32:23 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> We want to have kvm_make_all_cpus_request() to be an optmized version of
> 
>   kvm_for_each_vcpu(i, vcpu, kvm) {
>     kvm_make_request(vcpu, request);
>     kvm_vcpu_kick(vcpu);
>   }
> 
> and kvm_vcpu_kick() wakes up the target vcpu.  We know which requests do
> not need the wake up and use it to optimize the loop.
> 
> Thanks to that, this patch doesn't change the behavior of current users
> (the all don't need the wake up) and only prepares for future where the

s/the all/they all/

> wake up is going to be needed.
> 
> I think that most requests do not need the wake up, so we would flip the
> bit then.
> 
> kvm_vcpu_kick() will get this condition after it is merged with
> kvm_make_request() because we currently don't know which request is being
> kicked.

I find this sentence confusing: not all kicks are directly related to
requests.

> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e5d52b46b531..3772f7dcc72d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  		/* Set ->requests bit before we read ->mode. */
>  		smp_mb__after_atomic();
> 
> +		if (!(req & KVM_REQUEST_NO_WAKEUP))
> +			kvm_vcpu_wake_up(vcpu);
> +
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
>  		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
>  			cpumask_set_cpu(cpu, cpus);

The code change looks good to me.

Re: [PATCH v2 7/9] KVM: improve arch vcpu request defining

[Cornelia Huck]

On Wed, 26 Apr 2017 22:32:25 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> From: Andrew Jones <drjones@redhat.com>
> 
> Marc Zyngier suggested that we define the arch specific VCPU request
> base, rather than requiring each arch to remember to start from 8.
> That suggestion, along with Radim Krčmář's recent VCPU request flag
> addition, snowballed into defining something of an arch VCPU request
> defining API.
> 
> No functional change.
> 
> (Looks like x86 is running out of arch VCPU request bits.  Maybe
>  someday we'll need to extend to 64.)
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm/include/asm/kvm_host.h     |  2 +-
>  arch/arm64/include/asm/kvm_host.h   |  2 +-
>  arch/powerpc/include/asm/kvm_host.h |  4 ++--
>  arch/s390/include/asm/kvm_host.h    |  6 ++---
>  arch/x86/include/asm/kvm_host.h     | 44 ++++++++++++++++++-------------------
>  include/linux/kvm_host.h            |  8 +++++++
>  6 files changed, 37 insertions(+), 29 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH v2 5/9] KVM: perform a wake_up in kvm_make_all_cpus_request

[Paolo Bonzini]

On 27/04/2017 14:06, Cornelia Huck wrote:
> On Wed, 26 Apr 2017 22:32:23 +0200
> Radim Krčmář <rkrcmar@redhat.com> wrote:
> 
>> We want to have kvm_make_all_cpus_request() to be an optmized version of
>>
>>   kvm_for_each_vcpu(i, vcpu, kvm) {
>>     kvm_make_request(vcpu, request);
>>     kvm_vcpu_kick(vcpu);
>>   }
>>
>> and kvm_vcpu_kick() wakes up the target vcpu.  We know which requests do
>> not need the wake up and use it to optimize the loop.
>>
>> Thanks to that, this patch doesn't change the behavior of current users
>> (the all don't need the wake up) and only prepares for future where the
> 
> s/the all/they all/
> 
>> wake up is going to be needed.
>>
>> I think that most requests do not need the wake up, so we would flip the
>> bit then.
>>
>> kvm_vcpu_kick() will get this condition after it is merged with
>> kvm_make_request() because we currently don't know which request is being
>> kicked.
> 
> I find this sentence confusing: not all kicks are directly related to
> requests.

I agree, it is backwards.  Changing to "Later on, kvm_make_request()
will take care of kicking too, using this bit to make the decision
whether to kick or not".

Paolo
>>
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  virt/kvm/kvm_main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index e5d52b46b531..3772f7dcc72d 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>>  		/* Set ->requests bit before we read ->mode. */
>>  		smp_mb__after_atomic();
>>
>> +		if (!(req & KVM_REQUEST_NO_WAKEUP))
>> +			kvm_vcpu_wake_up(vcpu);
>> +
>>  		if (cpus != NULL && cpu != -1 && cpu != me &&
>>  		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
>>  			cpumask_set_cpu(cpu, cpus);
> 
> The code change looks good to me.
> 

Re: [PATCH v2 9/9] KVM: mark requests that need synchronization

[Paolo Bonzini]

On 26/04/2017 22:32, Radim Krčmář wrote:
>  v2: replaces [v1 1/6]
>      Ugh, KVM_ARCH_REQ_WAIT_NO_WAKEUP looks a weird ...

Yeah, let's drop patch 7 and just use bits for now.  I think using
KVM_ARCH_REQ_FLAGS directly should be fine, especially after the default
is flipped from "no wakeup" to "wakeup", but for 4.12 this is the
simplest incremental step.

> +/* TODO: merge with kvm_arch_vcpu_should_kick */
> +static bool kvm_should_kick_request(struct kvm_vcpu *vcpu, unsigned req)

I'm renaming this to kvm_request_needs_ipi; the point of the IPI for
synchronous requests is not the "kick", but the "ack" that comes back.

Paolo

> +{
> +	int mode = kvm_vcpu_exiting_guest_mode(vcpu);
> +
> +	return req & KVM_REQUEST_WAIT ?
> +		mode != OUTSIDE_GUEST_MODE : mode == IN_GUEST_MODE;
> +}
> +

Re: [PATCH v2 3/9] KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up

[Radim Krčmář]

2017-04-26 22:32+0200, Radim Krčmář:
> The #ifndef was protecting a missing halt_wakeup stat, but that is no
> longer necessary.

It was also protecting smp_send_reschedule() in kvm_vcpu_kick() and I
didn't include Christian's patch that exported it and also missed the
warning.  Going to send v3 of this patch that removes the ifdef really
only around kvm_vcpu_wake_up().

Sorry for that.

> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 357e67cba32e..e5d52b46b531 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2195,7 +2195,6 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_block);
>  
> -#ifndef CONFIG_S390
>  void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>  {
>  	struct swait_queue_head *wqp;
> @@ -2225,7 +2224,6 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>  	put_cpu();
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
> -#endif /* !CONFIG_S390 */
>  
>  int kvm_vcpu_yield_to(struct kvm_vcpu *target)
>  {
> -- 
> 2.12.2
> 

[PATCH v3] KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up

[Radim Krčmář]

The #ifndef was protecting a missing halt_wakeup stat, but that is no
longer necessary.  The #ifndef around kvm_vcpu_kick is still necessary
as s390 does not export smp_send_reschedule.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v3: kept kvm_vcpu_kick() under the #ifndef and compile tested with kvm
     configured as module too ...

 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 357e67cba32e..7112889e5e1a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2195,7 +2195,6 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_block);
 
-#ifndef CONFIG_S390
 void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wqp;
@@ -2209,6 +2208,7 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
 
+#ifndef CONFIG_S390
 /*
  * Kick a sleeping VCPU, or a guest VCPU in guest mode, into host kernel mode.
  */
-- 
2.12.2

Re: [PATCH v3] KVM: remove #ifndef CONFIG_S390 around kvm_vcpu_wake_up

[Cornelia Huck]

On Wed,  3 May 2017 18:16:34 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> The #ifndef was protecting a missing halt_wakeup stat, but that is no
> longer necessary.  The #ifndef around kvm_vcpu_kick is still necessary
> as s390 does not export smp_send_reschedule.

It feels like smp_send_reschedule() should be exported everywhere if
kvm wants to use it... but this certainly works as well.

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v3: kept kvm_vcpu_kick() under the #ifndef and compile tested with kvm
>      configured as module too ...
> 
>  virt/kvm/kvm_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 357e67cba32e..7112889e5e1a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2195,7 +2195,6 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_block);
> 
> -#ifndef CONFIG_S390
>  void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>  {
>  	struct swait_queue_head *wqp;
> @@ -2209,6 +2208,7 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
> 
> +#ifndef CONFIG_S390
>  /*
>   * Kick a sleeping VCPU, or a guest VCPU in guest mode, into host kernel mode.
>   */