[PATCH v4 00/11] KVM: arm/arm64: race fixes and vcpu requests

[PATCH v4 00/11] KVM: arm/arm64: race fixes and vcpu requests

[Andrew Jones]

This series fixes some hard to produce races by introducing the use of
vcpu requests. I've tested the series on a Mustang and compile-tested
the ARM bits.

Patch 3/11 adds documentation, as, at least for me, understanding vcpu
request interplay with vcpu kicks and vcpu mode and the memory barriers
that interplay implies, is exhausting.

v4:
  - Rebase on latest kvmarm/queue. No more kvm_arm_halt/resume_vcpu
  - Documentation additions/clarifications [Paolo]
  - go back to sleep after getting a signal [Christoffer]
  - timer doesn't need a vcpu request, just a wake up [Christoffer]
  - patch dropping/adding/reshuffling for above changes

v3:
  - The easier to reproduce PSCI races previously fixed with this
    series were fixed independently with a different approach suggested
    by Christoffer[1].
  - Based on Radim's latest vcpu request rework series[2]
  - Rewrote the documentation adding much more information and
    incorporating Christoffer's comments from v2.
  - Reworked the approach to controlling pause and power_off by
    requests.
  - Clear request for pending irq injection in the correct place, as
    pointed out by Christoffer.
  - Rebase required some simple changes to the PMU code patch.

v2:
  - No longer based on Radim's vcpu request API rework[3], except for
    including "add kvm_request_pending" as patch 1/10 [drew]
  - Added vcpu request documentation [drew]
  - Dropped the introduction of user settable MPIDRs [Christoffer]
  - Added vcpu requests to all request-less vcpu kicks [Christoffer]

[1] https://www.spinics.net/lists/arm-kernel/msg577630.html
[2] http://www.spinics.net/lists/kvm/msg148890.html
[3] https://www.spinics.net/lists/kvm/msg145588.html


Andrew Jones (10):
  KVM: improve arch vcpu request defining
  KVM: Add documentation for VCPU requests
  KVM: arm/arm64: properly use vcpu requests
  KVM: arm/arm64: replace pause checks with vcpu request checks
  KVM: arm/arm64: use vcpu requests for power_off
  KVM: arm/arm64: optimize VCPU RUN
  KVM: arm/arm64: change exit request to sleep request
  KVM: arm/arm64: use vcpu requests for irq injection
  KVM: arm/arm64: PMU: remove request-less vcpu kick
  KVM: arm/arm64: timer: remove request-less vcpu kick

Radim Krčmář (1):
  KVM: add kvm_request_pending

 Documentation/virtual/kvm/vcpu-requests.rst | 299 ++++++++++++++++++++++++++++
 arch/arm/include/asm/kvm_host.h             |   4 +-
 arch/arm/kvm/handle_exit.c                  |   1 +
 arch/arm64/include/asm/kvm_host.h           |   4 +-
 arch/arm64/kvm/handle_exit.c                |   1 +
 arch/mips/kvm/trap_emul.c                   |   2 +-
 arch/mips/kvm/vz.c                          |   2 +-
 arch/powerpc/include/asm/kvm_host.h         |   4 +-
 arch/powerpc/kvm/booke.c                    |   2 +-
 arch/powerpc/kvm/powerpc.c                  |   5 +-
 arch/s390/include/asm/kvm_host.h            |   6 +-
 arch/s390/kvm/kvm-s390.c                    |   2 +-
 arch/x86/include/asm/kvm_host.h             |  47 +++--
 arch/x86/kvm/x86.c                          |   4 +-
 include/linux/kvm_host.h                    |  12 ++
 virt/kvm/arm/arch_timer.c                   |   2 +-
 virt/kvm/arm/arm.c                          |  55 ++++-
 virt/kvm/arm/pmu.c                          |  40 ++--
 virt/kvm/arm/psci.c                         |   8 +-
 virt/kvm/arm/vgic/vgic.c                    |   9 +-
 20 files changed, 436 insertions(+), 73 deletions(-)
 create mode 100644 Documentation/virtual/kvm/vcpu-requests.rst

-- 
2.9.3

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 01/11] KVM: improve arch vcpu request defining

[Andrew Jones]

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 Krcmar'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     |  3 ++-
 arch/arm64/include/asm/kvm_host.h   |  3 ++-
 arch/powerpc/include/asm/kvm_host.h |  4 ++--
 arch/s390/include/asm/kvm_host.h    |  6 ++---
 arch/x86/include/asm/kvm_host.h     | 47 ++++++++++++++++++++-----------------
 include/linux/kvm_host.h            |  7 ++++++
 6 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 12274d46df70..c556babe467c 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -44,7 +44,8 @@
 #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
 #endif
 
-#define KVM_REQ_VCPU_EXIT	(8 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_VCPU_EXIT \
+	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | 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 32cbe8a3bb0d..0ff991c9c66e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -41,7 +41,8 @@
 
 #define KVM_VCPU_MAX_FEATURES 4
 
-#define KVM_REQ_VCPU_EXIT	(8 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_VCPU_EXIT \
+	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 
 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 9c51ac4b8f36..50e0bc9723cc 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 9c761fea0c98..563979976fab 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -48,28 +48,31 @@
 #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_WAIT | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_SCAN_IOAPIC       (23 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_GLOBAL_CLOCK_UPDATE 24
-#define KVM_REQ_APIC_PAGE_RELOAD  (25 | KVM_REQUEST_WAIT | 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_FLAGS(14, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_SCAN_IOAPIC \
+	KVM_ARCH_REQ_FLAGS(15, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_GLOBAL_CLOCK_UPDATE	KVM_ARCH_REQ(16)
+#define KVM_REQ_APIC_PAGE_RELOAD \
+	KVM_ARCH_REQ_FLAGS(17, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#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 8c0664309815..3724b51aab64 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -126,6 +126,13 @@ static inline bool is_error_page(struct page *page)
 #define KVM_REQ_MMU_RELOAD        (1 | KVM_REQUEST_WAIT | 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_USERSPACE_IRQ_SOURCE_ID		0
 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
-- 
2.9.3

[PATCH v4 02/11] KVM: add kvm_request_pending

[Andrew Jones]

From: Radim Krčmář <rkrcmar@redhat.com>

A first step in vcpu->requests encapsulation.  Additionally, we now
use READ_ONCE() when accessing vcpu->requests, which ensures we
always load vcpu->requests when it's accessed.  This is important as
other threads can change it any time.  Also, READ_ONCE() documents
that vcpu->requests is used with other threads, likely requiring
memory barriers, which it does.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
[ Documented the new use of READ_ONCE() and converted another check
  in arch/mips/kvm/vz.c ]
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/mips/kvm/trap_emul.c  | 2 +-
 arch/mips/kvm/vz.c         | 2 +-
 arch/powerpc/kvm/booke.c   | 2 +-
 arch/powerpc/kvm/powerpc.c | 5 ++---
 arch/s390/kvm/kvm-s390.c   | 2 +-
 arch/x86/kvm/x86.c         | 4 ++--
 include/linux/kvm_host.h   | 5 +++++
 7 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/mips/kvm/trap_emul.c b/arch/mips/kvm/trap_emul.c
index a563759fd142..6a0d7040d882 100644
--- a/arch/mips/kvm/trap_emul.c
+++ b/arch/mips/kvm/trap_emul.c
@@ -1094,7 +1094,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_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
index 71d8856ade64..74805035edc8 100644
--- a/arch/mips/kvm/vz.c
+++ b/arch/mips/kvm/vz.c
@@ -2337,7 +2337,7 @@ static int kvm_vz_check_requests(struct kvm_vcpu *vcpu, int cpu)
 	int ret = 0;
 	int i;
 
-	if (!vcpu->requests)
+	if (!kvm_request_pending(vcpu))
 		return 0;
 
 	if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 3eaac3809977..071b87ee682f 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -687,7 +687,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 f7cf2cd564ef..fd64f087737c 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -55,8 +55,7 @@ EXPORT_SYMBOL_GPL(kvmppc_pr_ops);
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
-	return !!(v->arch.pending_exceptions) ||
-	       v->requests;
+	return !!(v->arch.pending_exceptions) || kvm_request_pending(v);
 }
 
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
@@ -108,7 +107,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 689ac48361c6..ad41e0fa3a21 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2440,7 +2440,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 464da936c53d..f81060518635 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6710,7 +6710,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_check_request(KVM_REQ_MMU_RELOAD, vcpu))
 			kvm_mmu_unload(vcpu);
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
@@ -6874,7 +6874,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 3724b51aab64..0b50e7b35ed4 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1105,6 +1105,11 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
 	set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
 }
 
+static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
+{
+	return READ_ONCE(vcpu->requests);
+}
+
 static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
 {
 	return test_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
-- 
2.9.3

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 03/11] KVM: Add documentation for VCPU requests

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 Documentation/virtual/kvm/vcpu-requests.rst | 299 ++++++++++++++++++++++++++++
 1 file changed, 299 insertions(+)
 create mode 100644 Documentation/virtual/kvm/vcpu-requests.rst

