LinuxLists.cc - [PATCH v2 0/9] KVM: towards maintainable kvm_make_all_cpus

[permalink] [raw]

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

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

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

Reviewed-by: Christian Borntraeger <[email protected]>
Signed-off-by: Radim Krčmář <[email protected]>
---
arch/mips/kvm/emulate.c | 2 +-
arch/powerpc/kvm/book3s_pr.c | 2 +-
arch/powerpc/kvm/book3s_pr_papr.c | 2 +-
arch/powerpc/kvm/booke.c | 4 ++--
arch/powerpc/kvm/powerpc.c | 2 +-
arch/s390/kvm/kvm-s390.c | 2 +-
arch/x86/kvm/vmx.c | 2 +-
arch/x86/kvm/x86.c | 8 ++++----
include/linux/kvm_host.h | 14 ++++++++++++--
9 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c
index 34e78a3ee9d7..4144bfaef137 100644
--- a/arch/mips/kvm/emulate.c
+++ b/arch/mips/kvm/emulate.c
@@ -982,7 +982,7 @@ enum emulation_result kvm_mips_emul_wait(struct kvm_vcpu *vcpu)
* check if any I/O interrupts are pending.
*/
if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) {
- clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_UNHALT, vcpu);
vcpu->run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;
}
}
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index f026b062c0ed..69a09444d46e 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -349,7 +349,7 @@ static void kvmppc_set_msr_pr(struct kvm_vcpu *vcpu, u64 msr)
if (msr & MSR_POW) {
if (!vcpu->arch.pending_exceptions) {
kvm_vcpu_block(vcpu);
- clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_UNHALT, vcpu);
vcpu->stat.halt_wakeup++;

/* Unset POW bit after we woke up */
diff --git a/arch/powerpc/kvm/book3s_pr_papr.c b/arch/powerpc/kvm/book3s_pr_papr.c
index f102616febc7..bcbeeb62dd13 100644
--- a/arch/powerpc/kvm/book3s_pr_papr.c
+++ b/arch/powerpc/kvm/book3s_pr_papr.c
@@ -344,7 +344,7 @@ int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd)
case H_CEDE:
kvmppc_set_msr_fast(vcpu, kvmppc_get_msr(vcpu) | MSR_EE);
kvm_vcpu_block(vcpu);
- clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_UNHALT, vcpu);
vcpu->stat.halt_wakeup++;
return EMULATE_DONE;
case H_LOGICAL_CI_LOAD:
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 3c296c2eacf8..3eaac3809977 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -584,7 +584,7 @@ static void arm_next_watchdog(struct kvm_vcpu *vcpu)
* userspace, so clear the KVM_REQ_WATCHDOG request.
*/
if ((vcpu->arch.tsr & (TSR_ENW | TSR_WIS)) != (TSR_ENW | TSR_WIS))
- clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_WATCHDOG, vcpu);

spin_lock_irqsave(&vcpu->arch.wdt_lock, flags);
nr_jiffies = watchdog_next_timeout(vcpu);
@@ -695,7 +695,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
if (vcpu->arch.shared->msr & MSR_WE) {
local_irq_enable();
kvm_vcpu_block(vcpu);
- clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_UNHALT, vcpu);
hard_irq_disable();

kvmppc_set_exit_type(vcpu, EMULATED_MTMSRWE_EXITS);
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index cf725c580fc5..1ee22a910074 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -233,7 +233,7 @@ int kvmppc_kvm_pv(struct kvm_vcpu *vcpu)
case EV_HCALL_TOKEN(EV_IDLE):
r = EV_SUCCESS;
kvm_vcpu_block(vcpu);
- clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_UNHALT, vcpu);
break;
default:
r = EV_UNIMPLEMENTED;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 8771fef112a1..71a5f4023f3f 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2492,7 +2492,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
}

/* nothing to do, just clear the request */
- clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_UNHALT, vcpu);

return 0;
}
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a4ef63718101..b003b8dfb20c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6299,7 +6299,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
if (intr_window_requested && vmx_interrupt_allowed(vcpu))
return handle_interrupt_window(&vmx->vcpu);

