Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631704 > unrolled thread

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

Started byRadim Krčmář <rkrcmar@redhat.com>
First post2017-04-26 22:40 +0200
Last post2017-04-27 14:40 +0200
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 9/9] KVM: mark requests that need synchronization Radim Krčmář <rkrcmar@redhat.com> - 2017-04-26 22:40 +0200
    Re: [PATCH v2 9/9] KVM: mark requests that need synchronization Andrew Jones <drjones@redhat.com> - 2017-04-27 14:00 +0200
    Re: [PATCH v2 9/9] KVM: mark requests that need synchronization Paolo Bonzini <pbonzini@redhat.com> - 2017-04-27 14:40 +0200

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

FromRadim Krčmář <rkrcmar@redhat.com>
Date2017-04-26 22:40 +0200
Subject[PATCH v2 9/9] KVM: mark requests that need synchronization
Message-ID<tABTb-7dC-1@gated-at.bofh.it>
kvm_make_all_requests() provides a synchronization that waits until all
kicked VCPUs have acknowledged the kick.  This is important for
KVM_REQ_MMU_RELOAD as it prevents freeing while lockless paging is
underway.

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

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

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

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

[toc] | [next] | [standalone]

#1632050

FromAndrew Jones <drjones@redhat.com>
Date2017-04-27 14:00 +0200
Message-ID<tAQfv-8w0-11@gated-at.bofh.it>
In reply to#1631704
On Wed, Apr 26, 2017 at 10:32:27PM +0200, Radim Krčmář wrote:
> kvm_make_all_requests() provides a synchronization that waits until all
> kicked VCPUs have acknowledged the kick.  This is important for
> KVM_REQ_MMU_RELOAD as it prevents freeing while lockless paging is
> underway.
> 
> This patch adds the synchronization property into all requests that are
> currently being used with kvm_make_all_requests() in order to preserve
> the current behavior and only introduce a new framework.  Removing it
> from requests where it is not necessary is left for future patches.
> 
> A question is whether this propertly isn't better expressed as a
> function of the caller.

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

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

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

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

copy+paste mistake: s/KVM_REQUEST_NO_WAKEUP/KVM_REQUEST_WAIT/

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

Thanks,
drew

[toc] | [prev] | [next] | [standalone]

#1632073

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-04-27 14:40 +0200
Message-ID<tAQSe-Bq-11@gated-at.bofh.it>
In reply to#1631704

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

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

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

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

Paolo

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web