diff --git a/Documentation/virtual/kvm/vcpu-requests.rst b/Documentation/virtual/kvm/vcpu-requests.rst
new file mode 100644
index 000000000000..7a2b4c05c267
--- /dev/null
+++ b/Documentation/virtual/kvm/vcpu-requests.rst
@@ -0,0 +1,299 @@
+=================
+KVM VCPU Requests
+=================
+
+Overview
+========
+
+KVM supports an internal API enabling threads to request a VCPU thread to
+perform some activity.  For example, a thread may request a VCPU to flush
+its TLB with a VCPU request.  The API consists of the following functions::
+
+  /* Check if any requests are pending for VCPU @vcpu. */
+  bool kvm_request_pending(struct kvm_vcpu *vcpu);
+
+  /* Check if VCPU @vcpu has request @req pending. */
+  bool kvm_test_request(int req, struct kvm_vcpu *vcpu);
+
+  /* Clear request @req for VCPU @vcpu. */
+  void kvm_clear_request(int req, struct kvm_vcpu *vcpu);
+
+  /*
+   * Check if VCPU @vcpu has request @req pending. When the request is
+   * pending it will be cleared and a memory barrier, which pairs with
+   * another in kvm_make_request(), will be issued.
+   */
+  bool kvm_check_request(int req, struct kvm_vcpu *vcpu);
+
+  /*
+   * Make request @req of VCPU @vcpu. Issues a memory barrier, which pairs
+   * with another in kvm_check_request(), prior to setting the request.
+   */
+  void kvm_make_request(int req, struct kvm_vcpu *vcpu);
+
+  /* Make request @req of all VCPUs of the VM with struct kvm @kvm. */
+  bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req);
+
+Typically a requester wants the VCPU to perform the activity as soon
+as possible after making the request.  This means most requests
+(kvm_make_request() calls) are followed by a call to kvm_vcpu_kick(),
+and kvm_make_all_cpus_request() has the kicking of all VCPUs built
+into it.
+
+VCPU Kicks
+----------
+
+The goal of a VCPU kick is to bring a VCPU thread out of guest mode in
+order to perform some KVM maintenance.  To do so, an IPI is sent, forcing
+a guest mode exit.  However, a VCPU thread may not be in guest mode at the
+time of the kick.  Therefore, depending on the mode and state of the VCPU
+thread, there are two other actions a kick may take.  All three actions
+are listed below:
+
+1) Send an IPI.  This forces a guest mode exit.
+2) Waking a sleeping VCPU.  Sleeping VCPUs are VCPU threads outside guest
+   mode that wait on waitqueues.  Waking them removes the threads from
+   the waitqueues, allowing the threads to run again.  This behavior
+   may be suppressed, see KVM_REQUEST_NO_WAKEUP below.
+3) Nothing.  When the VCPU is not in guest mode and the VCPU thread is not
+   sleeping, then there is nothing to do.
+
+VCPU Mode
+---------
+
+VCPUs have a mode state, ``vcpu->mode``, that is used to track whether the
+guest is running in guest mode or not, as well as some specific
+outside guest mode states.  The architecture may use ``vcpu->mode`` to
+ensure VCPU requests are seen by VCPUs (see "Ensuring Requests Are Seen"),
+as well as to avoid sending unnecessary IPIs (see "IPI Reduction"), and
+even to ensure IPI acknowledgements are waited upon (see "Waiting for
+Acknowledgements").  The following modes are defined:
+
+OUTSIDE_GUEST_MODE
+
+  The VCPU thread is outside guest mode.
+
+IN_GUEST_MODE
+
+  The VCPU thread is in guest mode.
+
+EXITING_GUEST_MODE
+
+  The VCPU thread is transitioning from IN_GUEST_MODE to
+  OUTSIDE_GUEST_MODE.
+
+READING_SHADOW_PAGE_TABLES
+
+  The VCPU thread is outside guest mode, but it wants the sender of
+  certain VCPU requests, namely KVM_REQ_TLB_FLUSH, to wait until the VCPU
+  thread is done reading the page tables.
+
+VCPU Request Internals
+======================
+
+VCPU requests are simply bit indices of the ``vcpu->requests`` bitmap.
+This means general bitops, like those documented in [atomic-ops]_ could
+also be used, e.g. ::
+
+  clear_bit(KVM_REQ_UNHALT & KVM_REQUEST_MASK, &vcpu->requests);
+
+However, VCPU request users should refrain from doing so, as it would
+break the abstraction.  The first 8 bits are reserved for architecture
+independent requests, all additional bits are available for architecture
+dependent requests.
+
+Architecture Independent Requests
+---------------------------------
+
+KVM_REQ_TLB_FLUSH
+
+  KVM's common MMU notifier may need to flush all of a guest's TLB
+  entries, calling kvm_flush_remote_tlbs() to do so.  Architectures that
+  choose to use the common kvm_flush_remote_tlbs() implementation will
+  need to handle this VCPU request.
+
+KVM_REQ_MMU_RELOAD
+
+  When shadow page tables are used and memory slots are removed it's
+  necessary to inform each VCPU to completely refresh the tables.  This
+  request is used for that.
+
+KVM_REQ_PENDING_TIMER
+
+  This request may be made from a timer handler run on the host on behalf
+  of a VCPU.  It informs the VCPU thread to inject a timer interrupt.
+
+KVM_REQ_UNHALT
+
+  This request may be made from the KVM common function kvm_vcpu_block(),
+  which is used to emulate an instruction that causes a CPU to halt until
+  one of an architectural specific set of events and/or interrupts is
+  received (determined by checking kvm_arch_vcpu_runnable()).  When that
+  event or interrupt arrives kvm_vcpu_block() makes the request.  This is
+  in contrast to when kvm_vcpu_block() returns due to any other reason,
+  such as a pending signal, which does not indicate the VCPU's halt
+  emulation should stop, and therefore does not make the request.
+
+KVM_REQUEST_MASK
+----------------
+
+VCPU requests should be masked by KVM_REQUEST_MASK before using them with
+bitops.  This is because only the lower 8 bits are used to represent the
+request's number.  The upper bits are used as flags.  Currently only two
+flags are defined.
+
+VCPU Request Flags
+------------------
+
+KVM_REQUEST_NO_WAKEUP
+
+  This flag is applied to a request that does not need immediate
+  attention.  When a request does not need immediate attention, and the
+  VCPU's thread is outside guest mode sleeping, then the thread is not
+  awaken by a kick.
+
+KVM_REQUEST_WAIT
+
+  When requests with this flag are made with kvm_make_all_cpus_request(),
+  then the caller will wait for each VCPU to acknowledge the IPI before
+  proceeding.
+
+VCPU Requests with Associated State
+===================================
+
+Requesters that want the receiving VCPU to handle new state need to ensure
+the newly written state is observable to the receiving VCPU thread's CPU
+by the time it observes the request.  This means a write memory barrier
+must be inserted after writing the new state and before setting the VCPU
+request bit.  Additionally, on the receiving VCPU thread's side, a
+corresponding read barrier must be inserted after reading the request bit
+and before proceeding to read the new state associated with it.  See
+scenario 3, Message and Flag, of [lwn-mb]_ and the kernel documentation
+[memory-barriers]_.
+
+The pair of functions, kvm_check_request() and kvm_make_request(), provide
+the memory barriers, allowing this requirement to be handled internally by
+the API.
+
+Ensuring Requests Are Seen
+==========================
+
+When making requests to VCPUs, we want to avoid the receiving VCPU
+executing in guest mode for an arbitrary long time without handling the
+request.  We can be sure this won't happen as long as we ensure the VCPU
+thread checks kvm_request_pending() before entering guest mode and that a
+kick will send an IPI when necessary.  Extra care must be taken to cover
+the period after the VCPU thread's last kvm_request_pending() check and
+before it has entered guest mode, as kick IPIs will only trigger VCPU run
+loops for VCPU threads that are in guest mode or at least have already
+disabled interrupts in order to prepare to enter guest mode.  This means
+that an optimized implementation (see "IPI Reduction") must be certain
+when it's safe to not send the IPI.  One solution, which all architectures
+except s390 apply, is to:
+
+- set ``vcpu->mode`` to IN_GUEST_MODE between disabling the interrupts and
+  the last kvm_request_pending() check;
+- enable interrupts atomically when entering the guest.
+
+This solution also requires memory barriers to be placed carefully in both
+the requesting thread and the receiving VCPU.  With the memory barriers we
+can exclude the possibility of a VCPU thread observing
+!kvm_request_pending() on its last check and then not receiving an IPI for
+the next request made of it, even if the request is made immediately after
+the check.  This is done by way of the Dekker memory barrier pattern
+(scenario 10 of [lwn-mb]_).  As the Dekker pattern requires two variables,
+this solution pairs ``vcpu->mode`` with ``vcpu->requests``.  Substituting
+them into the pattern gives::
+
+  CPU1                                    CPU2
+  =================                       =================
+  local_irq_disable();
+  WRITE_ONCE(vcpu->mode, IN_GUEST_MODE);  kvm_make_request(REQ, vcpu);
+  smp_mb();                               smp_mb();
+  if (kvm_request_pending(vcpu)) {        if (READ_ONCE(vcpu->mode) ==
+                                              IN_GUEST_MODE) {
+      ...abort guest entry...                 ...send IPI...
+  }                                       }
+
+As stated above, the IPI is only useful for VCPU threads in guest mode or
+that have already disabled interrupts.  This is why this specific case of
+the Dekker pattern has been extended to disable interrupts before setting
+``vcpu->mode`` to IN_GUEST_MODE.  WRITE_ONCE() and READ_ONCE() are used to
+pedantically implement the memory barrier pattern, guaranteeing the
+compiler doesn't interfere with ``vcpu->mode``'s carefully planned
+accesses.
+
+IPI Reduction
+-------------
+
+As only one IPI is needed to get a VCPU to check for any/all requests,
+then they may be coalesced.  This is easily done by having the first IPI
+sending kick also change the VCPU mode to something !IN_GUEST_MODE.  The
+transitional state, EXITING_GUEST_MODE, is used for this purpose.
+
+Waiting for Acknowledgements
+----------------------------
+
+Some requests, those with the KVM_REQUEST_WAIT flag set, require IPIs to
+be sent, and the acknowledgements to be waited upon, even when the target
+VCPU threads are in modes other than IN_GUEST_MODE.  For example, one case
+is when a target VCPU thread is in READING_SHADOW_PAGE_TABLES mode, which
+is set after disabling interrupts.  For these cases, the "should send an
+IPI" condition becomes READ_ONCE(``vcpu->mode``) != OUTSIDE_GUEST_MODE.
+
+Request-less VCPU Kicks
+-----------------------
+
+As the determination of whether or not to send an IPI depends on the
+two-variable Dekker memory barrier pattern, then it's clear that
+request-less VCPU kicks are almost never correct.  Without the assurance
+that a non-IPI generating kick will still result in an action by the
+receiving VCPU, as the final kvm_request_pending() check does for
+request-accompanying kicks, then the kick may not do anything useful at
+all.  If, for instance, a request-less kick was made to a VCPU that was
+just about to set its mode to IN_GUEST_MODE, meaning no IPI is sent, then
+the VCPU thread may continue its entry without actually having done
+whatever it was the kick was meant to initiate.
+
+One exception is x86's posted interrupt mechanism.  In this case, however,
+even the request-less VCPU kick is coupled with the same
+local_irq_disable() + smp_mb() pattern described above; the ON bit
+(Outstanding Notification) in the posted interrupt descriptor takes the
+role of ``vcpu->requests``.  When sending a posted interrupt, PIR.ON is
+set before reading ``vcpu->mode``; dually, in the VCPU thread,
+vmx_sync_pir_to_irr() reads PIR after setting ``vcpu->mode`` to
+IN_GUEST_MODE.
+
+Additional Considerations
+=========================
+
+Sleeping VCPUs
+--------------
+
+VCPU threads may need to consider requests before and/or after calling
+functions that may put them to sleep, e.g. kvm_vcpu_block().  Whether they
+do or not, and, if they do, which requests need consideration, is
+architecture dependent.  kvm_vcpu_block() calls kvm_arch_vcpu_runnable()
+to check if it should awaken.  One reason to do so is to provide
+architectures a function where requests may be checked if necessary.
+
+Clearing Requests
+-----------------
+
+Generally it only makes sense for the receiving VCPU thread to clear a
+request.  However, in some circumstances, such as when the requesting
+thread is executing synchronously with the receiving VCPU thread, it's
+possible to know that the request may be cleared immediately, rather than
+waiting for the receiving VCPU thread to handle the request in VCPU RUN.
+The only current examples of this are kvm_vcpu_block() calls, where a
+side-effect of a call may be to set KVM_REQ_UNHALT.  When the requesting
+thread is itself the receiving VCPU, then it's possible to know that the
+request does not need to be handled in VCPU RUN, and therefore may be
+cleared immediately.
+
+References
+==========
+
+.. [atomic-ops] Documentation/core-api/atomic_ops.rst
+.. [memory-barriers] Documentation/memory-barriers.txt
+.. [lwn-mb] https://lwn.net/Articles/573436/
-- 
2.9.3

[PATCH v4 04/11] KVM: arm/arm64: properly use vcpu requests

[Andrew Jones]

arm/arm64 already has one VCPU request used when setting pause,
but it doesn't properly check requests in VCPU RUN. Check it
and also make sure we set vcpu->mode at the appropriate time
(before the check) and with the appropriate barriers. See
Documentation/virtual/kvm/vcpu-requests.rst. Also make sure we
don't leave any vcpu requests we don't intend to handle later
set in the request bitmap. If we don't clear them, then
kvm_request_pending() may return true when it shouldn't.

Using VCPU requests properly fixes a small race where pause
could get set just as a VCPU was entering guest mode.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/arm/kvm/handle_exit.c   |  1 +
 arch/arm64/kvm/handle_exit.c |  1 +
 virt/kvm/arm/arm.c           | 14 ++++++++++++--
 virt/kvm/arm/psci.c          |  1 +
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
index 5fd7968cdae9..a2b4f7b82356 100644
--- a/arch/arm/kvm/handle_exit.c
+++ b/arch/arm/kvm/handle_exit.c
@@ -72,6 +72,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		trace_kvm_wfx(*vcpu_pc(vcpu), false);
 		vcpu->stat.wfi_exit_stat++;
 		kvm_vcpu_block(vcpu);
+		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
 	}
 
 	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index fa1b18e364fc..17d8a1677a0b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -89,6 +89,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
 		vcpu->stat.wfi_exit_stat++;
 		kvm_vcpu_block(vcpu);
+		kvm_clear_request(KVM_REQ_UNHALT, vcpu);
 	}
 
 	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 3c387fdc4a9e..138212605ad9 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -546,6 +546,7 @@ void kvm_arm_resume_guest(struct kvm *kvm)
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		vcpu->arch.pause = false;
+		kvm_clear_request(KVM_REQ_VCPU_EXIT, vcpu);
 		swake_up(kvm_arch_vcpu_wq(vcpu));
 	}
 }
@@ -638,8 +639,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 
+		/*
+		 * Ensure we set mode to IN_GUEST_MODE after we disable
+		 * interrupts and before the final VCPU requests check.
+		 * See the comment in kvm_vcpu_exiting_guest_mode() and
+		 * Documentation/virtual/kvm/vcpu-requests.rst
+		 */
+		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
+
 		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
-			vcpu->arch.power_off || vcpu->arch.pause) {
+		    kvm_request_pending(vcpu) ||
+		    vcpu->arch.power_off || vcpu->arch.pause) {
+			vcpu->mode = OUTSIDE_GUEST_MODE;
 			local_irq_enable();
 			kvm_pmu_sync_hwstate(vcpu);
 			kvm_timer_sync_hwstate(vcpu);
@@ -655,7 +666,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		 */
 		trace_kvm_entry(*vcpu_pc(vcpu));
 		guest_enter_irqoff();
-		vcpu->mode = IN_GUEST_MODE;
 
 		ret = kvm_call_hyp(__kvm_vcpu_run, vcpu);
 
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index a08d7a93aebb..f68be2cc6256 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -57,6 +57,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
 	 * for KVM will preserve the register state.
 	 */
 	kvm_vcpu_block(vcpu);
+	kvm_clear_request(KVM_REQ_UNHALT, vcpu);
 
 	return PSCI_RET_SUCCESS;
 }
-- 
2.9.3

[PATCH v4 05/11] KVM: arm/arm64: replace pause checks with vcpu request checks

[Andrew Jones]

The current use of KVM_REQ_VCPU_EXIT for pause is fine.  Even the
requester clearing the request is OK, as this is the special case
where the sole requesting thread and receiving VCPU are executing
synchronously (see "Clearing Requests" in
Documentation/virtual/kvm/vcpu-requests.rst) However, that's about
to change, so let's ensure only the receiving VCPU clears the
request. Additionally, by guaranteeing KVM_REQ_VCPU_EXIT is always
set when pause is, we can avoid checking pause directly in VCPU RUN.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 virt/kvm/arm/arm.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 138212605ad9..21a4db90073f 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -546,7 +546,6 @@ void kvm_arm_resume_guest(struct kvm *kvm)
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		vcpu->arch.pause = false;
-		kvm_clear_request(KVM_REQ_VCPU_EXIT, vcpu);
 		swake_up(kvm_arch_vcpu_wq(vcpu));
 	}
 }
@@ -557,6 +556,11 @@ static void vcpu_sleep(struct kvm_vcpu *vcpu)
 
 	swait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
 				       (!vcpu->arch.pause)));
+
+	if (vcpu->arch.pause) {
+		/* Awaken to handle a signal, request we sleep again later. */
+		kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
+	}
 }
 
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
@@ -564,6 +568,14 @@ static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
 	return vcpu->arch.target >= 0;
 }
 
+static void check_vcpu_requests(struct kvm_vcpu *vcpu)
+{
+	if (kvm_request_pending(vcpu)) {
+		if (kvm_check_request(KVM_REQ_VCPU_EXIT, vcpu))
+			vcpu_sleep(vcpu);
+	}
+}
+
 /**
  * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
  * @vcpu:	The VCPU pointer
@@ -609,7 +621,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		update_vttbr(vcpu->kvm);
 
-		if (vcpu->arch.power_off || vcpu->arch.pause)
+		check_vcpu_requests(vcpu);
+
+		if (vcpu->arch.power_off)
 			vcpu_sleep(vcpu);
 
 		/*
@@ -649,7 +663,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
 		    kvm_request_pending(vcpu) ||
-		    vcpu->arch.power_off || vcpu->arch.pause) {
+		    vcpu->arch.power_off) {
 			vcpu->mode = OUTSIDE_GUEST_MODE;
 			local_irq_enable();
 			kvm_pmu_sync_hwstate(vcpu);
-- 
2.9.3

[PATCH v4 06/11] KVM: arm/arm64: use vcpu requests for power_off

[Andrew Jones]

System shutdown is currently using request-less VCPU kicks. This
leaves open a tiny race window, as it doesn't ensure the state
change to power_off is seen by a VCPU just about to enter guest
mode. VCPU requests, OTOH, are guaranteed to be seen (see "Ensuring
Requests Are Seen" of Documentation/virtual/kvm/vcpu-requests.rst)
This patch applies the EXIT request used by pause to power_off,
fixing the race.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 virt/kvm/arm/psci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index f68be2cc6256..f189d0ad30d5 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -179,10 +179,9 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
 	 * after this call is handled and before the VCPUs have been
 	 * re-initialized.
 	 */
-	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
+	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
 		tmp->arch.power_off = true;
-		kvm_vcpu_kick(tmp);
-	}
+	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_VCPU_EXIT);
 
 	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
 	vcpu->run->system_event.type = type;
-- 
2.9.3

[PATCH v4 07/11] KVM: arm/arm64: optimize VCPU RUN

[Andrew Jones]

We can make a small optimization by not checking the state of
the power_off field on each run. This is done by treating
power_off like pause, only checking it when we get the EXIT
VCPU request. When a VCPU powers off another VCPU the EXIT
request is already made, so we just need to make sure the
request is also made on self power off. kvm_vcpu_kick() isn't
necessary for these cases, as the VCPU would just be kicking
itself, but we add it anyway as a self kick doesn't cost much,
and it makes the code more future-proof.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 virt/kvm/arm/arm.c  | 19 +++++++++++--------
 virt/kvm/arm/psci.c |  2 ++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 21a4db90073f..9379b1d75ad3 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -368,6 +368,13 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	kvm_timer_vcpu_put(vcpu);
 }
 
+static void vcpu_power_off(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.power_off = true;
+	kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
+	kvm_vcpu_kick(vcpu);
+}
+
 int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
@@ -387,7 +394,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 		vcpu->arch.power_off = false;
 		break;
 	case KVM_MP_STATE_STOPPED:
-		vcpu->arch.power_off = true;
+		vcpu_power_off(vcpu);
 		break;
 	default:
 		return -EINVAL;