- if (test_bit(KVM_REQ_EVENT, &vcpu->requests))
+ if (kvm_test_request(KVM_REQ_EVENT, vcpu))
return 1;

err = emulate_instruction(vcpu, EMULTYPE_NO_REEXECUTE);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f68c5b2ba627..2de54c20fa9e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1753,7 +1753,7 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)

/* guest entries allowed */
kvm_for_each_vcpu(i, vcpu, kvm)
- clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);

spin_unlock(&ka->pvclock_gtod_sync_lock);
#endif
@@ -7041,7 +7041,7 @@ static int vcpu_run(struct kvm_vcpu *vcpu)
if (r <= 0)
break;

- clear_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_PENDING_TIMER, vcpu);
if (kvm_cpu_has_pending_timer(vcpu))
kvm_inject_pending_timer_irqs(vcpu);

@@ -7169,7 +7169,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
kvm_vcpu_block(vcpu);
kvm_apic_accept_events(vcpu);
- clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+ kvm_clear_request(KVM_REQ_UNHALT, vcpu);
r = -EAGAIN;
goto out;
}
@@ -8382,7 +8382,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
if (atomic_read(&vcpu->arch.nmi_queued))
return true;

- if (test_bit(KVM_REQ_SMI, &vcpu->requests))
+ if (kvm_test_request(KVM_REQ_SMI, vcpu))
return true;

if (kvm_arch_interrupt_allowed(vcpu) &&
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 397b7b5b1933..374fa92c7657 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1079,10 +1079,20 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
set_bit(req, &vcpu->requests);
}

+static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
+{
+ return test_bit(req, &vcpu->requests);
+}
+
+static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
+{
+ clear_bit(req, &vcpu->requests);
+}
+
static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
{
- if (test_bit(req, &vcpu->requests)) {
- clear_bit(req, &vcpu->requests);
+ if (kvm_test_request(req, vcpu)) {
+ kvm_clear_request(req, vcpu);

/*
* Ensure the rest of the request is visible to kvm_check_request's
--
2.12.2

2017-04-26 20:33:32

[permalink] [raw]

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

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

First 8 bits were arbitrarily reserved for request numbers.

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

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

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

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index de67ce647501..49358f20d36f 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -44,7 +44,7 @@
#define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
#endif

-#define KVM_REQ_VCPU_EXIT 8
+#define KVM_REQ_VCPU_EXIT (8 | KVM_REQUEST_NO_WAKEUP)

u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
int __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 522e4f60976e..1c9458a7ec92 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -41,7 +41,7 @@

#define KVM_VCPU_MAX_FEATURES 4

-#define KVM_REQ_VCPU_EXIT 8
+#define KVM_REQ_VCPU_EXIT (8 | KVM_REQUEST_NO_WAKEUP)

int __attribute_const__ kvm_target_cpu(void);
int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f5c942edbc86..19219826bed6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -61,10 +61,10 @@
#define KVM_REQ_PMI 19
#define KVM_REQ_SMI 20
#define KVM_REQ_MASTERCLOCK_UPDATE 21
-#define KVM_REQ_MCLOCK_INPROGRESS 22
-#define KVM_REQ_SCAN_IOAPIC 23
+#define KVM_REQ_MCLOCK_INPROGRESS (22 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_SCAN_IOAPIC (23 | KVM_REQUEST_NO_WAKEUP)
#define KVM_REQ_GLOBAL_CLOCK_UPDATE 24
-#define KVM_REQ_APIC_PAGE_RELOAD 25
+#define KVM_REQ_APIC_PAGE_RELOAD (25 | KVM_REQUEST_NO_WAKEUP)
#define KVM_REQ_HV_CRASH 26
#define KVM_REQ_IOAPIC_EOI_EXIT 27
#define KVM_REQ_HV_RESET 28
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 374fa92c7657..a805ddcb7eb0 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -115,12 +115,14 @@ static inline bool is_error_page(struct page *page)
return IS_ERR(page);
}

+#define KVM_REQUEST_MASK GENMASK(7,0)
+#define KVM_REQUEST_NO_WAKEUP BIT(8)
/*
* Architecture-independent vcpu->requests bit members
* Bits 4-7 are reserved for more arch-independent bits.
*/
-#define KVM_REQ_TLB_FLUSH 0
-#define KVM_REQ_MMU_RELOAD 1
+#define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_NO_WAKEUP)
#define KVM_REQ_PENDING_TIMER 2
#define KVM_REQ_UNHALT 3

@@ -1076,17 +1078,17 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
* caller. Paired with the smp_mb__after_atomic in kvm_check_request.
*/
smp_wmb();
- set_bit(req, &vcpu->requests);
+ set_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
}

static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
{
- return test_bit(req, &vcpu->requests);
+ return test_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
}

static inline void kvm_clear_request(int req, struct kvm_vcpu *vcpu)
{
- clear_bit(req, &vcpu->requests);
+ clear_bit(req & KVM_REQUEST_MASK, &vcpu->requests);
}

static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
--
2.12.2

2017-04-26 20:34:35

[permalink] [raw]

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

From: Andrew Jones <[email protected]>

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

No functional change.

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

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

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 49358f20d36f..1d48a4b65b86 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -44,7 +44,7 @@
#define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
#endif

-#define KVM_REQ_VCPU_EXIT (8 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_NO_WAKEUP(0)

u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
int __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1c9458a7ec92..d3370b79660e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -41,7 +41,7 @@

#define KVM_VCPU_MAX_FEATURES 4

-#define KVM_REQ_VCPU_EXIT (8 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_NO_WAKEUP(0)

int __attribute_const__ kvm_target_cpu(void);
int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 77c60826d145..51b4fda269c9 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -52,8 +52,8 @@
#define KVM_IRQCHIP_NUM_PINS 256

/* PPC-specific vcpu->requests bit members */
-#define KVM_REQ_WATCHDOG 8
-#define KVM_REQ_EPR_EXIT 9
+#define KVM_REQ_WATCHDOG KVM_ARCH_REQ(0)
+#define KVM_REQ_EPR_EXIT KVM_ARCH_REQ(1)

#include <linux/mmu_notifier.h>

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 426614a882a9..9c3bd94204ac 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -42,9 +42,9 @@
#define KVM_HALT_POLL_NS_DEFAULT 80000

/* s390-specific vcpu->requests bit members */
-#define KVM_REQ_ENABLE_IBS 8
-#define KVM_REQ_DISABLE_IBS 9
-#define KVM_REQ_ICPT_OPEREXC 10
+#define KVM_REQ_ENABLE_IBS KVM_ARCH_REQ(0)
+#define KVM_REQ_DISABLE_IBS KVM_ARCH_REQ(1)
+#define KVM_REQ_ICPT_OPEREXC KVM_ARCH_REQ(2)

#define SIGP_CTRL_C 0x80
#define SIGP_CTRL_SCN_MASK 0x3f
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 19219826bed6..15eb7d3837e3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -48,28 +48,28 @@
#define KVM_IRQCHIP_NUM_PINS KVM_IOAPIC_NUM_PINS

/* x86-specific vcpu->requests bit members */
-#define KVM_REQ_MIGRATE_TIMER 8
-#define KVM_REQ_REPORT_TPR_ACCESS 9
-#define KVM_REQ_TRIPLE_FAULT 10
-#define KVM_REQ_MMU_SYNC 11
-#define KVM_REQ_CLOCK_UPDATE 12
-#define KVM_REQ_EVENT 14
-#define KVM_REQ_APF_HALT 15
-#define KVM_REQ_STEAL_UPDATE 16
-#define KVM_REQ_NMI 17
-#define KVM_REQ_PMU 18
-#define KVM_REQ_PMI 19
-#define KVM_REQ_SMI 20
-#define KVM_REQ_MASTERCLOCK_UPDATE 21
-#define KVM_REQ_MCLOCK_INPROGRESS (22 | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_SCAN_IOAPIC (23 | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_GLOBAL_CLOCK_UPDATE 24
-#define KVM_REQ_APIC_PAGE_RELOAD (25 | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_HV_CRASH 26
-#define KVM_REQ_IOAPIC_EOI_EXIT 27
-#define KVM_REQ_HV_RESET 28
-#define KVM_REQ_HV_EXIT 29
-#define KVM_REQ_HV_STIMER 30
+#define KVM_REQ_MIGRATE_TIMER KVM_ARCH_REQ(0)
+#define KVM_REQ_REPORT_TPR_ACCESS KVM_ARCH_REQ(1)
+#define KVM_REQ_TRIPLE_FAULT KVM_ARCH_REQ(2)
+#define KVM_REQ_MMU_SYNC KVM_ARCH_REQ(3)
+#define KVM_REQ_CLOCK_UPDATE KVM_ARCH_REQ(4)
+#define KVM_REQ_EVENT KVM_ARCH_REQ(6)
+#define KVM_REQ_APF_HALT KVM_ARCH_REQ(7)
+#define KVM_REQ_STEAL_UPDATE KVM_ARCH_REQ(8)
+#define KVM_REQ_NMI KVM_ARCH_REQ(9)
+#define KVM_REQ_PMU KVM_ARCH_REQ(10)
+#define KVM_REQ_PMI KVM_ARCH_REQ(11)
+#define KVM_REQ_SMI KVM_ARCH_REQ(12)
+#define KVM_REQ_MASTERCLOCK_UPDATE KVM_ARCH_REQ(13)
+#define KVM_REQ_MCLOCK_INPROGRESS KVM_ARCH_REQ_NO_WAKEUP(14)
+#define KVM_REQ_SCAN_IOAPIC KVM_ARCH_REQ_NO_WAKEUP(15)
+#define KVM_REQ_GLOBAL_CLOCK_UPDATE KVM_ARCH_REQ(16)
+#define KVM_REQ_APIC_PAGE_RELOAD KVM_ARCH_REQ_NO_WAKEUP(17)
+#define KVM_REQ_HV_CRASH KVM_ARCH_REQ(18)
+#define KVM_REQ_IOAPIC_EOI_EXIT KVM_ARCH_REQ(19)
+#define KVM_REQ_HV_RESET KVM_ARCH_REQ(20)
+#define KVM_REQ_HV_EXIT KVM_ARCH_REQ(21)
+#define KVM_REQ_HV_STIMER KVM_ARCH_REQ(22)

#define CR0_RESERVED_BITS \
(~(unsigned long)(X86_CR0_PE | X86_CR0_MP | X86_CR0_EM | X86_CR0_TS \
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 84c5396564f7..955debd82cf2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -125,6 +125,14 @@ static inline bool is_error_page(struct page *page)
#define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_NO_WAKEUP)
#define KVM_REQ_PENDING_TIMER 2
#define KVM_REQ_UNHALT 3
+#define KVM_REQUEST_ARCH_BASE 8
+
+#define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
+ BUILD_BUG_ON((unsigned)(nr) >= 32 - KVM_REQUEST_ARCH_BASE); \
+ (unsigned)(((nr) + KVM_REQUEST_ARCH_BASE) | (flags)); \
+})
+#define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0)
+#define KVM_ARCH_REQ_NO_WAKEUP(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP)

#define KVM_USERSPACE_IRQ_SOURCE_ID 0
#define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1
--
2.12.2

2017-04-26 20:35:07

[permalink] [raw]

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

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

Acked-by: Christian Borntraeger <[email protected]>
Signed-off-by: Radim Krčmář <[email protected]>
---
virt/kvm/kvm_main.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 357e67cba32e..e5d52b46b531 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2195,7 +2195,6 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
}
EXPORT_SYMBOL_GPL(kvm_vcpu_block);

-#ifndef CONFIG_S390
void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
{
struct swait_queue_head *wqp;
@@ -2225,7 +2224,6 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
put_cpu();
}
EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
-#endif /* !CONFIG_S390 */

int kvm_vcpu_yield_to(struct kvm_vcpu *target)
{
--
2.12.2

2017-04-26 20:33:15

[permalink] [raw]

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

Reviewed-by: David Hildenbrand <[email protected]>
Signed-off-by: Radim Krčmář <[email protected]>
---
arch/x86/kvm/x86.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2de54c20fa9e..0936c3e2e51c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2203,8 +2203,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
bool tmp = (msr == MSR_KVM_SYSTEM_TIME);

if (ka->boot_vcpu_runs_old_kvmclock != tmp)
- set_bit(KVM_REQ_MASTERCLOCK_UPDATE,
- &vcpu->requests);
+ kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

ka->boot_vcpu_runs_old_kvmclock = tmp;
}
@@ -2802,11 +2801,6 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
return kvm_arch_has_noncoherent_dma(vcpu->kvm);
}