@@ -557,7 +564,7 @@ static void vcpu_sleep(struct kvm_vcpu *vcpu)
 	swait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
 				       (!vcpu->arch.pause)));
 
-	if (vcpu->arch.pause) {
+	if (vcpu->arch.power_off || vcpu->arch.pause) {
 		/* Awaken to handle a signal, request we sleep again later. */
 		kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
 	}
@@ -623,9 +630,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		check_vcpu_requests(vcpu);
 
-		if (vcpu->arch.power_off)
-			vcpu_sleep(vcpu);
-
 		/*
 		 * Preparing the interrupts to be injected also
 		 * involves poking the GIC, which must be done in a
@@ -662,8 +666,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
 
 		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
-		    kvm_request_pending(vcpu) ||
-		    vcpu->arch.power_off) {
+		    kvm_request_pending(vcpu)) {
 			vcpu->mode = OUTSIDE_GUEST_MODE;
 			local_irq_enable();
 			kvm_pmu_sync_hwstate(vcpu);
@@ -896,7 +899,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
 	 * Handle the "start in power-off" case.
 	 */
 	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
-		vcpu->arch.power_off = true;
+		vcpu_power_off(vcpu);
 	else
 		vcpu->arch.power_off = false;
 
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index f189d0ad30d5..4a436685c552 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -65,6 +65,8 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
 static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.power_off = true;
+	kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
+	kvm_vcpu_kick(vcpu);
 }
 
 static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
-- 
2.9.3

[PATCH v4 08/11] KVM: arm/arm64: change exit request to sleep request

[Andrew Jones]

A request called EXIT is too generic. All requests are meant to cause
exits, but different requests have different flags. Let's not make
it difficult to decide if the EXIT request is correct for some case
by just always providing unique requests for each case. This patch
changes EXIT to SLEEP, because that's what the request is asking the
VCPU to do.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/arm/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/kvm_host.h |  2 +-
 virt/kvm/arm/arm.c                | 12 ++++++------
 virt/kvm/arm/psci.c               |  4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index c556babe467c..fdd644c01c89 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 \
+#define KVM_REQ_SLEEP \
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0ff991c9c66e..9bd0d1040de9 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 \
+#define KVM_REQ_SLEEP \
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 
 int __attribute_const__ kvm_target_cpu(void);
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9379b1d75ad3..ddc833987dfb 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -371,7 +371,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 static void vcpu_power_off(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.power_off = true;
-	kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
+	kvm_make_request(KVM_REQ_SLEEP, vcpu);
 	kvm_vcpu_kick(vcpu);
 }
 
@@ -543,7 +543,7 @@ void kvm_arm_halt_guest(struct kvm *kvm)
 
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		vcpu->arch.pause = true;
-	kvm_make_all_cpus_request(kvm, KVM_REQ_VCPU_EXIT);
+	kvm_make_all_cpus_request(kvm, KVM_REQ_SLEEP);
 }
 
 void kvm_arm_resume_guest(struct kvm *kvm)
@@ -557,7 +557,7 @@ void kvm_arm_resume_guest(struct kvm *kvm)
 	}
 }
 
-static void vcpu_sleep(struct kvm_vcpu *vcpu)
+static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
 
@@ -566,7 +566,7 @@ static void vcpu_sleep(struct kvm_vcpu *vcpu)
 
 	if (vcpu->arch.power_off || vcpu->arch.pause) {
 		/* Awaken to handle a signal, request we sleep again later. */
-		kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
+		kvm_make_request(KVM_REQ_SLEEP, vcpu);
 	}
 }
 
@@ -578,8 +578,8 @@ static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
 static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 {
 	if (kvm_request_pending(vcpu)) {
-		if (kvm_check_request(KVM_REQ_VCPU_EXIT, vcpu))
-			vcpu_sleep(vcpu);
+		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
+			vcpu_req_sleep(vcpu);
 	}
 }
 
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 4a436685c552..f1e363bab5e8 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -65,7 +65,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
 static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.power_off = true;
-	kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
+	kvm_make_request(KVM_REQ_SLEEP, vcpu);
 	kvm_vcpu_kick(vcpu);
 }
 
@@ -183,7 +183,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
 	 */
 	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
 		tmp->arch.power_off = true;
-	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_VCPU_EXIT);
+	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);
 
 	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
 	vcpu->run->system_event.type = type;
-- 
2.9.3

[PATCH v4 09/11] KVM: arm/arm64: use vcpu requests for irq injection

[Andrew Jones]

Don't use request-less VCPU kicks when injecting IRQs, as a VCPU
kick meant to trigger the interrupt injection could be sent while
the VCPU is outside guest mode, which means no IPI is sent, and
after it has called kvm_vgic_flush_hwstate(), meaning it won't see
the updated GIC state until its next exit some time later for some
other reason.  The receiving VCPU only needs to check this request
in VCPU RUN to handle it.  By checking it, if it's pending, a
memory barrier will be issued that ensures all state is visible.
We still create a vcpu_req_irq_pending() function (which is a nop),
though, in order to allow us to use the standard request checking
pattern.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/arm/include/asm/kvm_host.h   |  1 +
 arch/arm64/include/asm/kvm_host.h |  1 +
 virt/kvm/arm/arm.c                | 12 ++++++++++++
 virt/kvm/arm/vgic/vgic.c          |  9 +++++++--
 4 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index fdd644c01c89..00ad56ee6455 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -46,6 +46,7 @@
 
 #define KVM_REQ_SLEEP \
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
 
 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 9bd0d1040de9..0c4fd1f46e10 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -43,6 +43,7 @@
 
 #define KVM_REQ_SLEEP \
 	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
 
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index ddc833987dfb..73a75ca91e41 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -570,6 +570,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
 	}
 }
 
+static void vcpu_req_irq_pending(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * Nothing to do here. kvm_check_request() already issued a memory
+	 * barrier that pairs with kvm_make_request(), so all hardware state
+	 * we need to flush should now be visible.
+	 */
+}
+
 static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.target >= 0;
@@ -580,6 +589,8 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 	if (kvm_request_pending(vcpu)) {
 		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
 			vcpu_req_sleep(vcpu);
+		if (kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu))
+			vcpu_req_irq_pending(vcpu);
 	}
 }
 
@@ -771,6 +782,7 @@ static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
 	 * trigger a world-switch round on the running physical CPU to set the
 	 * virtual IRQ/FIQ fields in the HCR appropriately.
 	 */
+	kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
 	kvm_vcpu_kick(vcpu);
 
 	return 0;
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index aea080a2c443..c66feaca2a5d 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -286,8 +286,10 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
 		 * won't see this one until it exits for some other
 		 * reason.
 		 */
-		if (vcpu)
+		if (vcpu) {
+			kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
 			kvm_vcpu_kick(vcpu);
+		}
 		return false;
 	}
 
@@ -333,6 +335,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
 	spin_unlock(&irq->irq_lock);
 	spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
 
+	kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
 	kvm_vcpu_kick(vcpu);
 
 	return true;
@@ -722,8 +725,10 @@ void vgic_kick_vcpus(struct kvm *kvm)
 	 * a good kick...
 	 */
 	kvm_for_each_vcpu(c, vcpu, kvm) {
-		if (kvm_vgic_vcpu_pending_irq(vcpu))
+		if (kvm_vgic_vcpu_pending_irq(vcpu)) {
+			kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
 			kvm_vcpu_kick(vcpu);
+		}
 	}
 }
 
-- 
2.9.3

[PATCH v4 10/11] KVM: arm/arm64: PMU: remove request-less vcpu kick

[Andrew Jones]

Refactor PMU overflow handling in order to remove the request-less
vcpu kick.  Now, since kvm_vgic_inject_irq() uses vcpu requests,
there should be no chance that a kick sent at just the wrong time
(between the VCPU's call to kvm_pmu_flush_hwstate() and before it
enters guest mode) results in a failure for the guest to see updated
GIC state until its next exit some time later for some other reason.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/pmu.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 4b43e7f3b158..2451607dc25e 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -203,6 +203,23 @@ static u64 kvm_pmu_overflow_status(struct kvm_vcpu *vcpu)
 	return reg;
 }
 
+static void kvm_pmu_check_overflow(struct kvm_vcpu *vcpu)
+{
+	struct kvm_pmu *pmu = &vcpu->arch.pmu;
+	bool overflow = !!kvm_pmu_overflow_status(vcpu);
+
+	if (pmu->irq_level == overflow)
+		return;
+
+	pmu->irq_level = overflow;
+
+	if (likely(irqchip_in_kernel(vcpu->kvm))) {
+		int ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
+					      pmu->irq_num, overflow);
+		WARN_ON(ret);
+	}
+}
+
 /**
  * kvm_pmu_overflow_set - set PMU overflow interrupt
  * @vcpu: The vcpu pointer
@@ -210,37 +227,18 @@ static u64 kvm_pmu_overflow_status(struct kvm_vcpu *vcpu)
  */
 void kvm_pmu_overflow_set(struct kvm_vcpu *vcpu, u64 val)
 {
-	u64 reg;
-
 	if (val == 0)
 		return;
 
 	vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= val;
-	reg = kvm_pmu_overflow_status(vcpu);
-	if (reg != 0)
-		kvm_vcpu_kick(vcpu);
+	kvm_pmu_check_overflow(vcpu);
 }
 
 static void kvm_pmu_update_state(struct kvm_vcpu *vcpu)
 {
-	struct kvm_pmu *pmu = &vcpu->arch.pmu;
-	bool overflow;
-
 	if (!kvm_arm_pmu_v3_ready(vcpu))
 		return;
-
-	overflow = !!kvm_pmu_overflow_status(vcpu);
-	if (pmu->irq_level == overflow)
-		return;
-
-	pmu->irq_level = overflow;
-
-	if (likely(irqchip_in_kernel(vcpu->kvm))) {
-		int ret;
-		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
-					  pmu->irq_num, overflow);
-		WARN_ON(ret);
-	}
+	kvm_pmu_check_overflow(vcpu);
 }
 
 bool kvm_pmu_should_notify_user(struct kvm_vcpu *vcpu)
-- 
2.9.3

[PATCH v4 11/11] KVM: arm/arm64: timer: remove request-less vcpu kick

[Andrew Jones]

The timer work is only scheduled for a VCPU when that VCPU is
blocked. This means we only need to wake it up, not kick (IPI)
it. While calling kvm_vcpu_kick() would just do the wake up,
and not kick, anyway, let's change this to avoid request-less
vcpu kicks, as they're generally not a good idea (see
"Request-less VCPU Kicks" in
Documentation/virtual/kvm/vcpu-requests.rst)

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 virt/kvm/arm/arch_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 5976609ef27c..c9cd56f39b1e 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -95,7 +95,7 @@ static void kvm_timer_inject_irq_work(struct work_struct *work)
 	 * If the vcpu is blocked we want to wake it up so that it will see
 	 * the timer has expired when entering the guest.
 	 */
-	kvm_vcpu_kick(vcpu);
+	swake_up(kvm_arch_vcpu_wq(vcpu));
 }
 
 static u64 kvm_timer_compute_delta(struct arch_timer_context *timer_ctx)
-- 
2.9.3

Re: [PATCH v4 03/11] KVM: Add documentation for VCPU requests

[Christoffer Dall]

Hi Drew,

On Tue, May 16, 2017 at 04:20:27AM +0200, Andrew Jones wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  Documentation/virtual/kvm/vcpu-requests.rst | 299 ++++++++++++++++++++++++++++
>  1 file changed, 299 insertions(+)
>  create mode 100644 Documentation/virtual/kvm/vcpu-requests.rst
> 
> diff --git a/Documentation/virtual/kvm/vcpu-requests.rst b/Documentation/virtual/kvm/vcpu-requests.rst
> new file mode 100644
> index 000000000000..7a2b4c05c267
> --- /dev/null
> +++ b/Documentation/virtual/kvm/vcpu-requests.rst
> @@ -0,0 +1,299 @@
> +=================
> +KVM VCPU Requests
> +=================
> +
> +Overview
> +========
> +
> +KVM supports an internal API enabling threads to request a VCPU thread to
> +perform some activity.  For example, a thread may request a VCPU to flush
> +its TLB with a VCPU request.  The API consists of the following functions::
> +
> +  /* Check if any requests are pending for VCPU @vcpu. */
> +  bool kvm_request_pending(struct kvm_vcpu *vcpu);
> +
> +  /* Check if VCPU @vcpu has request @req pending. */
> +  bool kvm_test_request(int req, struct kvm_vcpu *vcpu);
> +
> +  /* Clear request @req for VCPU @vcpu. */
> +  void kvm_clear_request(int req, struct kvm_vcpu *vcpu);
> +
> +  /*
> +   * Check if VCPU @vcpu has request @req pending. When the request is
> +   * pending it will be cleared and a memory barrier, which pairs with
> +   * another in kvm_make_request(), will be issued.
> +   */
> +  bool kvm_check_request(int req, struct kvm_vcpu *vcpu);
> +
> +  /*
> +   * Make request @req of VCPU @vcpu. Issues a memory barrier, which pairs
> +   * with another in kvm_check_request(), prior to setting the request.
> +   */
> +  void kvm_make_request(int req, struct kvm_vcpu *vcpu);
> +
> +  /* Make request @req of all VCPUs of the VM with struct kvm @kvm. */
> +  bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req);
> +
> +Typically a requester wants the VCPU to perform the activity as soon
> +as possible after making the request.  This means most requests
> +(kvm_make_request() calls) are followed by a call to kvm_vcpu_kick(),
> +and kvm_make_all_cpus_request() has the kicking of all VCPUs built
> +into it.
> +
> +VCPU Kicks
> +----------
> +
> +The goal of a VCPU kick is to bring a VCPU thread out of guest mode in
> +order to perform some KVM maintenance.  To do so, an IPI is sent, forcing
> +a guest mode exit.  However, a VCPU thread may not be in guest mode at the
> +time of the kick.  Therefore, depending on the mode and state of the VCPU
> +thread, there are two other actions a kick may take.  All three actions
> +are listed below:
> +
> +1) Send an IPI.  This forces a guest mode exit.
> +2) Waking a sleeping VCPU.  Sleeping VCPUs are VCPU threads outside guest
> +   mode that wait on waitqueues.  Waking them removes the threads from
> +   the waitqueues, allowing the threads to run again.  This behavior
> +   may be suppressed, see KVM_REQUEST_NO_WAKEUP below.
> +3) Nothing.  When the VCPU is not in guest mode and the VCPU thread is not
> +   sleeping, then there is nothing to do.
> +
> +VCPU Mode
> +---------
> +
> +VCPUs have a mode state, ``vcpu->mode``, that is used to track whether the
> +guest is running in guest mode or not, as well as some specific
> +outside guest mode states.  The architecture may use ``vcpu->mode`` to
> +ensure VCPU requests are seen by VCPUs (see "Ensuring Requests Are Seen"),
> +as well as to avoid sending unnecessary IPIs (see "IPI Reduction"), and
> +even to ensure IPI acknowledgements are waited upon (see "Waiting for
> +Acknowledgements").  The following modes are defined:
> +
> +OUTSIDE_GUEST_MODE
> +
> +  The VCPU thread is outside guest mode.
> +
> +IN_GUEST_MODE
> +
> +  The VCPU thread is in guest mode.
> +
> +EXITING_GUEST_MODE
> +
> +  The VCPU thread is transitioning from IN_GUEST_MODE to
> +  OUTSIDE_GUEST_MODE.
> +
> +READING_SHADOW_PAGE_TABLES
> +
> +  The VCPU thread is outside guest mode, but it wants the sender of
> +  certain VCPU requests, namely KVM_REQ_TLB_FLUSH, to wait until the VCPU
> +  thread is done reading the page tables.
> +
> +VCPU Request Internals
> +======================
> +
> +VCPU requests are simply bit indices of the ``vcpu->requests`` bitmap.
> +This means general bitops, like those documented in [atomic-ops]_ could
> +also be used, e.g. ::
> +
> +  clear_bit(KVM_REQ_UNHALT & KVM_REQUEST_MASK, &vcpu->requests);
> +
> +However, VCPU request users should refrain from doing so, as it would
> +break the abstraction.  The first 8 bits are reserved for architecture
> +independent requests, all additional bits are available for architecture
> +dependent requests.
> +
> +Architecture Independent Requests
> +---------------------------------
> +
> +KVM_REQ_TLB_FLUSH
> +
> +  KVM's common MMU notifier may need to flush all of a guest's TLB
> +  entries, calling kvm_flush_remote_tlbs() to do so.  Architectures that
> +  choose to use the common kvm_flush_remote_tlbs() implementation will
> +  need to handle this VCPU request.
> +
> +KVM_REQ_MMU_RELOAD
> +
> +  When shadow page tables are used and memory slots are removed it's
> +  necessary to inform each VCPU to completely refresh the tables.  This
> +  request is used for that.
> +
> +KVM_REQ_PENDING_TIMER
> +
> +  This request may be made from a timer handler run on the host on behalf
> +  of a VCPU.  It informs the VCPU thread to inject a timer interrupt.
> +
> +KVM_REQ_UNHALT
> +
> +  This request may be made from the KVM common function kvm_vcpu_block(),
> +  which is used to emulate an instruction that causes a CPU to halt until
> +  one of an architectural specific set of events and/or interrupts is
> +  received (determined by checking kvm_arch_vcpu_runnable()).  When that
> +  event or interrupt arrives kvm_vcpu_block() makes the request.  This is
> +  in contrast to when kvm_vcpu_block() returns due to any other reason,
> +  such as a pending signal, which does not indicate the VCPU's halt
> +  emulation should stop, and therefore does not make the request.
> +
> +KVM_REQUEST_MASK
> +----------------
> +
> +VCPU requests should be masked by KVM_REQUEST_MASK before using them with
> +bitops.  This is because only the lower 8 bits are used to represent the
> +request's number.  The upper bits are used as flags.  Currently only two
> +flags are defined.
> +
> +VCPU Request Flags
> +------------------
> +
> +KVM_REQUEST_NO_WAKEUP
> +
> +  This flag is applied to a request that does not need immediate
> +  attention.  When a request does not need immediate attention, and the

Isn't this an over-simplification?  I thought KVM_REQUEST_NO_WAKEUP was
only used in cases where you simply want to make sure the VCPU is not
ignoring a request while exexuting in the guest, but if it's sleeping
and therefore not in the guest, it can just handle the request whenever
it wakes up anyway.  So perhaps something like:

   This flag is applied to a request that only needs immediate attention
   from VCPUs running in the guest.  That is, sleeping VCPUs do not need
   to be removed from their waitqueues but can handle this request when
   they wake up for any other reason.

> +  VCPU's thread is outside guest mode sleeping, then the thread is not
> +  awaken by a kick.
> +
> +KVM_REQUEST_WAIT
> +
> +  When requests with this flag are made with kvm_make_all_cpus_request(),
> +  then the caller will wait for each VCPU to acknowledge the IPI before
> +  proceeding.

How does the interaction work with KVM_REQUEST_NO_WAKEUP?  Does it mean
it only waits on those VCPUs to which is needs to send an IPI, but the
rest are ignored ?

> +
> +VCPU Requests with Associated State
> +===================================
> +
> +Requesters that want the receiving VCPU to handle new state need to ensure
> +the newly written state is observable to the receiving VCPU thread's CPU
> +by the time it observes the request.  This means a write memory barrier
> +must be inserted after writing the new state and before setting the VCPU
> +request bit.  Additionally, on the receiving VCPU thread's side, a
> +corresponding read barrier must be inserted after reading the request bit
> +and before proceeding to read the new state associated with it.  See
> +scenario 3, Message and Flag, of [lwn-mb]_ and the kernel documentation
> +[memory-barriers]_.
> +
> +The pair of functions, kvm_check_request() and kvm_make_request(), provide
> +the memory barriers, allowing this requirement to be handled internally by
> +the API.
> +
> +Ensuring Requests Are Seen
> +==========================
> +
> +When making requests to VCPUs, we want to avoid the receiving VCPU
> +executing in guest mode for an arbitrary long time without handling the
> +request.  We can be sure this won't happen as long as we ensure the VCPU
> +thread checks kvm_request_pending() before entering guest mode and that a
> +kick will send an IPI when necessary.  Extra care must be taken to cover

                        ^ to force an exit from the guest

> +the period after the VCPU thread's last kvm_request_pending() check and
> +before it has entered guest mode, as kick IPIs will only trigger VCPU run
> +loops for VCPU threads that are in guest mode or at least have already

IPIs trigger VCPU run loops?  I think you mean that IPIs triggers a
return from guest mode which eventually results in running another
iteration of the run loop?

> +disabled interrupts in order to prepare to enter guest mode.  This means
> +that an optimized implementation (see "IPI Reduction") must be certain
> +when it's safe to not send the IPI.  One solution, which all architectures
> +except s390 apply, is to:
> +
> +- set ``vcpu->mode`` to IN_GUEST_MODE between disabling the interrupts and
> +  the last kvm_request_pending() check;
> +- enable interrupts atomically when entering the guest.
> +
> +This solution also requires memory barriers to be placed carefully in both
> +the requesting thread and the receiving VCPU.  With the memory barriers we
> +can exclude the possibility of a VCPU thread observing
> +!kvm_request_pending() on its last check and then not receiving an IPI for
> +the next request made of it, even if the request is made immediately after
> +the check.  This is done by way of the Dekker memory barrier pattern
> +(scenario 10 of [lwn-mb]_).  As the Dekker pattern requires two variables,
> +this solution pairs ``vcpu->mode`` with ``vcpu->requests``.  Substituting
> +them into the pattern gives::
> +
> +  CPU1                                    CPU2
> +  =================                       =================
> +  local_irq_disable();
> +  WRITE_ONCE(vcpu->mode, IN_GUEST_MODE);  kvm_make_request(REQ, vcpu);
> +  smp_mb();                               smp_mb();
> +  if (kvm_request_pending(vcpu)) {        if (READ_ONCE(vcpu->mode) ==
> +                                              IN_GUEST_MODE) {
> +      ...abort guest entry...                 ...send IPI...
> +  }                                       }
> +
> +As stated above, the IPI is only useful for VCPU threads in guest mode or
> +that have already disabled interrupts.  This is why this specific case of
> +the Dekker pattern has been extended to disable interrupts before setting
> +``vcpu->mode`` to IN_GUEST_MODE.  WRITE_ONCE() and READ_ONCE() are used to
> +pedantically implement the memory barrier pattern, guaranteeing the
> +compiler doesn't interfere with ``vcpu->mode``'s carefully planned
> +accesses.
> +
> +IPI Reduction
> +-------------
> +
> +As only one IPI is needed to get a VCPU to check for any/all requests,
> +then they may be coalesced.  This is easily done by having the first IPI
> +sending kick also change the VCPU mode to something !IN_GUEST_MODE.  The
> +transitional state, EXITING_GUEST_MODE, is used for this purpose.
> +
> +Waiting for Acknowledgements
> +----------------------------
> +
> +Some requests, those with the KVM_REQUEST_WAIT flag set, require IPIs to
> +be sent, and the acknowledgements to be waited upon, even when the target
> +VCPU threads are in modes other than IN_GUEST_MODE.  For example, one case
> +is when a target VCPU thread is in READING_SHADOW_PAGE_TABLES mode, which
> +is set after disabling interrupts.  For these cases, the "should send an
> +IPI" condition becomes READ_ONCE(``vcpu->mode``) != OUTSIDE_GUEST_MODE.

Hmm, did you mean, "To support these cases, the condition for sending an
IPI checks not to be equal to IN_GUEST_MODE, but different from
OUTSIDE_GUEST_MODE.".

(The confusion is whether we check different things depending on which
request we're dealing with, or just explaining why the single
implementation is done the way it is.)

> +
> +Request-less VCPU Kicks
> +-----------------------
> +
> +As the determination of whether or not to send an IPI depends on the
> +two-variable Dekker memory barrier pattern, then it's clear that
> +request-less VCPU kicks are almost never correct.  Without the assurance
> +that a non-IPI generating kick will still result in an action by the
> +receiving VCPU, as the final kvm_request_pending() check does for
> +request-accompanying kicks, then the kick may not do anything useful at
> +all.  If, for instance, a request-less kick was made to a VCPU that was
> +just about to set its mode to IN_GUEST_MODE, meaning no IPI is sent, then
> +the VCPU thread may continue its entry without actually having done
> +whatever it was the kick was meant to initiate.
> +
> +One exception is x86's posted interrupt mechanism.  In this case, however,
> +even the request-less VCPU kick is coupled with the same
> +local_irq_disable() + smp_mb() pattern described above; the ON bit
> +(Outstanding Notification) in the posted interrupt descriptor takes the
> +role of ``vcpu->requests``.  When sending a posted interrupt, PIR.ON is
> +set before reading ``vcpu->mode``; dually, in the VCPU thread,
> +vmx_sync_pir_to_irr() reads PIR after setting ``vcpu->mode`` to
> +IN_GUEST_MODE.
> +
> +Additional Considerations
> +=========================
> +
> +Sleeping VCPUs
> +--------------
> +
> +VCPU threads may need to consider requests before and/or after calling
> +functions that may put them to sleep, e.g. kvm_vcpu_block().  Whether they
> +do or not, and, if they do, which requests need consideration, is
> +architecture dependent.  kvm_vcpu_block() calls kvm_arch_vcpu_runnable()
> +to check if it should awaken.  One reason to do so is to provide
> +architectures a function where requests may be checked if necessary.
> +
> +Clearing Requests
> +-----------------
> +
> +Generally it only makes sense for the receiving VCPU thread to clear a
> +request.  However, in some circumstances, such as when the requesting
> +thread is executing synchronously with the receiving VCPU thread, it's

what does it mean that the requesting thread is executing synchronously
with the receiving VCPU thread?  How can that ever be enforced on SMP
system, if the two threads are not the same thread?

If we simply mean that some sequence of operations between the two
threads is synchronized (for example using locks), then that's a
different (less general) case from the two thread overall executing
synchronously, I think?

> +possible to know that the request may be cleared immediately, rather than
> +waiting for the receiving VCPU thread to handle the request in VCPU RUN.
> +The only current examples of this are kvm_vcpu_block() calls, where a
> +side-effect of a call may be to set KVM_REQ_UNHALT.  When the requesting
> +thread is itself the receiving VCPU, 

when the requsting thread and the VCPU thread are the same thread ?

> then it's possible to know that the
> +request does not need to be handled in VCPU RUN, and therefore may be
> +cleared immediately.
> +
> +References
> +==========
> +
> +.. [atomic-ops] Documentation/core-api/atomic_ops.rst
> +.. [memory-barriers] Documentation/memory-barriers.txt
> +.. [lwn-mb] https://lwn.net/Articles/573436/
> -- 
> 2.9.3
> 

This write up is excellent.

My comments are mostly for clarification, so:

Acked-by: Christoffer Dall <cdall@linaro.org>


Thanks,
-Christoffer

Re: [PATCH v4 03/11] KVM: Add documentation for VCPU requests

[Andrew Jones]

On Fri, May 26, 2017 at 09:31:00AM +0200, Christoffer Dall wrote:
> Hi Drew,
> 
> On Tue, May 16, 2017 at 04:20:27AM +0200, Andrew Jones wrote:
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  Documentation/virtual/kvm/vcpu-requests.rst | 299 ++++++++++++++++++++++++++++
> >  1 file changed, 299 insertions(+)
> >  create mode 100644 Documentation/virtual/kvm/vcpu-requests.rst
> > 
> > diff --git a/Documentation/virtual/kvm/vcpu-requests.rst b/Documentation/virtual/kvm/vcpu-requests.rst
> > new file mode 100644
> > index 000000000000..7a2b4c05c267
> > --- /dev/null
> > +++ b/Documentation/virtual/kvm/vcpu-requests.rst
> > @@ -0,0 +1,299 @@
> > +=================
> > +KVM VCPU Requests
> > +=================
> > +
> > +Overview
> > +========
> > +
> > +KVM supports an internal API enabling threads to request a VCPU thread to
> > +perform some activity.  For example, a thread may request a VCPU to flush
> > +its TLB with a VCPU request.  The API consists of the following functions::
> > +
> > +  /* Check if any requests are pending for VCPU @vcpu. */
> > +  bool kvm_request_pending(struct kvm_vcpu *vcpu);
> > +
> > +  /* Check if VCPU @vcpu has request @req pending. */
> > +  bool kvm_test_request(int req, struct kvm_vcpu *vcpu);
> > +
> > +  /* Clear request @req for VCPU @vcpu. */
> > +  void kvm_clear_request(int req, struct kvm_vcpu *vcpu);
> > +
> > +  /*
> > +   * Check if VCPU @vcpu has request @req pending. When the request is
> > +   * pending it will be cleared and a memory barrier, which pairs with
> > +   * another in kvm_make_request(), will be issued.
> > +   */
> > +  bool kvm_check_request(int req, struct kvm_vcpu *vcpu);
> > +
> > +  /*
> > +   * Make request @req of VCPU @vcpu. Issues a memory barrier, which pairs
> > +   * with another in kvm_check_request(), prior to setting the request.
> > +   */
> > +  void kvm_make_request(int req, struct kvm_vcpu *vcpu);
> > +
> > +  /* Make request @req of all VCPUs of the VM with struct kvm @kvm. */
> > +  bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req);
> > +
> > +Typically a requester wants the VCPU to perform the activity as soon
> > +as possible after making the request.  This means most requests
> > +(kvm_make_request() calls) are followed by a call to kvm_vcpu_kick(),
> > +and kvm_make_all_cpus_request() has the kicking of all VCPUs built
> > +into it.
> > +
> > +VCPU Kicks
> > +----------
> > +
> > +The goal of a VCPU kick is to bring a VCPU thread out of guest mode in
> > +order to perform some KVM maintenance.  To do so, an IPI is sent, forcing
> > +a guest mode exit.  However, a VCPU thread may not be in guest mode at the
> > +time of the kick.  Therefore, depending on the mode and state of the VCPU
> > +thread, there are two other actions a kick may take.  All three actions
> > +are listed below:
> > +
> > +1) Send an IPI.  This forces a guest mode exit.
> > +2) Waking a sleeping VCPU.  Sleeping VCPUs are VCPU threads outside guest
> > +   mode that wait on waitqueues.  Waking them removes the threads from
> > +   the waitqueues, allowing the threads to run again.  This behavior
> > +   may be suppressed, see KVM_REQUEST_NO_WAKEUP below.
> > +3) Nothing.  When the VCPU is not in guest mode and the VCPU thread is not
> > +   sleeping, then there is nothing to do.
> > +
> > +VCPU Mode
> > +---------
> > +
> > +VCPUs have a mode state, ``vcpu->mode``, that is used to track whether the
> > +guest is running in guest mode or not, as well as some specific
> > +outside guest mode states.  The architecture may use ``vcpu->mode`` to
> > +ensure VCPU requests are seen by VCPUs (see "Ensuring Requests Are Seen"),
> > +as well as to avoid sending unnecessary IPIs (see "IPI Reduction"), and
> > +even to ensure IPI acknowledgements are waited upon (see "Waiting for
> > +Acknowledgements").  The following modes are defined:
> > +
> > +OUTSIDE_GUEST_MODE
> > +
> > +  The VCPU thread is outside guest mode.
> > +
> > +IN_GUEST_MODE
> > +
> > +  The VCPU thread is in guest mode.
> > +
> > +EXITING_GUEST_MODE
> > +
> > +  The VCPU thread is transitioning from IN_GUEST_MODE to
> > +  OUTSIDE_GUEST_MODE.
> > +
> > +READING_SHADOW_PAGE_TABLES
> > +
> > +  The VCPU thread is outside guest mode, but it wants the sender of
> > +  certain VCPU requests, namely KVM_REQ_TLB_FLUSH, to wait until the VCPU
> > +  thread is done reading the page tables.
> > +
> > +VCPU Request Internals
> > +======================
> > +
> > +VCPU requests are simply bit indices of the ``vcpu->requests`` bitmap.
> > +This means general bitops, like those documented in [atomic-ops]_ could
> > +also be used, e.g. ::
> > +
> > +  clear_bit(KVM_REQ_UNHALT & KVM_REQUEST_MASK, &vcpu->requests);
> > +
> > +However, VCPU request users should refrain from doing so, as it would
> > +break the abstraction.  The first 8 bits are reserved for architecture
> > +independent requests, all additional bits are available for architecture
> > +dependent requests.
> > +
> > +Architecture Independent Requests
> > +---------------------------------
> > +
> > +KVM_REQ_TLB_FLUSH
> > +
> > +  KVM's common MMU notifier may need to flush all of a guest's TLB
> > +  entries, calling kvm_flush_remote_tlbs() to do so.  Architectures that
> > +  choose to use the common kvm_flush_remote_tlbs() implementation will
> > +  need to handle this VCPU request.
> > +
> > +KVM_REQ_MMU_RELOAD
> > +
> > +  When shadow page tables are used and memory slots are removed it's
> > +  necessary to inform each VCPU to completely refresh the tables.  This
> > +  request is used for that.
> > +
> > +KVM_REQ_PENDING_TIMER
> > +
> > +  This request may be made from a timer handler run on the host on behalf
> > +  of a VCPU.  It informs the VCPU thread to inject a timer interrupt.
> > +
> > +KVM_REQ_UNHALT
> > +
> > +  This request may be made from the KVM common function kvm_vcpu_block(),
> > +  which is used to emulate an instruction that causes a CPU to halt until
> > +  one of an architectural specific set of events and/or interrupts is
> > +  received (determined by checking kvm_arch_vcpu_runnable()).  When that
> > +  event or interrupt arrives kvm_vcpu_block() makes the request.  This is
> > +  in contrast to when kvm_vcpu_block() returns due to any other reason,
> > +  such as a pending signal, which does not indicate the VCPU's halt
> > +  emulation should stop, and therefore does not make the request.
> > +
> > +KVM_REQUEST_MASK
> > +----------------
> > +
> > +VCPU requests should be masked by KVM_REQUEST_MASK before using them with
> > +bitops.  This is because only the lower 8 bits are used to represent the
> > +request's number.  The upper bits are used as flags.  Currently only two
> > +flags are defined.
> > +
> > +VCPU Request Flags
> > +------------------
> > +
> > +KVM_REQUEST_NO_WAKEUP
> > +
> > +  This flag is applied to a request that does not need immediate
> > +  attention.  When a request does not need immediate attention, and the
> 
> Isn't this an over-simplification?  I thought KVM_REQUEST_NO_WAKEUP was
> only used in cases where you simply want to make sure the VCPU is not
> ignoring a request while exexuting in the guest, but if it's sleeping
> and therefore not in the guest, it can just handle the request whenever
> it wakes up anyway.  So perhaps something like:
> 
>    This flag is applied to a request that only needs immediate attention
>    from VCPUs running in the guest.  That is, sleeping VCPUs do not need
>    to be removed from their waitqueues but can handle this request when
>    they wake up for any other reason.

I like this paragraph. I'll use it in v5.

> 
> > +  VCPU's thread is outside guest mode sleeping, then the thread is not
> > +  awaken by a kick.
> > +
> > +KVM_REQUEST_WAIT
> > +
> > +  When requests with this flag are made with kvm_make_all_cpus_request(),
> > +  then the caller will wait for each VCPU to acknowledge the IPI before
> > +  proceeding.
> 
> How does the interaction work with KVM_REQUEST_NO_WAKEUP?  Does it mean
> it only waits on those VCPUs to which is needs to send an IPI, but the
> rest are ignored ?

I'll discuss interaction with KVM_REQUEST_NO_WAKEUP here and also refer
to the "Waiting for Acknowledgements" section below.

> 
> > +
> > +VCPU Requests with Associated State
> > +===================================
> > +
> > +Requesters that want the receiving VCPU to handle new state need to ensure
> > +the newly written state is observable to the receiving VCPU thread's CPU
> > +by the time it observes the request.  This means a write memory barrier
> > +must be inserted after writing the new state and before setting the VCPU
> > +request bit.  Additionally, on the receiving VCPU thread's side, a
> > +corresponding read barrier must be inserted after reading the request bit
> > +and before proceeding to read the new state associated with it.  See
> > +scenario 3, Message and Flag, of [lwn-mb]_ and the kernel documentation
> > +[memory-barriers]_.
> > +
> > +The pair of functions, kvm_check_request() and kvm_make_request(), provide
> > +the memory barriers, allowing this requirement to be handled internally by
> > +the API.
> > +
> > +Ensuring Requests Are Seen
> > +==========================
> > +
> > +When making requests to VCPUs, we want to avoid the receiving VCPU
> > +executing in guest mode for an arbitrary long time without handling the
> > +request.  We can be sure this won't happen as long as we ensure the VCPU
> > +thread checks kvm_request_pending() before entering guest mode and that a
> > +kick will send an IPI when necessary.  Extra care must be taken to cover
> 
>                         ^ to force an exit from the guest
> 
> > +the period after the VCPU thread's last kvm_request_pending() check and
> > +before it has entered guest mode, as kick IPIs will only trigger VCPU run
> > +loops for VCPU threads that are in guest mode or at least have already
> 
> IPIs trigger VCPU run loops?  I think you mean that IPIs triggers a
> return from guest mode which eventually results in running another
> iteration of the run loop?

yeah, that's what I mean. I'll clarify it.

> 
> > +disabled interrupts in order to prepare to enter guest mode.  This means
> > +that an optimized implementation (see "IPI Reduction") must be certain
> > +when it's safe to not send the IPI.  One solution, which all architectures
> > +except s390 apply, is to:
> > +
> > +- set ``vcpu->mode`` to IN_GUEST_MODE between disabling the interrupts and
> > +  the last kvm_request_pending() check;
> > +- enable interrupts atomically when entering the guest.
> > +
> > +This solution also requires memory barriers to be placed carefully in both
> > +the requesting thread and the receiving VCPU.  With the memory barriers we
> > +can exclude the possibility of a VCPU thread observing
> > +!kvm_request_pending() on its last check and then not receiving an IPI for
> > +the next request made of it, even if the request is made immediately after
> > +the check.  This is done by way of the Dekker memory barrier pattern
> > +(scenario 10 of [lwn-mb]_).  As the Dekker pattern requires two variables,
> > +this solution pairs ``vcpu->mode`` with ``vcpu->requests``.  Substituting
> > +them into the pattern gives::
> > +
> > +  CPU1                                    CPU2
> > +  =================                       =================
> > +  local_irq_disable();
> > +  WRITE_ONCE(vcpu->mode, IN_GUEST_MODE);  kvm_make_request(REQ, vcpu);
> > +  smp_mb();                               smp_mb();
> > +  if (kvm_request_pending(vcpu)) {        if (READ_ONCE(vcpu->mode) ==
> > +                                              IN_GUEST_MODE) {
> > +      ...abort guest entry...                 ...send IPI...
> > +  }                                       }
> > +
> > +As stated above, the IPI is only useful for VCPU threads in guest mode or
> > +that have already disabled interrupts.  This is why this specific case of
> > +the Dekker pattern has been extended to disable interrupts before setting
> > +``vcpu->mode`` to IN_GUEST_MODE.  WRITE_ONCE() and READ_ONCE() are used to
> > +pedantically implement the memory barrier pattern, guaranteeing the
> > +compiler doesn't interfere with ``vcpu->mode``'s carefully planned
> > +accesses.
> > +
> > +IPI Reduction
> > +-------------
> > +
> > +As only one IPI is needed to get a VCPU to check for any/all requests,
> > +then they may be coalesced.  This is easily done by having the first IPI
> > +sending kick also change the VCPU mode to something !IN_GUEST_MODE.  The
> > +transitional state, EXITING_GUEST_MODE, is used for this purpose.
> > +
> > +Waiting for Acknowledgements
> > +----------------------------
> > +
> > +Some requests, those with the KVM_REQUEST_WAIT flag set, require IPIs to
> > +be sent, and the acknowledgements to be waited upon, even when the target
> > +VCPU threads are in modes other than IN_GUEST_MODE.  For example, one case
> > +is when a target VCPU thread is in READING_SHADOW_PAGE_TABLES mode, which
> > +is set after disabling interrupts.  For these cases, the "should send an
> > +IPI" condition becomes READ_ONCE(``vcpu->mode``) != OUTSIDE_GUEST_MODE.
> 
> Hmm, did you mean, "To support these cases, the condition for sending an
> IPI checks not to be equal to IN_GUEST_MODE, but different from
> OUTSIDE_GUEST_MODE.".

Yup, I'll clarify it.

> 
> (The confusion is whether we check different things depending on which
> request we're dealing with, or just explaining why the single
> implementation is done the way it is.)

Checking different things depends on the request type (requests
flagged with KVM_REQUEST_WAIT). I'll clarify this.

> 
> > +
> > +Request-less VCPU Kicks
> > +-----------------------
> > +
> > +As the determination of whether or not to send an IPI depends on the
> > +two-variable Dekker memory barrier pattern, then it's clear that
> > +request-less VCPU kicks are almost never correct.  Without the assurance
> > +that a non-IPI generating kick will still result in an action by the
> > +receiving VCPU, as the final kvm_request_pending() check does for
> > +request-accompanying kicks, then the kick may not do anything useful at
> > +all.  If, for instance, a request-less kick was made to a VCPU that was
> > +just about to set its mode to IN_GUEST_MODE, meaning no IPI is sent, then
> > +the VCPU thread may continue its entry without actually having done
> > +whatever it was the kick was meant to initiate.
> > +
> > +One exception is x86's posted interrupt mechanism.  In this case, however,
> > +even the request-less VCPU kick is coupled with the same
> > +local_irq_disable() + smp_mb() pattern described above; the ON bit
> > +(Outstanding Notification) in the posted interrupt descriptor takes the
> > +role of ``vcpu->requests``.  When sending a posted interrupt, PIR.ON is
> > +set before reading ``vcpu->mode``; dually, in the VCPU thread,
> > +vmx_sync_pir_to_irr() reads PIR after setting ``vcpu->mode`` to
> > +IN_GUEST_MODE.
> > +
> > +Additional Considerations
> > +=========================
> > +
> > +Sleeping VCPUs
> > +--------------
> > +
> > +VCPU threads may need to consider requests before and/or after calling
> > +functions that may put them to sleep, e.g. kvm_vcpu_block().  Whether they
> > +do or not, and, if they do, which requests need consideration, is
> > +architecture dependent.  kvm_vcpu_block() calls kvm_arch_vcpu_runnable()
> > +to check if it should awaken.  One reason to do so is to provide
> > +architectures a function where requests may be checked if necessary.
> > +
> > +Clearing Requests
> > +-----------------
> > +
> > +Generally it only makes sense for the receiving VCPU thread to clear a
> > +request.  However, in some circumstances, such as when the requesting
> > +thread is executing synchronously with the receiving VCPU thread, it's
> 
> what does it mean that the requesting thread is executing synchronously
> with the receiving VCPU thread?  How can that ever be enforced on SMP
> system, if the two threads are not the same thread?
> 
> If we simply mean that some sequence of operations between the two
> threads is synchronized (for example using locks), then that's a
> different (less general) case from the two thread overall executing
> synchronously, I think?

I meant with locks - not a general synchronization, but a temporarily
enforced one. I'll try to clarify that language.

> 
> > +possible to know that the request may be cleared immediately, rather than
> > +waiting for the receiving VCPU thread to handle the request in VCPU RUN.
> > +The only current examples of this are kvm_vcpu_block() calls, where a
> > +side-effect of a call may be to set KVM_REQ_UNHALT.  When the requesting
> > +thread is itself the receiving VCPU, 
> 
> when the requsting thread and the VCPU thread are the same thread ?

I use your language here as it's clearer.

> 
> > then it's possible to know that the
> > +request does not need to be handled in VCPU RUN, and therefore may be
> > +cleared immediately.
> > +
> > +References
> > +==========
> > +
> > +.. [atomic-ops] Documentation/core-api/atomic_ops.rst
> > +.. [memory-barriers] Documentation/memory-barriers.txt
> > +.. [lwn-mb] https://lwn.net/Articles/573436/
> > -- 
> > 2.9.3
> > 
> 
> This write up is excellent.

Thanks!

> 
> My comments are mostly for clarification, so:
> 
> Acked-by: Christoffer Dall <cdall@linaro.org>
> 
> 
> Thanks,
> -Christoffer

drew

Re: [PATCH v4 11/11] KVM: arm/arm64: timer: remove request-less vcpu kick

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:35AM +0200, Andrew Jones wrote:
> The timer work is only scheduled for a VCPU when that VCPU is
> blocked. This means we only need to wake it up, not kick (IPI)
> it. While calling kvm_vcpu_kick() would just do the wake up,
> and not kick, anyway, let's change this to avoid request-less
> vcpu kicks, as they're generally not a good idea (see
> "Request-less VCPU Kicks" in
> Documentation/virtual/kvm/vcpu-requests.rst)
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  virt/kvm/arm/arch_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 5976609ef27c..c9cd56f39b1e 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -95,7 +95,7 @@ static void kvm_timer_inject_irq_work(struct work_struct *work)
>  	 * If the vcpu is blocked we want to wake it up so that it will see
>  	 * the timer has expired when entering the guest.
>  	 */
> -	kvm_vcpu_kick(vcpu);
> +	swake_up(kvm_arch_vcpu_wq(vcpu));