-static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu)
-{
- set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests);
-}
-
void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
{
/* Address WBINVD may be executed by guest */
@@ -2850,7 +2844,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
if (!vcpu->kvm->arch.use_master_clock || vcpu->cpu == -1)
kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
if (vcpu->cpu != cpu)
- kvm_migrate_timers(vcpu);
+ kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu);
vcpu->cpu = cpu;
}

--
2.12.2

2017-04-26 20:34:53

[permalink] [raw]

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

From: Andrew Jones <[email protected]>

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

kvm_vcpu_kick
kvm_arch_vcpu_should_kick
kvm_vcpu_exiting_guest_mode
cmpxchg

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

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

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0936c3e2e51c..69fcee26f4da 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6853,7 +6853,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)

/*
* 1) We should set ->mode before checking ->requests. Please see
- * the comment in kvm_make_all_cpus_request.
+ * the comment in kvm_vcpu_exiting_guest_mode().
*
* 2) For APICv, we should set ->mode before checking PIR.ON. This
* pairs with the memory barrier implicit in pi_test_and_set_on
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a805ddcb7eb0..84c5396564f7 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -270,6 +270,12 @@ struct kvm_vcpu {

static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
{
+ /*
+ * The memory barrier ensures a previous write to vcpu->requests cannot
+ * be reordered with the read of vcpu->mode. It pairs with the general
+ * memory barrier following the write of vcpu->mode in VCPU RUN.
+ */
+ smp_mb__before_atomic();
return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE);
}

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3772f7dcc72d..1efb07643035 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -183,9 +183,6 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
kvm_make_request(req, vcpu);
cpu = vcpu->cpu;

- /* Set ->requests bit before we read ->mode. */
- smp_mb__after_atomic();
-
if (!(req & KVM_REQUEST_NO_WAKEUP))
kvm_vcpu_wake_up(vcpu);

--
2.12.2

2017-04-26 20:33:41

[permalink] [raw]

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

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

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

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

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

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

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

Signed-off-by: Radim Krčmář <[email protected]>
---
virt/kvm/kvm_main.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e5d52b46b531..3772f7dcc72d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
/* Set ->requests bit before we read ->mode. */
smp_mb__after_atomic();

+ if (!(req & KVM_REQUEST_NO_WAKEUP))
+ kvm_vcpu_wake_up(vcpu);
+
if (cpus != NULL && cpu != -1 && cpu != me &&
kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
cpumask_set_cpu(cpu, cpus);
--
2.12.2

2017-04-26 20:33:58

[permalink] [raw]

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

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

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

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 955debd82cf2..38cfe372918c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -698,7 +698,7 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn);
void kvm_vcpu_block(struct kvm_vcpu *vcpu);
void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu);
void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu);
-void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
+bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
int kvm_vcpu_yield_to(struct kvm_vcpu *target);
void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1efb07643035..632f7b3e198c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -183,8 +183,8 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
kvm_make_request(req, vcpu);
cpu = vcpu->cpu;

- if (!(req & KVM_REQUEST_NO_WAKEUP))
- kvm_vcpu_wake_up(vcpu);
+ if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu))
+ continue;

if (cpus != NULL && cpu != -1 && cpu != me &&
kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
@@ -2195,7 +2195,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
}
EXPORT_SYMBOL_GPL(kvm_vcpu_block);

-void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
+bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
{
struct swait_queue_head *wqp;

@@ -2203,8 +2203,10 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
if (swait_active(wqp)) {
swake_up(wqp);
++vcpu->stat.halt_wakeup;
+ return true;
}

+ return false;
}
EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);

@@ -2216,7 +2218,9 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
int me;
int cpu = vcpu->cpu;