We have kvm_vcpu_wake_up().  Why not use that?

Thanks,
-Christoffer

>  }
>  
>  static u64 kvm_timer_compute_delta(struct arch_timer_context *timer_ctx)
> -- 
> 2.9.3
> 

Re: [PATCH v4 01/11] KVM: improve arch vcpu request defining

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:25AM +0200, Andrew Jones wrote:
> 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 Krcmar'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>

Acked-by: Christoffer Dall <cdall@linaro.org>

> ---
>  arch/arm/include/asm/kvm_host.h     |  3 ++-
>  arch/arm64/include/asm/kvm_host.h   |  3 ++-
>  arch/powerpc/include/asm/kvm_host.h |  4 ++--
>  arch/s390/include/asm/kvm_host.h    |  6 ++---
>  arch/x86/include/asm/kvm_host.h     | 47 ++++++++++++++++++++-----------------
>  include/linux/kvm_host.h            |  7 ++++++
>  6 files changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 12274d46df70..c556babe467c 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -44,7 +44,8 @@
>  #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
>  #endif
>  
> -#define KVM_REQ_VCPU_EXIT	(8 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> +#define KVM_REQ_VCPU_EXIT \
> +	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | 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 32cbe8a3bb0d..0ff991c9c66e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -41,7 +41,8 @@
>  
>  #define KVM_VCPU_MAX_FEATURES 4
>  
> -#define KVM_REQ_VCPU_EXIT	(8 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> +#define KVM_REQ_VCPU_EXIT \
> +	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  
>  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 9c51ac4b8f36..50e0bc9723cc 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 9c761fea0c98..563979976fab 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -48,28 +48,31 @@
>  #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_WAIT | KVM_REQUEST_NO_WAKEUP)
> -#define KVM_REQ_SCAN_IOAPIC       (23 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> -#define KVM_REQ_GLOBAL_CLOCK_UPDATE 24
> -#define KVM_REQ_APIC_PAGE_RELOAD  (25 | KVM_REQUEST_WAIT | 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_FLAGS(14, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> +#define KVM_REQ_SCAN_IOAPIC \
> +	KVM_ARCH_REQ_FLAGS(15, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> +#define KVM_REQ_GLOBAL_CLOCK_UPDATE	KVM_ARCH_REQ(16)
> +#define KVM_REQ_APIC_PAGE_RELOAD \
> +	KVM_ARCH_REQ_FLAGS(17, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> +#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 8c0664309815..3724b51aab64 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -126,6 +126,13 @@ static inline bool is_error_page(struct page *page)
>  #define KVM_REQ_MMU_RELOAD        (1 | KVM_REQUEST_WAIT | 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_USERSPACE_IRQ_SOURCE_ID		0
>  #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
> -- 
> 2.9.3
> 

Re: [PATCH v4 02/11] KVM: add kvm_request_pending

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:26AM +0200, Andrew Jones wrote:
> From: Radim Krčmář <rkrcmar@redhat.com>
> 
> A first step in vcpu->requests encapsulation.  Additionally, we now
> use READ_ONCE() when accessing vcpu->requests, which ensures we
> always load vcpu->requests when it's accessed.  This is important as
> other threads can change it any time.  Also, READ_ONCE() documents
> that vcpu->requests is used with other threads, likely requiring
> memory barriers, which it does.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> [ Documented the new use of READ_ONCE() and converted another check
>   in arch/mips/kvm/vz.c ]
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Acked-by: Christoffer Dall <cdall@linaro.org>

> ---
>  arch/mips/kvm/trap_emul.c  | 2 +-
>  arch/mips/kvm/vz.c         | 2 +-
>  arch/powerpc/kvm/booke.c   | 2 +-
>  arch/powerpc/kvm/powerpc.c | 5 ++---
>  arch/s390/kvm/kvm-s390.c   | 2 +-
>  arch/x86/kvm/x86.c         | 4 ++--
>  include/linux/kvm_host.h   | 5 +++++
>  7 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/mips/kvm/trap_emul.c b/arch/mips/kvm/trap_emul.c
> index a563759fd142..6a0d7040d882 100644
> --- a/arch/mips/kvm/trap_emul.c
> +++ b/arch/mips/kvm/trap_emul.c
> @@ -1094,7 +1094,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_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
> index 71d8856ade64..74805035edc8 100644
> --- a/arch/mips/kvm/vz.c
> +++ b/arch/mips/kvm/vz.c
> @@ -2337,7 +2337,7 @@ static int kvm_vz_check_requests(struct kvm_vcpu *vcpu, int cpu)
>  	int ret = 0;
>  	int i;
>  
> -	if (!vcpu->requests)
> +	if (!kvm_request_pending(vcpu))
>  		return 0;
>  
>  	if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 3eaac3809977..071b87ee682f 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -687,7 +687,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 f7cf2cd564ef..fd64f087737c 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -55,8 +55,7 @@ EXPORT_SYMBOL_GPL(kvmppc_pr_ops);
>  
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>  {
> -	return !!(v->arch.pending_exceptions) ||
> -	       v->requests;
> +	return !!(v->arch.pending_exceptions) || kvm_request_pending(v);
>  }
>  
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
> @@ -108,7 +107,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 689ac48361c6..ad41e0fa3a21 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2440,7 +2440,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 464da936c53d..f81060518635 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6710,7 +6710,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_check_request(KVM_REQ_MMU_RELOAD, vcpu))
>  			kvm_mmu_unload(vcpu);
>  		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
> @@ -6874,7 +6874,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 3724b51aab64..0b50e7b35ed4 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1105,6 +1105,11 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
>  	set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
>  }
>  
> +static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
> +{
> +	return READ_ONCE(vcpu->requests);
> +}
> +
>  static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
>  {
>  	return test_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
> -- 
> 2.9.3
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 05/11] KVM: arm/arm64: replace pause checks with vcpu request checks

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:29AM +0200, Andrew Jones wrote:
> The current use of KVM_REQ_VCPU_EXIT for pause is fine.  Even the
> requester clearing the request is OK, as this is the special case
> where the sole requesting thread and receiving VCPU are executing
> synchronously (see "Clearing Requests" in
> Documentation/virtual/kvm/vcpu-requests.rst) However, that's about
> to change, so let's ensure only the receiving VCPU clears the
> request. Additionally, by guaranteeing KVM_REQ_VCPU_EXIT is always
> set when pause is, we can avoid checking pause directly in VCPU RUN.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  virt/kvm/arm/arm.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 138212605ad9..21a4db90073f 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -546,7 +546,6 @@ void kvm_arm_resume_guest(struct kvm *kvm)
>  
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
>  		vcpu->arch.pause = false;
> -		kvm_clear_request(KVM_REQ_VCPU_EXIT, vcpu);
>  		swake_up(kvm_arch_vcpu_wq(vcpu));
>  	}
>  }
> @@ -557,6 +556,11 @@ static void vcpu_sleep(struct kvm_vcpu *vcpu)
>  
>  	swait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
>  				       (!vcpu->arch.pause)));
> +
> +	if (vcpu->arch.pause) {
> +		/* Awaken to handle a signal, request we sleep again later. */
> +		kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
> +	}
>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> @@ -564,6 +568,14 @@ static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>  	return vcpu->arch.target >= 0;
>  }
>  
> +static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_request_pending(vcpu)) {
> +		if (kvm_check_request(KVM_REQ_VCPU_EXIT, vcpu))
> +			vcpu_sleep(vcpu);
> +	}
> +}
> +
>  /**
>   * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
>   * @vcpu:	The VCPU pointer
> @@ -609,7 +621,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		update_vttbr(vcpu->kvm);
>  
> -		if (vcpu->arch.power_off || vcpu->arch.pause)
> +		check_vcpu_requests(vcpu);
> +
> +		if (vcpu->arch.power_off)
>  			vcpu_sleep(vcpu);
>  
>  		/*
> @@ -649,7 +663,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>  		    kvm_request_pending(vcpu) ||
> -		    vcpu->arch.power_off || vcpu->arch.pause) {
> +		    vcpu->arch.power_off) {
>  			vcpu->mode = OUTSIDE_GUEST_MODE;
>  			local_irq_enable();
>  			kvm_pmu_sync_hwstate(vcpu);
> -- 
> 2.9.3
> 

Re: [PATCH v4 06/11] KVM: arm/arm64: use vcpu requests for power_off

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:30AM +0200, Andrew Jones wrote:
> System shutdown is currently using request-less VCPU kicks. This
> leaves open a tiny race window, as it doesn't ensure the state
> change to power_off is seen by a VCPU just about to enter guest
> mode. VCPU requests, OTOH, are guaranteed to be seen (see "Ensuring
> Requests Are Seen" of Documentation/virtual/kvm/vcpu-requests.rst)
> This patch applies the EXIT request used by pause to power_off,
> fixing the race.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  virt/kvm/arm/psci.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index f68be2cc6256..f189d0ad30d5 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -179,10 +179,9 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  	 * after this call is handled and before the VCPUs have been
>  	 * re-initialized.
>  	 */
> -	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> +	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
>  		tmp->arch.power_off = true;
> -		kvm_vcpu_kick(tmp);
> -	}
> +	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_VCPU_EXIT);
>  
>  	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
>  	vcpu->run->system_event.type = type;
> -- 
> 2.9.3
> 