- kvm_vcpu_wake_up(vcpu);
+ if (kvm_vcpu_wake_up(vcpu))
+ return;
+
me = get_cpu();
if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
if (kvm_arch_vcpu_should_kick(vcpu))
--
2.12.2

2017-04-26 20:34:07

[permalink] [raw]

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

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

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

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

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

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 1d48a4b65b86..2190a7ddd515 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -44,7 +44,7 @@
#define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
#endif

-#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_NO_WAKEUP(0)
+#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_WAIT_NO_WAKEUP(0)

u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
int __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d3370b79660e..98f3d01ae91b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -41,7 +41,7 @@

#define KVM_VCPU_MAX_FEATURES 4

-#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_NO_WAKEUP(0)
+#define KVM_REQ_VCPU_EXIT KVM_ARCH_REQ_WAIT_NO_WAKEUP(0)

int __attribute_const__ kvm_target_cpu(void);
int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 15eb7d3837e3..77083d7e9540 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -61,10 +61,10 @@
#define KVM_REQ_PMI KVM_ARCH_REQ(11)
#define KVM_REQ_SMI KVM_ARCH_REQ(12)
#define KVM_REQ_MASTERCLOCK_UPDATE KVM_ARCH_REQ(13)
-#define KVM_REQ_MCLOCK_INPROGRESS KVM_ARCH_REQ_NO_WAKEUP(14)
-#define KVM_REQ_SCAN_IOAPIC KVM_ARCH_REQ_NO_WAKEUP(15)
+#define KVM_REQ_MCLOCK_INPROGRESS KVM_ARCH_REQ_WAIT_NO_WAKEUP(14)
+#define KVM_REQ_SCAN_IOAPIC KVM_ARCH_REQ_WAIT_NO_WAKEUP(15)
#define KVM_REQ_GLOBAL_CLOCK_UPDATE KVM_ARCH_REQ(16)
-#define KVM_REQ_APIC_PAGE_RELOAD KVM_ARCH_REQ_NO_WAKEUP(17)
+#define KVM_REQ_APIC_PAGE_RELOAD KVM_ARCH_REQ_WAIT_NO_WAKEUP(17)
#define KVM_REQ_HV_CRASH KVM_ARCH_REQ(18)
#define KVM_REQ_IOAPIC_EOI_EXIT KVM_ARCH_REQ(19)
#define KVM_REQ_HV_RESET KVM_ARCH_REQ(20)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 38cfe372918c..a668f33b20dc 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -117,12 +117,13 @@ static inline bool is_error_page(struct page *page)

#define KVM_REQUEST_MASK GENMASK(7,0)
#define KVM_REQUEST_NO_WAKEUP BIT(8)
+#define KVM_REQUEST_WAIT BIT(9)
/*
* Architecture-independent vcpu->requests bit members
* Bits 4-7 are reserved for more arch-independent bits.
*/
-#define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_TLB_FLUSH (0 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)
+#define KVM_REQ_MMU_RELOAD (1 | KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)
#define KVM_REQ_PENDING_TIMER 2
#define KVM_REQ_UNHALT 3
#define KVM_REQUEST_ARCH_BASE 8
@@ -133,6 +134,9 @@ static inline bool is_error_page(struct page *page)
})
#define KVM_ARCH_REQ(nr) KVM_ARCH_REQ_FLAGS(nr, 0)
#define KVM_ARCH_REQ_NO_WAKEUP(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP)
+#define KVM_ARCH_REQ_WAIT(nr) KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP)
+#define KVM_ARCH_REQ_WAIT_NO_WAKEUP(nr) \
+ KVM_ARCH_REQ_FLAGS(nr, KVM_REQUEST_NO_WAKEUP | KVM_REQUEST_WAIT)

#define KVM_USERSPACE_IRQ_SOURCE_ID 0
#define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 632f7b3e198c..dbf0410cd8b2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -165,6 +165,15 @@ void vcpu_put(struct kvm_vcpu *vcpu)
}
EXPORT_SYMBOL_GPL(vcpu_put);