Re: [PATCH v4 06/11] KVM: arm/arm64: use vcpu requests for power_off

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:30AM +0200, Andrew Jones wrote:
> System shutdown is currently using request-less VCPU kicks. This
> leaves open a tiny race window, as it doesn't ensure the state
> change to power_off is seen by a VCPU just about to enter guest
> mode. VCPU requests, OTOH, are guaranteed to be seen (see "Ensuring
> Requests Are Seen" of Documentation/virtual/kvm/vcpu-requests.rst)
> This patch applies the EXIT request used by pause to power_off,
> fixing the race.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  virt/kvm/arm/psci.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index f68be2cc6256..f189d0ad30d5 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -179,10 +179,9 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  	 * after this call is handled and before the VCPUs have been
>  	 * re-initialized.
>  	 */
> -	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> +	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
>  		tmp->arch.power_off = true;
> -		kvm_vcpu_kick(tmp);
> -	}
> +	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_VCPU_EXIT);
>  
>  	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
>  	vcpu->run->system_event.type = type;
> -- 
> 2.9.3
> 

Re: [PATCH v4 07/11] KVM: arm/arm64: optimize VCPU RUN

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:31AM +0200, Andrew Jones wrote:
> We can make a small optimization by not checking the state of
> the power_off field on each run. This is done by treating
> power_off like pause, only checking it when we get the EXIT
> VCPU request. When a VCPU powers off another VCPU the EXIT
> request is already made, so we just need to make sure the
> request is also made on self power off. kvm_vcpu_kick() isn't
> necessary for these cases, as the VCPU would just be kicking
> itself, but we add it anyway as a self kick doesn't cost much,
> and it makes the code more future-proof.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  virt/kvm/arm/arm.c  | 19 +++++++++++--------
>  virt/kvm/arm/psci.c |  2 ++
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 21a4db90073f..9379b1d75ad3 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -368,6 +368,13 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	kvm_timer_vcpu_put(vcpu);
>  }
>  
> +static void vcpu_power_off(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.power_off = true;
> +	kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
> +	kvm_vcpu_kick(vcpu);
> +}
> +
>  int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
>  				    struct kvm_mp_state *mp_state)
>  {
> @@ -387,7 +394,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  		vcpu->arch.power_off = false;
>  		break;
>  	case KVM_MP_STATE_STOPPED:
> -		vcpu->arch.power_off = true;
> +		vcpu_power_off(vcpu);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -557,7 +564,7 @@ static void vcpu_sleep(struct kvm_vcpu *vcpu)
>  	swait_event_interruptible(*wq, ((!vcpu->arch.power_off) &&
>  				       (!vcpu->arch.pause)));
>  
> -	if (vcpu->arch.pause) {
> +	if (vcpu->arch.power_off || vcpu->arch.pause) {
>  		/* Awaken to handle a signal, request we sleep again later. */
>  		kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
>  	}
> @@ -623,9 +630,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		check_vcpu_requests(vcpu);
>  
> -		if (vcpu->arch.power_off)
> -			vcpu_sleep(vcpu);
> -
>  		/*
>  		 * Preparing the interrupts to be injected also
>  		 * involves poking the GIC, which must be done in a
> @@ -662,8 +666,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
>  
>  		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> -		    kvm_request_pending(vcpu) ||
> -		    vcpu->arch.power_off) {
> +		    kvm_request_pending(vcpu)) {
>  			vcpu->mode = OUTSIDE_GUEST_MODE;
>  			local_irq_enable();
>  			kvm_pmu_sync_hwstate(vcpu);
> @@ -896,7 +899,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	 * Handle the "start in power-off" case.
>  	 */
>  	if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features))
> -		vcpu->arch.power_off = true;
> +		vcpu_power_off(vcpu);
>  	else
>  		vcpu->arch.power_off = false;
>  
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index f189d0ad30d5..4a436685c552 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -65,6 +65,8 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.power_off = true;
> +	kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
> +	kvm_vcpu_kick(vcpu);
>  }
>  
>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> -- 
> 2.9.3
> 

Re: [PATCH v4 08/11] KVM: arm/arm64: change exit request to sleep request

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:32AM +0200, Andrew Jones wrote:
> A request called EXIT is too generic. All requests are meant to cause
> exits, but different requests have different flags. Let's not make
> it difficult to decide if the EXIT request is correct for some case
> by just always providing unique requests for each case. This patch
> changes EXIT to SLEEP, because that's what the request is asking the
> VCPU to do.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Acked-by: Christoffer Dall <cdall@linaro.org>