+/* TODO: merge with kvm_arch_vcpu_should_kick */
+static bool kvm_should_kick_request(struct kvm_vcpu *vcpu, unsigned req)
+{
+ int mode = kvm_vcpu_exiting_guest_mode(vcpu);
+
+ return req & KVM_REQUEST_WAIT ?
+ mode != OUTSIDE_GUEST_MODE : mode == IN_GUEST_MODE;
+}
+
static void ack_flush(void *_completed)
{
}
@@ -174,6 +183,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
int i, cpu, me;
cpumask_var_t cpus;
bool called = true;
+ bool wait = req & KVM_REQUEST_WAIT;
struct kvm_vcpu *vcpu;

zalloc_cpumask_var(&cpus, GFP_ATOMIC);
@@ -187,13 +197,13 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
continue;

if (cpus != NULL && cpu != -1 && cpu != me &&
- kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
+ kvm_should_kick_request(vcpu, req))
cpumask_set_cpu(cpu, cpus);
}
if (unlikely(cpus == NULL))
- smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+ smp_call_function_many(cpu_online_mask, ack_flush, NULL, wait);
else if (!cpumask_empty(cpus))
- smp_call_function_many(cpus, ack_flush, NULL, 1);
+ smp_call_function_many(cpus, ack_flush, NULL, wait);
else
called = false;
put_cpu();
--
2.12.2

2017-04-27 11:33:19

[permalink] [raw]

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

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

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

>
> Reviewed-by: Christian Borntraeger <[email protected]>
> Signed-off-by: Radim Krčmář <[email protected]>

Otherwise
Reviewed-by: Andrew Jones <[email protected]>

2017-04-27 11:33:38

[permalink] [raw]

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

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

Reviewed-by: Andrew Jones <[email protected]>

2017-04-27 11:35:46

[permalink] [raw]

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

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

Reviewed-by: Andrew Jones <[email protected]>

2017-04-27 11:37:05

[permalink] [raw]

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

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

Reviewed-by: Andrew Jones <[email protected]>

2017-04-27 11:39:11

[permalink] [raw]

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

On Wed, 26 Apr 2017 22:32:19 +0200
Radim Krčmář <[email protected]> wrote:

> Users were expected to use kvm_check_request() for testing and clearing,
> but request have expanded their use since then and some users want to
> only test or do a faster clear.
>
> Make sure that requests are not directly accessed with bit operations.
>
> Reviewed-by: Christian Borntraeger <[email protected]>
> Signed-off-by: Radim Krčmář <[email protected]>
> ---
> arch/mips/kvm/emulate.c | 2 +-
> arch/powerpc/kvm/book3s_pr.c | 2 +-
> arch/powerpc/kvm/book3s_pr_papr.c | 2 +-
> arch/powerpc/kvm/booke.c | 4 ++--
> arch/powerpc/kvm/powerpc.c | 2 +-
> arch/s390/kvm/kvm-s390.c | 2 +-
> arch/x86/kvm/vmx.c | 2 +-
> arch/x86/kvm/x86.c | 8 ++++----
> include/linux/kvm_host.h | 14 ++++++++++++--
> 9 files changed, 24 insertions(+), 14 deletions(-)

Reviewed-by: Cornelia Huck <[email protected]>

2017-04-27 11:41:27

[permalink] [raw]

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

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

Reviewed-by: Andrew Jones <[email protected]>

2017-04-27 11:46:05

[permalink] [raw]

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

On Wed, 26 Apr 2017 22:32:21 +0200
Radim Krčmář <[email protected]> wrote:

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

Reviewed-by: Cornelia Huck <[email protected]>

2017-04-27 11:55:51

[permalink] [raw]

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

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

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

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

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

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

copy+paste mistake: s/KVM_REQUEST_NO_WAKEUP/KVM_REQUEST_WAIT/

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

Thanks,
drew

2017-04-27 12:00:38

[permalink] [raw]

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

On Wed, 26 Apr 2017 22:32:22 +0200
Radim Krčmář <[email protected]> wrote:

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

Reviewed-by: Cornelia Huck <[email protected]>

2017-04-27 12:07:15

[permalink] [raw]

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

On Wed, 26 Apr 2017 22:32:23 +0200
Radim Krčmář <[email protected]> wrote:

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

s/the all/they all/

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

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

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

The code change looks good to me.

2017-04-27 12:11:19