> ---
>  arch/arm/include/asm/kvm_host.h   |  2 +-
>  arch/arm64/include/asm/kvm_host.h |  2 +-
>  virt/kvm/arm/arm.c                | 12 ++++++------
>  virt/kvm/arm/psci.c               |  4 ++--
>  4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index c556babe467c..fdd644c01c89 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 \
> +#define KVM_REQ_SLEEP \
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  
>  u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 0ff991c9c66e..9bd0d1040de9 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 \
> +#define KVM_REQ_SLEEP \
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  
>  int __attribute_const__ kvm_target_cpu(void);
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 9379b1d75ad3..ddc833987dfb 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -371,7 +371,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  static void vcpu_power_off(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.power_off = true;
> -	kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
> +	kvm_make_request(KVM_REQ_SLEEP, vcpu);
>  	kvm_vcpu_kick(vcpu);
>  }
>  
> @@ -543,7 +543,7 @@ void kvm_arm_halt_guest(struct kvm *kvm)
>  
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		vcpu->arch.pause = true;
> -	kvm_make_all_cpus_request(kvm, KVM_REQ_VCPU_EXIT);
> +	kvm_make_all_cpus_request(kvm, KVM_REQ_SLEEP);
>  }
>  
>  void kvm_arm_resume_guest(struct kvm *kvm)
> @@ -557,7 +557,7 @@ void kvm_arm_resume_guest(struct kvm *kvm)
>  	}
>  }
>  
> -static void vcpu_sleep(struct kvm_vcpu *vcpu)
> +static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>  {
>  	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
>  
> @@ -566,7 +566,7 @@ static void vcpu_sleep(struct kvm_vcpu *vcpu)
>  
>  	if (vcpu->arch.power_off || vcpu->arch.pause) {
>  		/* Awaken to handle a signal, request we sleep again later. */
> -		kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
> +		kvm_make_request(KVM_REQ_SLEEP, vcpu);
>  	}
>  }
>  
> @@ -578,8 +578,8 @@ static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>  static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>  {
>  	if (kvm_request_pending(vcpu)) {
> -		if (kvm_check_request(KVM_REQ_VCPU_EXIT, vcpu))
> -			vcpu_sleep(vcpu);
> +		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> +			vcpu_req_sleep(vcpu);
>  	}
>  }
>  
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 4a436685c552..f1e363bab5e8 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -65,7 +65,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)
>  static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.power_off = true;
> -	kvm_make_request(KVM_REQ_VCPU_EXIT, vcpu);
> +	kvm_make_request(KVM_REQ_SLEEP, vcpu);
>  	kvm_vcpu_kick(vcpu);
>  }
>  
> @@ -183,7 +183,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)
>  	 */
>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
>  		tmp->arch.power_off = true;
> -	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_VCPU_EXIT);
> +	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);
>  
>  	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
>  	vcpu->run->system_event.type = type;
> -- 
> 2.9.3
> 

Re: [PATCH v4 09/11] KVM: arm/arm64: use vcpu requests for irq injection

[Christoffer Dall]

On Tue, May 16, 2017 at 04:20:33AM +0200, Andrew Jones wrote:
> Don't use request-less VCPU kicks when injecting IRQs, as a VCPU
> kick meant to trigger the interrupt injection could be sent while
> the VCPU is outside guest mode, which means no IPI is sent, and
> after it has called kvm_vgic_flush_hwstate(), meaning it won't see
> the updated GIC state until its next exit some time later for some
> other reason.  The receiving VCPU only needs to check this request
> in VCPU RUN to handle it.  By checking it, if it's pending, a
> memory barrier will be issued that ensures all state is visible.
> We still create a vcpu_req_irq_pending() function (which is a nop),
> though, in order to allow us to use the standard request checking
> pattern.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm/include/asm/kvm_host.h   |  1 +
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  virt/kvm/arm/arm.c                | 12 ++++++++++++
>  virt/kvm/arm/vgic/vgic.c          |  9 +++++++--
>  4 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index fdd644c01c89..00ad56ee6455 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -46,6 +46,7 @@
>  
>  #define KVM_REQ_SLEEP \
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
>  
>  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 9bd0d1040de9..0c4fd1f46e10 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -43,6 +43,7 @@
>  
>  #define KVM_REQ_SLEEP \
>  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
>  
>  int __attribute_const__ kvm_target_cpu(void);
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index ddc833987dfb..73a75ca91e41 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -570,6 +570,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>  	}
>  }
>  
> +static void vcpu_req_irq_pending(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * Nothing to do here. kvm_check_request() already issued a memory
> +	 * barrier that pairs with kvm_make_request(), so all hardware state
> +	 * we need to flush should now be visible.
> +	 */

I don't understand this comment :(

And I don't much like this empty function either.


> +}
> +
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->arch.target >= 0;
> @@ -580,6 +589,8 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>  	if (kvm_request_pending(vcpu)) {
>  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
>  			vcpu_req_sleep(vcpu);
> +		if (kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu))
> +			vcpu_req_irq_pending(vcpu);


Can we just do:
		/* 
		 * Clear IRQ_PENDING requests that were made to
		 * guarantee that a VCPU sees new virtual interrupts.
		 */
		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);

?

>  	}
>  }
>  
> @@ -771,6 +782,7 @@ static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
>  	 * trigger a world-switch round on the running physical CPU to set the
>  	 * virtual IRQ/FIQ fields in the HCR appropriately.
>  	 */
> +	kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
>  	kvm_vcpu_kick(vcpu);
>  
>  	return 0;
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index aea080a2c443..c66feaca2a5d 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -286,8 +286,10 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
>  		 * won't see this one until it exits for some other
>  		 * reason.
>  		 */
> -		if (vcpu)
> +		if (vcpu) {
> +			kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
>  			kvm_vcpu_kick(vcpu);
> +		}
>  		return false;
>  	}
>  
> @@ -333,6 +335,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
>  	spin_unlock(&irq->irq_lock);
>  	spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
>  
> +	kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
>  	kvm_vcpu_kick(vcpu);
>  
>  	return true;
> @@ -722,8 +725,10 @@ void vgic_kick_vcpus(struct kvm *kvm)
>  	 * a good kick...
>  	 */
>  	kvm_for_each_vcpu(c, vcpu, kvm) {
> -		if (kvm_vgic_vcpu_pending_irq(vcpu))
> +		if (kvm_vgic_vcpu_pending_irq(vcpu)) {
> +			kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
>  			kvm_vcpu_kick(vcpu);
> +		}
>  	}
>  }
>  
> -- 
> 2.9.3
> 
Otherwise:

Reviewed-by: Christoffer Dall <cdall@linaro.org>

Re: [PATCH v4 09/11] KVM: arm/arm64: use vcpu requests for irq injection

[Andrew Jones]

On Thu, Jun 01, 2017 at 12:35:33PM +0200, Christoffer Dall wrote:
> On Tue, May 16, 2017 at 04:20:33AM +0200, Andrew Jones wrote:
> > Don't use request-less VCPU kicks when injecting IRQs, as a VCPU
> > kick meant to trigger the interrupt injection could be sent while
> > the VCPU is outside guest mode, which means no IPI is sent, and
> > after it has called kvm_vgic_flush_hwstate(), meaning it won't see
> > the updated GIC state until its next exit some time later for some
> > other reason.  The receiving VCPU only needs to check this request
> > in VCPU RUN to handle it.  By checking it, if it's pending, a
> > memory barrier will be issued that ensures all state is visible.
> > We still create a vcpu_req_irq_pending() function (which is a nop),
> > though, in order to allow us to use the standard request checking
> > pattern.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arch/arm/include/asm/kvm_host.h   |  1 +
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  virt/kvm/arm/arm.c                | 12 ++++++++++++
> >  virt/kvm/arm/vgic/vgic.c          |  9 +++++++--
> >  4 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > index fdd644c01c89..00ad56ee6455 100644
> > --- a/arch/arm/include/asm/kvm_host.h
> > +++ b/arch/arm/include/asm/kvm_host.h
> > @@ -46,6 +46,7 @@
> >  
> >  #define KVM_REQ_SLEEP \
> >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> >  
> >  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 9bd0d1040de9..0c4fd1f46e10 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -43,6 +43,7 @@
> >  
> >  #define KVM_REQ_SLEEP \
> >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> >  
> >  int __attribute_const__ kvm_target_cpu(void);
> >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index ddc833987dfb..73a75ca91e41 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -570,6 +570,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> >  	}
> >  }
> >  
> > +static void vcpu_req_irq_pending(struct kvm_vcpu *vcpu)
> > +{
> > +	/*
> > +	 * Nothing to do here. kvm_check_request() already issued a memory
> > +	 * barrier that pairs with kvm_make_request(), so all hardware state
> > +	 * we need to flush should now be visible.
> > +	 */
> 
> I don't understand this comment :(

We need a kvm_check_request() to pair with a requesting VCPU's setting
of virtual irq state and call of kvm_make_request(). The requester's
kvm_make_request() ensures the target VCPU executes VCPU RUN, and the
barriers wrapped in kvm_check/make_request() ensure that when VCPU RUN
calls kvm_vgic_flush_hwstate() that the virtual irq state set by the
requester is visible to the target VCPU.

But you knew all that already :-) So, maybe I just need to replace
'all hardware state we need to flush should now be visible.' with
'the virtual irq state is now visible.'?

> 
> And I don't much like this empty function either.
> 
> 
> > +}
> > +
> >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> >  {
> >  	return vcpu->arch.target >= 0;
> > @@ -580,6 +589,8 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> >  	if (kvm_request_pending(vcpu)) {
> >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> >  			vcpu_req_sleep(vcpu);
> > +		if (kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu))
> > +			vcpu_req_irq_pending(vcpu);
> 
> 
> Can we just do:
> 		/* 
> 		 * Clear IRQ_PENDING requests that were made to
> 		 * guarantee that a VCPU sees new virtual interrupts.
> 		 */
> 		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
> 
> ?

I won't insist, but I prefer the empty function to breaking the pattern
of this if-sequence, and also to the calling of a function named "check"
without considering its return value.

> 
> >  	}
> >  }
> >  
> > @@ -771,6 +782,7 @@ static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
> >  	 * trigger a world-switch round on the running physical CPU to set the
> >  	 * virtual IRQ/FIQ fields in the HCR appropriately.
> >  	 */
> > +	kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> >  	kvm_vcpu_kick(vcpu);
> >  
> >  	return 0;
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index aea080a2c443..c66feaca2a5d 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -286,8 +286,10 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
> >  		 * won't see this one until it exits for some other
> >  		 * reason.
> >  		 */
> > -		if (vcpu)
> > +		if (vcpu) {
> > +			kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> >  			kvm_vcpu_kick(vcpu);
> > +		}
> >  		return false;
> >  	}
> >  
> > @@ -333,6 +335,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq)
> >  	spin_unlock(&irq->irq_lock);
> >  	spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> >  
> > +	kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> >  	kvm_vcpu_kick(vcpu);
> >  
> >  	return true;
> > @@ -722,8 +725,10 @@ void vgic_kick_vcpus(struct kvm *kvm)
> >  	 * a good kick...
> >  	 */
> >  	kvm_for_each_vcpu(c, vcpu, kvm) {
> > -		if (kvm_vgic_vcpu_pending_irq(vcpu))
> > +		if (kvm_vgic_vcpu_pending_irq(vcpu)) {
> > +			kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> >  			kvm_vcpu_kick(vcpu);
> > +		}
> >  	}
> >  }
> >  
> > -- 
> > 2.9.3
> > 
> Otherwise:
> 
> Reviewed-by: Christoffer Dall <cdall@linaro.org>

Thanks,
drew

Re: [PATCH v4 11/11] KVM: arm/arm64: timer: remove request-less vcpu kick

[Andrew Jones]

On Thu, Jun 01, 2017 at 12:34:54PM +0200, Christoffer Dall wrote:
> On Tue, May 16, 2017 at 04:20:35AM +0200, Andrew Jones wrote:
> > The timer work is only scheduled for a VCPU when that VCPU is
> > blocked. This means we only need to wake it up, not kick (IPI)
> > it. While calling kvm_vcpu_kick() would just do the wake up,
> > and not kick, anyway, let's change this to avoid request-less
> > vcpu kicks, as they're generally not a good idea (see
> > "Request-less VCPU Kicks" in
> > Documentation/virtual/kvm/vcpu-requests.rst)
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  virt/kvm/arm/arch_timer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 5976609ef27c..c9cd56f39b1e 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -95,7 +95,7 @@ static void kvm_timer_inject_irq_work(struct work_struct *work)
> >  	 * If the vcpu is blocked we want to wake it up so that it will see
> >  	 * the timer has expired when entering the guest.
> >  	 */
> > -	kvm_vcpu_kick(vcpu);
> > +	swake_up(kvm_arch_vcpu_wq(vcpu));
> 
> We have kvm_vcpu_wake_up().  Why not use that?

The are two differences between swake_up(kvm_arch_vcpu_wq(vcpu)) and
kvm_vcpu_wake_up(vcpu)
 1. kvm_vcpu_wake_up() has a return value: true on wake up, else false
 2. kvm_vcpu_wake_up() increments the halt_wakeup stat when the vcpu
    is awaken

(1) doesn't really matter, but (2) might. Hmm, I think we do want to
increment that stat in this case though, so I should change this.

Also, we have another use of swake_up(kvm_arch_vcpu_wq(vcpu)), in
kvm_arm_resume_guest(), but there I don't think we want to increment
the halt stat, so that one is probably OK.

Thanks,
drew

Re: [PATCH v4 11/11] KVM: arm/arm64: timer: remove request-less vcpu kick

[Paolo Bonzini]

On 01/06/2017 13:09, Andrew Jones wrote:
>>> -	kvm_vcpu_kick(vcpu);
>>> +	swake_up(kvm_arch_vcpu_wq(vcpu));
>> We have kvm_vcpu_wake_up().  Why not use that?
> The are two differences between swake_up(kvm_arch_vcpu_wq(vcpu)) and
> kvm_vcpu_wake_up(vcpu)
>  1. kvm_vcpu_wake_up() has a return value: true on wake up, else false
>  2. kvm_vcpu_wake_up() increments the halt_wakeup stat when the vcpu
>     is awaken
> 
> (1) doesn't really matter, but (2) might. Hmm, I think we do want to
> increment that stat in this case though, so I should change this.

Yep.

> Also, we have another use of swake_up(kvm_arch_vcpu_wq(vcpu)), in
> kvm_arm_resume_guest(), but there I don't think we want to increment
> the halt stat, so that one is probably OK.

I would define a __kvm_vcpu_wake_up if you don't want the stat.

Paolo

Re: [PATCH v4 11/11] KVM: arm/arm64: timer: remove request-less vcpu kick

[Christoffer Dall]

On Thu, Jun 01, 2017 at 02:37:52PM +0200, Paolo Bonzini wrote:
> 
> 
> On 01/06/2017 13:09, Andrew Jones wrote:
> >>> -	kvm_vcpu_kick(vcpu);
> >>> +	swake_up(kvm_arch_vcpu_wq(vcpu));
> >> We have kvm_vcpu_wake_up().  Why not use that?
> > The are two differences between swake_up(kvm_arch_vcpu_wq(vcpu)) and
> > kvm_vcpu_wake_up(vcpu)
> >  1. kvm_vcpu_wake_up() has a return value: true on wake up, else false
> >  2. kvm_vcpu_wake_up() increments the halt_wakeup stat when the vcpu
> >     is awaken
> > 
> > (1) doesn't really matter, but (2) might. Hmm, I think we do want to
> > increment that stat in this case though, so I should change this.
> 
> Yep.
> 
> > Also, we have another use of swake_up(kvm_arch_vcpu_wq(vcpu)), in
> > kvm_arm_resume_guest(), but there I don't think we want to increment
> > the halt stat, so that one is probably OK.
> 
> I would define a __kvm_vcpu_wake_up if you don't want the stat.
> 
I didn't mind the other one, because it is in a function in the code
right above the sleep function, which uses swait_event_interruptible
directly without calling kvm_vcpu_block, so the symmetry is clear there,
but not in the timer case.

-Christoffer

Re: [PATCH v4 09/11] KVM: arm/arm64: use vcpu requests for irq injection

[Christoffer Dall]

On Thu, Jun 01, 2017 at 12:59:21PM +0200, Andrew Jones wrote:
> On Thu, Jun 01, 2017 at 12:35:33PM +0200, Christoffer Dall wrote:
> > On Tue, May 16, 2017 at 04:20:33AM +0200, Andrew Jones wrote:
> > > Don't use request-less VCPU kicks when injecting IRQs, as a VCPU
> > > kick meant to trigger the interrupt injection could be sent while
> > > the VCPU is outside guest mode, which means no IPI is sent, and
> > > after it has called kvm_vgic_flush_hwstate(), meaning it won't see
> > > the updated GIC state until its next exit some time later for some
> > > other reason.  The receiving VCPU only needs to check this request
> > > in VCPU RUN to handle it.  By checking it, if it's pending, a
> > > memory barrier will be issued that ensures all state is visible.
> > > We still create a vcpu_req_irq_pending() function (which is a nop),
> > > though, in order to allow us to use the standard request checking
> > > pattern.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  arch/arm/include/asm/kvm_host.h   |  1 +
> > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > >  virt/kvm/arm/arm.c                | 12 ++++++++++++
> > >  virt/kvm/arm/vgic/vgic.c          |  9 +++++++--
> > >  4 files changed, 21 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > index fdd644c01c89..00ad56ee6455 100644
> > > --- a/arch/arm/include/asm/kvm_host.h
> > > +++ b/arch/arm/include/asm/kvm_host.h
> > > @@ -46,6 +46,7 @@
> > >  
> > >  #define KVM_REQ_SLEEP \
> > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > >  
> > >  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 9bd0d1040de9..0c4fd1f46e10 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -43,6 +43,7 @@
> > >  
> > >  #define KVM_REQ_SLEEP \
> > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > >  
> > >  int __attribute_const__ kvm_target_cpu(void);
> > >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > index ddc833987dfb..73a75ca91e41 100644
> > > --- a/virt/kvm/arm/arm.c
> > > +++ b/virt/kvm/arm/arm.c
> > > @@ -570,6 +570,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > >  	}
> > >  }
> > >  
> > > +static void vcpu_req_irq_pending(struct kvm_vcpu *vcpu)
> > > +{
> > > +	/*
> > > +	 * Nothing to do here. kvm_check_request() already issued a memory
> > > +	 * barrier that pairs with kvm_make_request(), so all hardware state
> > > +	 * we need to flush should now be visible.
> > > +	 */
> > 
> > I don't understand this comment :(
> 
> We need a kvm_check_request() to pair with a requesting VCPU's setting
> of virtual irq state and call of kvm_make_request(). The requester's
> kvm_make_request() ensures the target VCPU executes VCPU RUN, and the
> barriers wrapped in kvm_check/make_request() ensure that when VCPU RUN
> calls kvm_vgic_flush_hwstate() that the virtual irq state set by the
> requester is visible to the target VCPU.
> 
> But you knew all that already :-) So, maybe I just need to replace
> 'all hardware state we need to flush should now be visible.' with
> 'the virtual irq state is now visible.'?
> 

Yes, the hardware state is definitely vague here, but I also feel like
you're trying to repeat parts of the documentation here, and it's better
to just refer to the complete documentation for the how the barreirs
etc. work, and just explain why you don't need to do anything, yet still
have a vcpu request.

Which is why I suggested the alternative comment below instead ;)

> > 
> > And I don't much like this empty function either.
> > 
> > 
> > > +}
> > > +
> > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > >  {
> > >  	return vcpu->arch.target >= 0;
> > > @@ -580,6 +589,8 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > >  	if (kvm_request_pending(vcpu)) {
> > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > >  			vcpu_req_sleep(vcpu);
> > > +		if (kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu))
> > > +			vcpu_req_irq_pending(vcpu);
> > 
> > 
> > Can we just do:
> > 		/* 
> > 		 * Clear IRQ_PENDING requests that were made to
> > 		 * guarantee that a VCPU sees new virtual interrupts.
> > 		 */
> > 		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
> > 
> > ?
> 
> I won't insist, but I prefer the empty function to breaking the pattern
> of this if-sequence, and also to the calling of a function named "check"
> without considering its return value.

I like scratching our OCD as much as the next reviewer, but I think my
version more clearly expresses the purpose, which is more important than
having multiple identically-looking statements.

Thanks,
-Christoffer

Re: [PATCH v4 09/11] KVM: arm/arm64: use vcpu requests for irq injection

[Andrew Jones]

On Thu, Jun 01, 2017 at 03:27:16PM +0200, Christoffer Dall wrote:
> On Thu, Jun 01, 2017 at 12:59:21PM +0200, Andrew Jones wrote:
> > On Thu, Jun 01, 2017 at 12:35:33PM +0200, Christoffer Dall wrote:
> > > On Tue, May 16, 2017 at 04:20:33AM +0200, Andrew Jones wrote:
> > > > Don't use request-less VCPU kicks when injecting IRQs, as a VCPU
> > > > kick meant to trigger the interrupt injection could be sent while
> > > > the VCPU is outside guest mode, which means no IPI is sent, and
> > > > after it has called kvm_vgic_flush_hwstate(), meaning it won't see
> > > > the updated GIC state until its next exit some time later for some
> > > > other reason.  The receiving VCPU only needs to check this request
> > > > in VCPU RUN to handle it.  By checking it, if it's pending, a
> > > > memory barrier will be issued that ensures all state is visible.
> > > > We still create a vcpu_req_irq_pending() function (which is a nop),
> > > > though, in order to allow us to use the standard request checking
> > > > pattern.
> > > > 
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > >  arch/arm/include/asm/kvm_host.h   |  1 +
> > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > >  virt/kvm/arm/arm.c                | 12 ++++++++++++
> > > >  virt/kvm/arm/vgic/vgic.c          |  9 +++++++--
> > > >  4 files changed, 21 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > > index fdd644c01c89..00ad56ee6455 100644
> > > > --- a/arch/arm/include/asm/kvm_host.h
> > > > +++ b/arch/arm/include/asm/kvm_host.h
> > > > @@ -46,6 +46,7 @@
> > > >  
> > > >  #define KVM_REQ_SLEEP \
> > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > >  
> > > >  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 9bd0d1040de9..0c4fd1f46e10 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -43,6 +43,7 @@
> > > >  
> > > >  #define KVM_REQ_SLEEP \
> > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > >  
> > > >  int __attribute_const__ kvm_target_cpu(void);
> > > >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > index ddc833987dfb..73a75ca91e41 100644
> > > > --- a/virt/kvm/arm/arm.c
> > > > +++ b/virt/kvm/arm/arm.c
> > > > @@ -570,6 +570,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > > >  	}
> > > >  }
> > > >  
> > > > +static void vcpu_req_irq_pending(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +	/*
> > > > +	 * Nothing to do here. kvm_check_request() already issued a memory
> > > > +	 * barrier that pairs with kvm_make_request(), so all hardware state
> > > > +	 * we need to flush should now be visible.
> > > > +	 */
> > > 
> > > I don't understand this comment :(
> > 
> > We need a kvm_check_request() to pair with a requesting VCPU's setting
> > of virtual irq state and call of kvm_make_request(). The requester's
> > kvm_make_request() ensures the target VCPU executes VCPU RUN, and the
> > barriers wrapped in kvm_check/make_request() ensure that when VCPU RUN
> > calls kvm_vgic_flush_hwstate() that the virtual irq state set by the
> > requester is visible to the target VCPU.
> > 
> > But you knew all that already :-) So, maybe I just need to replace
> > 'all hardware state we need to flush should now be visible.' with
> > 'the virtual irq state is now visible.'?
> > 
> 
> Yes, the hardware state is definitely vague here, but I also feel like
> you're trying to repeat parts of the documentation here, and it's better
> to just refer to the complete documentation for the how the barreirs
> etc. work, and just explain why you don't need to do anything, yet still
> have a vcpu request.
> 
> Which is why I suggested the alternative comment below instead ;)
> 
> > > 
> > > And I don't much like this empty function either.
> > > 
> > > 
> > > > +}
> > > > +
> > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > >  {
> > > >  	return vcpu->arch.target >= 0;
> > > > @@ -580,6 +589,8 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > >  	if (kvm_request_pending(vcpu)) {
> > > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > >  			vcpu_req_sleep(vcpu);
> > > > +		if (kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu))
> > > > +			vcpu_req_irq_pending(vcpu);
> > > 
> > > 
> > > Can we just do:
> > > 		/* 
> > > 		 * Clear IRQ_PENDING requests that were made to
> > > 		 * guarantee that a VCPU sees new virtual interrupts.
> > > 		 */
> > > 		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
> > > 
> > > ?
> > 
> > I won't insist, but I prefer the empty function to breaking the pattern
> > of this if-sequence, and also to the calling of a function named "check"
> > without considering its return value.
> 
> I like scratching our OCD as much as the next reviewer, but I think my
> version more clearly expresses the purpose, which is more important than
> having multiple identically-looking statements.
> 
> Thanks,
> -Christoffer

I'll take your suggestion, and hopefully my OCD won't keep me up at night.
I'll get v5 tested and sent tomorrow.

Thanks,
drew

Re: [PATCH v4 09/11] KVM: arm/arm64: use vcpu requests for irq injection

[Christoffer Dall]

On Thu, Jun 01, 2017 at 03:38:43PM +0200, Andrew Jones wrote:
> On Thu, Jun 01, 2017 at 03:27:16PM +0200, Christoffer Dall wrote:
> > On Thu, Jun 01, 2017 at 12:59:21PM +0200, Andrew Jones wrote:
> > > On Thu, Jun 01, 2017 at 12:35:33PM +0200, Christoffer Dall wrote:
> > > > On Tue, May 16, 2017 at 04:20:33AM +0200, Andrew Jones wrote:
> > > > > Don't use request-less VCPU kicks when injecting IRQs, as a VCPU
> > > > > kick meant to trigger the interrupt injection could be sent while
> > > > > the VCPU is outside guest mode, which means no IPI is sent, and
> > > > > after it has called kvm_vgic_flush_hwstate(), meaning it won't see
> > > > > the updated GIC state until its next exit some time later for some
> > > > > other reason.  The receiving VCPU only needs to check this request
> > > > > in VCPU RUN to handle it.  By checking it, if it's pending, a
> > > > > memory barrier will be issued that ensures all state is visible.
> > > > > We still create a vcpu_req_irq_pending() function (which is a nop),
> > > > > though, in order to allow us to use the standard request checking
> > > > > pattern.
> > > > > 
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > ---
> > > > >  arch/arm/include/asm/kvm_host.h   |  1 +
> > > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > > >  virt/kvm/arm/arm.c                | 12 ++++++++++++
> > > > >  virt/kvm/arm/vgic/vgic.c          |  9 +++++++--
> > > > >  4 files changed, 21 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > > > index fdd644c01c89..00ad56ee6455 100644
> > > > > --- a/arch/arm/include/asm/kvm_host.h
> > > > > +++ b/arch/arm/include/asm/kvm_host.h
> > > > > @@ -46,6 +46,7 @@
> > > > >  
> > > > >  #define KVM_REQ_SLEEP \
> > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > >  
> > > > >  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 9bd0d1040de9..0c4fd1f46e10 100644
> > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > @@ -43,6 +43,7 @@
> > > > >  
> > > > >  #define KVM_REQ_SLEEP \
> > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > >  
> > > > >  int __attribute_const__ kvm_target_cpu(void);
> > > > >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > > index ddc833987dfb..73a75ca91e41 100644
> > > > > --- a/virt/kvm/arm/arm.c
> > > > > +++ b/virt/kvm/arm/arm.c
> > > > > @@ -570,6 +570,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static void vcpu_req_irq_pending(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > +	/*
> > > > > +	 * Nothing to do here. kvm_check_request() already issued a memory
> > > > > +	 * barrier that pairs with kvm_make_request(), so all hardware state
> > > > > +	 * we need to flush should now be visible.
> > > > > +	 */
> > > > 
> > > > I don't understand this comment :(
> > > 
> > > We need a kvm_check_request() to pair with a requesting VCPU's setting
> > > of virtual irq state and call of kvm_make_request(). The requester's
> > > kvm_make_request() ensures the target VCPU executes VCPU RUN, and the
> > > barriers wrapped in kvm_check/make_request() ensure that when VCPU RUN
> > > calls kvm_vgic_flush_hwstate() that the virtual irq state set by the
> > > requester is visible to the target VCPU.
> > > 
> > > But you knew all that already :-) So, maybe I just need to replace
> > > 'all hardware state we need to flush should now be visible.' with
> > > 'the virtual irq state is now visible.'?
> > > 
> > 
> > Yes, the hardware state is definitely vague here, but I also feel like
> > you're trying to repeat parts of the documentation here, and it's better
> > to just refer to the complete documentation for the how the barreirs
> > etc. work, and just explain why you don't need to do anything, yet still
> > have a vcpu request.
> > 
> > Which is why I suggested the alternative comment below instead ;)
> > 
> > > > 
> > > > And I don't much like this empty function either.
> > > > 
> > > > 
> > > > > +}
> > > > > +
> > > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > > >  {
> > > > >  	return vcpu->arch.target >= 0;
> > > > > @@ -580,6 +589,8 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > > >  	if (kvm_request_pending(vcpu)) {
> > > > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > > >  			vcpu_req_sleep(vcpu);
> > > > > +		if (kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu))
> > > > > +			vcpu_req_irq_pending(vcpu);
> > > > 
> > > > 
> > > > Can we just do:
> > > > 		/* 
> > > > 		 * Clear IRQ_PENDING requests that were made to
> > > > 		 * guarantee that a VCPU sees new virtual interrupts.
> > > > 		 */
> > > > 		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
> > > > 
> > > > ?
> > > 
> > > I won't insist, but I prefer the empty function to breaking the pattern
> > > of this if-sequence, and also to the calling of a function named "check"
> > > without considering its return value.
> > 
> > I like scratching our OCD as much as the next reviewer, but I think my
> > version more clearly expresses the purpose, which is more important than
> > having multiple identically-looking statements.
> > 
> > Thanks,
> > -Christoffer
> 
> I'll take your suggestion, and hopefully my OCD won't keep me up at night.
> I'll get v5 tested and sent tomorrow.
> 
Thanks Drew!
-Christoffer