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

Groups > linux.kernel > #1716855 > unrolled thread

[PATCH RFC v3 0/9] KVM: allow dynamic kvm->vcpus array

Back to article view | Back to linux.kernel

Contents

  [PATCH RFC v3 0/9] KVM: allow dynamic kvm->vcpus array Radim Krčmář <rkrcmar@redhat.com> - 2017-08-21 22:40 +0200
    [PATCH RFC v3 6/9] KVM: rework kvm_vcpu_on_spin loop Radim Krčmář <rkrcmar@redhat.com> - 2017-08-21 22:40 +0200
      Re: [PATCH RFC v3 6/9] KVM: rework kvm_vcpu_on_spin loop David Hildenbrand <david@redhat.com> - 2017-08-22 16:10 +0200
    [PATCH RFC v3 5/9] KVM: remove unused __KVM_HAVE_ARCH_VM_ALLOC Radim Krčmář <rkrcmar@redhat.com> - 2017-08-21 22:40 +0200
    [PATCH RFC v3 1/9] KVM: s390: optimize detection of started vcpus Radim Krčmář <rkrcmar@redhat.com> - 2017-08-21 22:40 +0200
      Re: [PATCH RFC v3 1/9] KVM: s390: optimize detection of started vcpus Christian Borntraeger <borntraeger@de.ibm.com> - 2017-08-22 09:30 +0200
      Re: [PATCH RFC v3 1/9] KVM: s390: optimize detection of started vcpus David Hildenbrand <david@redhat.com> - 2017-08-22 13:40 +0200
    [PATCH RFC v3 2/9] KVM: arm/arm64: fix vcpu self-detection in vgic_v3_dispatch_sgi() Radim Krčmář <rkrcmar@redhat.com> - 2017-08-21 22:40 +0200
      Re: [PATCH RFC v3 2/9] KVM: arm/arm64: fix vcpu self-detection in  vgic_v3_dispatch_sgi() David Hildenbrand <david@redhat.com> - 2017-08-22 13:50 +0200
    [PATCH RFC v3 7/9] KVM: add kvm_free_vcpus and kvm_arch_free_vcpus Radim Krčmář <rkrcmar@redhat.com> - 2017-08-21 22:40 +0200
      Re: [PATCH RFC v3 7/9] KVM: add kvm_free_vcpus and  kvm_arch_free_vcpus David Hildenbrand <david@redhat.com> - 2017-08-22 16:20 +0200
    [PATCH RFC v3 4/9] KVM: arm/arm64: use locking helpers in kvm_vgic_create() Radim Krčmář <rkrcmar@redhat.com> - 2017-08-21 22:40 +0200
      Re: [PATCH RFC v3 4/9] KVM: arm/arm64: use locking helpers in  kvm_vgic_create() David Hildenbrand <david@redhat.com> - 2017-08-22 14:00 +0200
    [PATCH RFC v3 3/9] KVM: remember position in kvm->vcpus array Radim Krčmář <rkrcmar@redhat.com> - 2017-08-21 22:50 +0200
      Re: [PATCH RFC v3 3/9] KVM: remember position in kvm->vcpus array David Hildenbrand <david@redhat.com> - 2017-08-22 13:50 +0200

#1716855 — [PATCH RFC v3 0/9] KVM: allow dynamic kvm->vcpus array

The only common part with v2 is [v3 5/9].

The crucial part of this series is adding a separate mechanism for
kvm_for_each_vcpu() [v3 8/9] and with that change, I think that the
dynamic array [v3 9/9] would be nicer if protected by RCU, like in v2:
The protection can be nicely hidden in kvm_get_vcpu().  I just had the
split done before implementing [v3 8/9] and presented it for
consideration.

Smoke tested on x86 only.


Radim Krčmář (9):
  KVM: s390: optimize detection of started vcpus
  KVM: arm/arm64: fix vcpu self-detection in vgic_v3_dispatch_sgi()
  KVM: remember position in kvm->vcpus array
  KVM: arm/arm64: use locking helpers in kvm_vgic_create()
  KVM: remove unused __KVM_HAVE_ARCH_VM_ALLOC
  KVM: rework kvm_vcpu_on_spin loop
  KVM: add kvm_free_vcpus and kvm_arch_free_vcpus
  KVM: implement kvm_for_each_vcpu with a list
  KVM: split kvm->vcpus into chunks

 arch/mips/kvm/mips.c                |  19 ++----
 arch/powerpc/kvm/book3s_32_mmu.c    |   3 +-
 arch/powerpc/kvm/book3s_64_mmu.c    |   3 +-
 arch/powerpc/kvm/book3s_hv.c        |   7 +-
 arch/powerpc/kvm/book3s_pr.c        |   5 +-
 arch/powerpc/kvm/book3s_xics.c      |   2 +-
 arch/powerpc/kvm/book3s_xics.h      |   3 +-
 arch/powerpc/kvm/book3s_xive.c      |  18 +++---
 arch/powerpc/kvm/book3s_xive.h      |   3 +-
 arch/powerpc/kvm/e500_emulate.c     |   3 +-
 arch/powerpc/kvm/powerpc.c          |  16 ++---
 arch/s390/include/asm/kvm_host.h    |   1 +
 arch/s390/kvm/interrupt.c           |   3 +-
 arch/s390/kvm/kvm-s390.c            |  77 ++++++++--------------
 arch/s390/kvm/kvm-s390.h            |   6 +-
 arch/s390/kvm/sigp.c                |   3 +-
 arch/x86/kvm/hyperv.c               |   3 +-
 arch/x86/kvm/i8254.c                |   3 +-
 arch/x86/kvm/i8259.c                |   7 +-
 arch/x86/kvm/ioapic.c               |   3 +-
 arch/x86/kvm/irq_comm.c             |  10 +--
 arch/x86/kvm/lapic.c                |   5 +-
 arch/x86/kvm/svm.c                  |   3 +-
 arch/x86/kvm/vmx.c                  |   5 +-
 arch/x86/kvm/x86.c                  |  34 ++++------
 include/linux/kvm_host.h            |  81 ++++++++++++-----------
 virt/kvm/arm/arch_timer.c           |  10 ++-
 virt/kvm/arm/arm.c                  |  25 ++++----
 virt/kvm/arm/pmu.c                  |   3 +-
 virt/kvm/arm/psci.c                 |   7 +-
 virt/kvm/arm/vgic/vgic-init.c       |  31 ++++-----
 virt/kvm/arm/vgic/vgic-kvm-device.c |  30 +++++----
 virt/kvm/arm/vgic/vgic-mmio-v2.c    |   5 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c    |  22 ++++---
 virt/kvm/arm/vgic/vgic.c            |   3 +-
 virt/kvm/kvm_main.c                 | 124 +++++++++++++++++++++++-------------
 36 files changed, 278 insertions(+), 308 deletions(-)

-- 
2.13.3

[toc] | [next] | [standalone]

#1716856 — [PATCH RFC v3 6/9] KVM: rework kvm_vcpu_on_spin loop

The original code managed to obfuscate a straightforward idea:
start iterating from the selected index and reset the index to 0 when
reaching the end of online vcpus, then iterate until reaching the index
that we started at.

The resulting code is a bit better, IMO.  (Still horrible, though.)

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

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index abd5cb1feb9e..cfb3c0efdd51 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -498,6 +498,19 @@ static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
 	     (vcpup = kvm_get_vcpu(kvm, idx)) != NULL; \
 	     idx++)
 
+#define kvm_for_each_vcpu_from(idx, vcpup, from, kvm) \
+	for (idx = from, vcpup = kvm_get_vcpu(kvm, idx); \
+	     vcpup; \
+	     ({ \
+		idx++; \
+		if (idx >= atomic_read(&kvm->online_vcpus)) \
+			idx = 0; \
+		if (idx == from) \
+			vcpup = NULL; \
+		else \
+			vcpup = kvm_get_vcpu(kvm, idx); \
+	      }))
+
 static inline struct kvm_vcpu *kvm_get_vcpu_by_id(struct kvm *kvm, int id)
 {
 	struct kvm_vcpu *vcpu = NULL;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d89261d0d8c6..33a15e176927 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2333,8 +2333,7 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
 	struct kvm_vcpu *vcpu;
 	int last_boosted_vcpu = me->kvm->last_boosted_vcpu;
 	int yielded = 0;
-	int try = 3;
-	int pass;
+	int try = 2;
 	int i;
 
 	kvm_vcpu_set_in_spin_loop(me, true);
@@ -2345,34 +2344,24 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
 	 * VCPU is holding the lock that we need and will release it.
 	 * We approximate round-robin by starting at the last boosted VCPU.
 	 */
-	for (pass = 0; pass < 2 && !yielded && try; pass++) {
-		kvm_for_each_vcpu(i, vcpu, kvm) {
-			if (!pass && i <= last_boosted_vcpu) {
-				i = last_boosted_vcpu;
-				continue;
-			} else if (pass && i > last_boosted_vcpu)
-				break;
-			if (!ACCESS_ONCE(vcpu->preempted))
-				continue;
-			if (vcpu == me)
-				continue;
-			if (swait_active(&vcpu->wq) && !kvm_arch_vcpu_runnable(vcpu))
-				continue;
-			if (yield_to_kernel_mode && !kvm_arch_vcpu_in_kernel(vcpu))
-				continue;
-			if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
-				continue;
+	kvm_for_each_vcpu_from(i, vcpu, last_boosted_vcpu, kvm) {
+		if (!ACCESS_ONCE(vcpu->preempted))
+			continue;
+		if (vcpu == me)
+			continue;
+		if (swait_active(&vcpu->wq) && !kvm_arch_vcpu_runnable(vcpu))
+			continue;
+		if (yield_to_kernel_mode && !kvm_arch_vcpu_in_kernel(vcpu))
+			continue;
+		if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
+			continue;
 
-			yielded = kvm_vcpu_yield_to(vcpu);
-			if (yielded > 0) {
-				kvm->last_boosted_vcpu = i;
-				break;
-			} else if (yielded < 0) {
-				try--;
-				if (!try)
-					break;
-			}
-		}
+		yielded = kvm_vcpu_yield_to(vcpu);
+		if (yielded > 0) {
+			kvm->last_boosted_vcpu = i;
+			break;
+		} else if (yielded < 0 && !try--)
+			break;
 	}
 	kvm_vcpu_set_in_spin_loop(me, false);
 
-- 
2.13.3

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

#1717445 — Re: [PATCH RFC v3 6/9] KVM: rework kvm_vcpu_on_spin loop

On 21.08.2017 22:35, Radim Krčmář wrote:
> The original code managed to obfuscate a straightforward idea:
> start iterating from the selected index and reset the index to 0 when
> reaching the end of online vcpus, then iterate until reaching the index
> that we started at.
> 
> The resulting code is a bit better, IMO.  (Still horrible, though.)

I think I prefer dropping this patch and maybe _after_ we have the list
implementation in place, simply start walking the list from
last_boosted_vcpu? (store a pointer instead of an index then, of course)

If I understand correctly, this would then be simply, one walk from
last_boosted_vcpu until we hit last_boosted_vcpu again.


> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  include/linux/kvm_host.h | 13 +++++++++++++
>  virt/kvm/kvm_main.c      | 47 ++++++++++++++++++-----------------------------
>  2 files changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index abd5cb1feb9e..cfb3c0efdd51 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -498,6 +498,19 @@ static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
>  	     (vcpup = kvm_get_vcpu(kvm, idx)) != NULL; \
>  	     idx++)
>  
> +#define kvm_for_each_vcpu_from(idx, vcpup, from, kvm) \
> +	for (idx = from, vcpup = kvm_get_vcpu(kvm, idx); \
> +	     vcpup; \
> +	     ({ \
> +		idx++; \
> +		if (idx >= atomic_read(&kvm->online_vcpus)) \
> +			idx = 0; \
> +		if (idx == from) \
> +			vcpup = NULL; \
> +		else \
> +			vcpup = kvm_get_vcpu(kvm, idx); \
> +	      }))
> +
>  static inline struct kvm_vcpu *kvm_get_vcpu_by_id(struct kvm *kvm, int id)
>  {
>  	struct kvm_vcpu *vcpu = NULL;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d89261d0d8c6..33a15e176927 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2333,8 +2333,7 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
>  	struct kvm_vcpu *vcpu;
>  	int last_boosted_vcpu = me->kvm->last_boosted_vcpu;
>  	int yielded = 0;
> -	int try = 3;
> -	int pass;
> +	int try = 2;
>  	int i;
>  
>  	kvm_vcpu_set_in_spin_loop(me, true);
> @@ -2345,34 +2344,24 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
>  	 * VCPU is holding the lock that we need and will release it.
>  	 * We approximate round-robin by starting at the last boosted VCPU.
>  	 */
> -	for (pass = 0; pass < 2 && !yielded && try; pass++) {
> -		kvm_for_each_vcpu(i, vcpu, kvm) {
> -			if (!pass && i <= last_boosted_vcpu) {
> -				i = last_boosted_vcpu;
> -				continue;
> -			} else if (pass && i > last_boosted_vcpu)
> -				break;
> -			if (!ACCESS_ONCE(vcpu->preempted))
> -				continue;
> -			if (vcpu == me)
> -				continue;
> -			if (swait_active(&vcpu->wq) && !kvm_arch_vcpu_runnable(vcpu))
> -				continue;
> -			if (yield_to_kernel_mode && !kvm_arch_vcpu_in_kernel(vcpu))
> -				continue;
> -			if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
> -				continue;
> +	kvm_for_each_vcpu_from(i, vcpu, last_boosted_vcpu, kvm) {
> +		if (!ACCESS_ONCE(vcpu->preempted))
> +			continue;
> +		if (vcpu == me)
> +			continue;
> +		if (swait_active(&vcpu->wq) && !kvm_arch_vcpu_runnable(vcpu))
> +			continue;
> +		if (yield_to_kernel_mode && !kvm_arch_vcpu_in_kernel(vcpu))
> +			continue;
> +		if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
> +			continue;
>  
> -			yielded = kvm_vcpu_yield_to(vcpu);
> -			if (yielded > 0) {
> -				kvm->last_boosted_vcpu = i;
> -				break;
> -			} else if (yielded < 0) {
> -				try--;
> -				if (!try)
> -					break;
> -			}
> -		}
> +		yielded = kvm_vcpu_yield_to(vcpu);
> +		if (yielded > 0) {
> +			kvm->last_boosted_vcpu = i;
> +			break;
> +		} else if (yielded < 0 && !try--)
> +			break;
>  	}
>  	kvm_vcpu_set_in_spin_loop(me, false);
>  
> 


-- 

Thanks,

David

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

#1716857 — [PATCH RFC v3 5/9] KVM: remove unused __KVM_HAVE_ARCH_VM_ALLOC

Moving it to generic code will allow us to extend it with ease.
Christian noted that it was only used in the removed ia64.

Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 include/linux/kvm_host.h | 12 ------------
 virt/kvm/kvm_main.c      | 16 +++++++++++++---
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a8ff956616d2..abd5cb1feb9e 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -798,18 +798,6 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
 bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu);
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
 
-#ifndef __KVM_HAVE_ARCH_VM_ALLOC
-static inline struct kvm *kvm_arch_alloc_vm(void)
-{
-	return kzalloc(sizeof(struct kvm), GFP_KERNEL);
-}
-
-static inline void kvm_arch_free_vm(struct kvm *kvm)
-{
-	kfree(kvm);
-}
-#endif
-
 #ifdef __KVM_HAVE_ARCH_NONCOHERENT_DMA
 void kvm_arch_register_noncoherent_dma(struct kvm *kvm);
 void kvm_arch_unregister_noncoherent_dma(struct kvm *kvm);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index caf8323f7df7..d89261d0d8c6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -640,10 +640,20 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 	return 0;
 }
 
+static inline struct kvm *kvm_alloc_vm(void)
+{
+	return kzalloc(sizeof(struct kvm), GFP_KERNEL);
+}
+
+static inline void kvm_free_vm(struct kvm *kvm)
+{
+	kfree(kvm);
+}
+
 static struct kvm *kvm_create_vm(unsigned long type)
 {
 	int r, i;
-	struct kvm *kvm = kvm_arch_alloc_vm();
+	struct kvm *kvm = kvm_alloc_vm();
 
 	if (!kvm)
 		return ERR_PTR(-ENOMEM);
@@ -720,7 +730,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 		kfree(kvm_get_bus(kvm, i));
 	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++)
 		kvm_free_memslots(kvm, __kvm_memslots(kvm, i));
-	kvm_arch_free_vm(kvm);
+	kvm_free_vm(kvm);
 	mmdrop(current->mm);
 	return ERR_PTR(r);
 }
@@ -771,7 +781,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 		kvm_free_memslots(kvm, __kvm_memslots(kvm, i));
 	cleanup_srcu_struct(&kvm->irq_srcu);
 	cleanup_srcu_struct(&kvm->srcu);
-	kvm_arch_free_vm(kvm);
+	kvm_free_vm(kvm);
 	preempt_notifier_dec();
 	hardware_disable_all();
 	mmdrop(mm);
-- 
2.13.3

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

#1716858 — [PATCH RFC v3 1/9] KVM: s390: optimize detection of started vcpus

We can add a variable instead of scanning all online VCPUs to know how
many are started.  We can't trivially tell which VCPU is the last one,
though.

Suggested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/kvm-s390.c         | 39 +++++++++++++++------------------------
 2 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index a409d5991934..be0d0bdf585b 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -735,6 +735,7 @@ struct kvm_arch{
 	struct mutex ipte_mutex;
 	struct ratelimit_state sthyi_limit;
 	spinlock_t start_stop_lock;
+	unsigned started_vcpus;
 	struct sie_page2 *sie_page2;
 	struct kvm_s390_cpu_model model;
 	struct kvm_s390_crypto crypto;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 9f23a9e81a91..1534778a3c66 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3414,25 +3414,17 @@ static void __enable_ibs_on_vcpu(struct kvm_vcpu *vcpu)
 
 void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
 {
-	int i, online_vcpus, started_vcpus = 0;
-
 	if (!is_vcpu_stopped(vcpu))
 		return;
 
 	trace_kvm_s390_vcpu_start_stop(vcpu->vcpu_id, 1);
 	/* Only one cpu at a time may enter/leave the STOPPED state. */
 	spin_lock(&vcpu->kvm->arch.start_stop_lock);
-	online_vcpus = atomic_read(&vcpu->kvm->online_vcpus);
 
-	for (i = 0; i < online_vcpus; i++) {
-		if (!is_vcpu_stopped(vcpu->kvm->vcpus[i]))
-			started_vcpus++;
-	}
-
-	if (started_vcpus == 0) {
+	if (vcpu->kvm->arch.started_vcpus == 0) {
 		/* we're the only active VCPU -> speed it up */
 		__enable_ibs_on_vcpu(vcpu);
-	} else if (started_vcpus == 1) {
+	} else if (vcpu->kvm->arch.started_vcpus == 1) {
 		/*
 		 * As we are starting a second VCPU, we have to disable
 		 * the IBS facility on all VCPUs to remove potentially
@@ -3441,6 +3433,8 @@ void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
 		__disable_ibs_on_all_vcpus(vcpu->kvm);
 	}
 
+	vcpu->kvm->arch.started_vcpus++;
+
 	atomic_andnot(CPUSTAT_STOPPED, &vcpu->arch.sie_block->cpuflags);
 	/*
 	 * Another VCPU might have used IBS while we were offline.
@@ -3453,16 +3447,12 @@ void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
 
 void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
 {
-	int i, online_vcpus, started_vcpus = 0;
-	struct kvm_vcpu *started_vcpu = NULL;
-
 	if (is_vcpu_stopped(vcpu))
 		return;
 
 	trace_kvm_s390_vcpu_start_stop(vcpu->vcpu_id, 0);
 	/* Only one cpu at a time may enter/leave the STOPPED state. */
 	spin_lock(&vcpu->kvm->arch.start_stop_lock);
-	online_vcpus = atomic_read(&vcpu->kvm->online_vcpus);
 
 	/* SIGP STOP and SIGP STOP AND STORE STATUS has been fully processed */
 	kvm_s390_clear_stop_irq(vcpu);
@@ -3470,19 +3460,20 @@ void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
 	atomic_or(CPUSTAT_STOPPED, &vcpu->arch.sie_block->cpuflags);
 	__disable_ibs_on_vcpu(vcpu);
 
-	for (i = 0; i < online_vcpus; i++) {
-		if (!is_vcpu_stopped(vcpu->kvm->vcpus[i])) {
-			started_vcpus++;
-			started_vcpu = vcpu->kvm->vcpus[i];
-		}
-	}
+	vcpu->kvm->arch.started_vcpus--;
+
+	if (vcpu->kvm->arch.started_vcpus == 1) {
+		struct kvm_vcpu *started_vcpu;
 
-	if (started_vcpus == 1) {
 		/*
-		 * As we only have one VCPU left, we want to enable the
-		 * IBS facility for that VCPU to speed it up.
+		 * As we only have one VCPU left, we want to enable the IBS
+		 * facility for that VCPU to speed it up.
 		 */
-		__enable_ibs_on_vcpu(started_vcpu);
+		kvm_for_each_vcpu(i, started_vcpu, vcpu->kvm)
+			if (!is_vcpu_stopped(started_vcpu)) {
+				__enable_ibs_on_vcpu(started_vcpu);
+				break;
+			}
 	}
 
 	spin_unlock(&vcpu->kvm->arch.start_stop_lock);
-- 
2.13.3

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

#1717090 — Re: [PATCH RFC v3 1/9] KVM: s390: optimize detection of started vcpus

On 08/21/2017 10:35 PM, Radim Krčmář wrote:
> We can add a variable instead of scanning all online VCPUs to know how
> many are started.  We can't trivially tell which VCPU is the last one,
> though.
> 
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>



> @@ -3453,16 +3447,12 @@ void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
> 
>  void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>  {
> -	int i, online_vcpus, started_vcpus = 0;

here you remove i.

> -	struct kvm_vcpu *started_vcpu = NULL;
> -
>  	if (is_vcpu_stopped(vcpu))
>  		return;
> 
>  	trace_kvm_s390_vcpu_start_stop(vcpu->vcpu_id, 0);
>  	/* Only one cpu at a time may enter/leave the STOPPED state. */
>  	spin_lock(&vcpu->kvm->arch.start_stop_lock);
> -	online_vcpus = atomic_read(&vcpu->kvm->online_vcpus);
> 
>  	/* SIGP STOP and SIGP STOP AND STORE STATUS has been fully processed */
>  	kvm_s390_clear_stop_irq(vcpu);
> @@ -3470,19 +3460,20 @@ void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>  	atomic_or(CPUSTAT_STOPPED, &vcpu->arch.sie_block->cpuflags);
>  	__disable_ibs_on_vcpu(vcpu);
> 
> -	for (i = 0; i < online_vcpus; i++) {
> -		if (!is_vcpu_stopped(vcpu->kvm->vcpus[i])) {
> -			started_vcpus++;
> -			started_vcpu = vcpu->kvm->vcpus[i];
> -		}
> -	}
> +	vcpu->kvm->arch.started_vcpus--;
> +
> +	if (vcpu->kvm->arch.started_vcpus == 1) {
> +		struct kvm_vcpu *started_vcpu;
> 
> -	if (started_vcpus == 1) {
>  		/*
> -		 * As we only have one VCPU left, we want to enable the
> -		 * IBS facility for that VCPU to speed it up.
> +		 * As we only have one VCPU left, we want to enable the IBS
> +		 * facility for that VCPU to speed it up.
>  		 */
> -		__enable_ibs_on_vcpu(started_vcpu);
> +		kvm_for_each_vcpu(i, started_vcpu, vcpu->kvm)

here you need i.
> +			if (!is_vcpu_stopped(started_vcpu)) {
> +				__enable_ibs_on_vcpu(started_vcpu);
> +				break;
> +			}
>  	}
> 
>  	spin_unlock(&vcpu->kvm->arch.start_stop_lock);
> 

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

#1717322 — Re: [PATCH RFC v3 1/9] KVM: s390: optimize detection of started vcpus

On 21.08.2017 22:35, Radim Krčmář wrote:
> We can add a variable instead of scanning all online VCPUs to know how
> many are started.  We can't trivially tell which VCPU is the last one,
> though.

You could keep the started vcpus in a list. Then you might drop unsigned
started_vcpus;

No started vcpus: Start pointer NULL
Single started vcpu: Only one element in the list (easy to check)
> 1 started vcpus: More than one element int he list (easy to check)


> 
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  1 +
>  arch/s390/kvm/kvm-s390.c         | 39 +++++++++++++++------------------------
>  2 files changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index a409d5991934..be0d0bdf585b 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -735,6 +735,7 @@ struct kvm_arch{
>  	struct mutex ipte_mutex;
>  	struct ratelimit_state sthyi_limit;
>  	spinlock_t start_stop_lock;
> +	unsigned started_vcpus;
>  	struct sie_page2 *sie_page2;
>  	struct kvm_s390_cpu_model model;
>  	struct kvm_s390_crypto crypto;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 9f23a9e81a91..1534778a3c66 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3414,25 +3414,17 @@ static void __enable_ibs_on_vcpu(struct kvm_vcpu *vcpu)
>  
>  void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
>  {
> -	int i, online_vcpus, started_vcpus = 0;
> -
>  	if (!is_vcpu_stopped(vcpu))
>  		return;
>  
>  	trace_kvm_s390_vcpu_start_stop(vcpu->vcpu_id, 1);
>  	/* Only one cpu at a time may enter/leave the STOPPED state. */
>  	spin_lock(&vcpu->kvm->arch.start_stop_lock);
> -	online_vcpus = atomic_read(&vcpu->kvm->online_vcpus);
>  
> -	for (i = 0; i < online_vcpus; i++) {
> -		if (!is_vcpu_stopped(vcpu->kvm->vcpus[i]))
> -			started_vcpus++;
> -	}
> -
> -	if (started_vcpus == 0) {
> +	if (vcpu->kvm->arch.started_vcpus == 0) {
>  		/* we're the only active VCPU -> speed it up */
>  		__enable_ibs_on_vcpu(vcpu);
> -	} else if (started_vcpus == 1) {
> +	} else if (vcpu->kvm->arch.started_vcpus == 1) {
>  		/*
>  		 * As we are starting a second VCPU, we have to disable
>  		 * the IBS facility on all VCPUs to remove potentially
> @@ -3441,6 +3433,8 @@ void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
>  		__disable_ibs_on_all_vcpus(vcpu->kvm);
>  	}
>  
> +	vcpu->kvm->arch.started_vcpus++;
> +
>  	atomic_andnot(CPUSTAT_STOPPED, &vcpu->arch.sie_block->cpuflags);
>  	/*
>  	 * Another VCPU might have used IBS while we were offline.
> @@ -3453,16 +3447,12 @@ void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)
>  
>  void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>  {
> -	int i, online_vcpus, started_vcpus = 0;
> -	struct kvm_vcpu *started_vcpu = NULL;
> -
>  	if (is_vcpu_stopped(vcpu))
>  		return;
>  
>  	trace_kvm_s390_vcpu_start_stop(vcpu->vcpu_id, 0);
>  	/* Only one cpu at a time may enter/leave the STOPPED state. */
>  	spin_lock(&vcpu->kvm->arch.start_stop_lock);
> -	online_vcpus = atomic_read(&vcpu->kvm->online_vcpus);
>  
>  	/* SIGP STOP and SIGP STOP AND STORE STATUS has been fully processed */
>  	kvm_s390_clear_stop_irq(vcpu);
> @@ -3470,19 +3460,20 @@ void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)
>  	atomic_or(CPUSTAT_STOPPED, &vcpu->arch.sie_block->cpuflags);
>  	__disable_ibs_on_vcpu(vcpu);
>  
> -	for (i = 0; i < online_vcpus; i++) {
> -		if (!is_vcpu_stopped(vcpu->kvm->vcpus[i])) {
> -			started_vcpus++;
> -			started_vcpu = vcpu->kvm->vcpus[i];
> -		}
> -	}
> +	vcpu->kvm->arch.started_vcpus--;
> +
> +	if (vcpu->kvm->arch.started_vcpus == 1) {
> +		struct kvm_vcpu *started_vcpu;
>  
> -	if (started_vcpus == 1) {
>  		/*
> -		 * As we only have one VCPU left, we want to enable the
> -		 * IBS facility for that VCPU to speed it up.
> +		 * As we only have one VCPU left, we want to enable the IBS
> +		 * facility for that VCPU to speed it up.
>  		 */
> -		__enable_ibs_on_vcpu(started_vcpu);
> +		kvm_for_each_vcpu(i, started_vcpu, vcpu->kvm)
> +			if (!is_vcpu_stopped(started_vcpu)) {
> +				__enable_ibs_on_vcpu(started_vcpu);
> +				break;
> +			}
>  	}
>  
>  	spin_unlock(&vcpu->kvm->arch.start_stop_lock);
> 


-- 

Thanks,

David

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

#1716861 — [PATCH RFC v3 2/9] KVM: arm/arm64: fix vcpu self-detection in vgic_v3_dispatch_sgi()

The index in kvm->vcpus array and vcpu->vcpu_id are very different
things.  Comparing struct kvm_vcpu pointers is a sure way to know.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 408ef06638fc..9d4b69b766ec 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -797,7 +797,6 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
 	u16 target_cpus;
 	u64 mpidr;
 	int sgi, c;
-	int vcpu_id = vcpu->vcpu_id;
 	bool broadcast;
 
 	sgi = (reg & ICC_SGI1R_SGI_ID_MASK) >> ICC_SGI1R_SGI_ID_SHIFT;
@@ -821,7 +820,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
 			break;
 
 		/* Don't signal the calling VCPU */
-		if (broadcast && c == vcpu_id)
+		if (broadcast && c_vcpu == vcpu)
 			continue;
 
 		if (!broadcast) {
-- 
2.13.3

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

#1717327 — Re: [PATCH RFC v3 2/9] KVM: arm/arm64: fix vcpu self-detection in vgic_v3_dispatch_sgi()

On 21.08.2017 22:35, Radim Krčmář wrote:
> The index in kvm->vcpus array and vcpu->vcpu_id are very different
> things.  Comparing struct kvm_vcpu pointers is a sure way to know.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 408ef06638fc..9d4b69b766ec 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -797,7 +797,6 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>  	u16 target_cpus;
>  	u64 mpidr;
>  	int sgi, c;
> -	int vcpu_id = vcpu->vcpu_id;
>  	bool broadcast;
>  
>  	sgi = (reg & ICC_SGI1R_SGI_ID_MASK) >> ICC_SGI1R_SGI_ID_SHIFT;
> @@ -821,7 +820,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>  			break;
>  
>  		/* Don't signal the calling VCPU */
> -		if (broadcast && c == vcpu_id)
> +		if (broadcast && c_vcpu == vcpu)
>  			continue;
>  
>  		if (!broadcast) {
> 

Yes, this looks correct to me.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

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

#1716863 — [PATCH RFC v3 7/9] KVM: add kvm_free_vcpus and kvm_arch_free_vcpus

Generalize clearing of kvm->vcpus.  This should not be needed at all as
all accesses to VCPUs in the destruction path are bugs, but maybe helps
to catch them.  The call path crosses arch/common code way too much, so
extra untangling patch is welcome.  Doing the clearing later seems be
ok.

I don't see a reason for the locking the mutex, so there definitely is
a room for improvements.

Suggested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/mips/kvm/mips.c       | 13 ++-----------
 arch/powerpc/kvm/powerpc.c | 17 +++++++++--------
 arch/s390/kvm/kvm-s390.c   |  9 +--------
 arch/x86/kvm/x86.c         |  9 +--------
 include/linux/kvm_host.h   |  2 ++
 virt/kvm/arm/arm.c         | 17 +++++++++--------
 virt/kvm/kvm_main.c        | 17 +++++++++++++++++
 7 files changed, 41 insertions(+), 43 deletions(-)

diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index bce2a6431430..770c40b9df37 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/kvm/mips.c
@@ -160,7 +160,7 @@ int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-void kvm_mips_free_vcpus(struct kvm *kvm)
+void kvm_arch_free_vcpus(struct kvm *kvm)
 {
 	unsigned int i;
 	struct kvm_vcpu *vcpu;
@@ -168,15 +168,6 @@ void kvm_mips_free_vcpus(struct kvm *kvm)
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		kvm_arch_vcpu_free(vcpu);
 	}
-
-	mutex_lock(&kvm->lock);
-
-	for (i = 0; i < atomic_read(&kvm->online_vcpus); i++)
-		kvm->vcpus[i] = NULL;
-
-	atomic_set(&kvm->online_vcpus, 0);
-
-	mutex_unlock(&kvm->lock);
 }
 
 static void kvm_mips_free_gpa_pt(struct kvm *kvm)
@@ -188,7 +179,7 @@ static void kvm_mips_free_gpa_pt(struct kvm *kvm)
 
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
-	kvm_mips_free_vcpus(kvm);
+	kvm_free_vcpus(kvm);
 	kvm_mips_free_gpa_pt(kvm);
 }
 
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 3480faaf1ef8..1c563545473c 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -456,11 +456,17 @@ int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-void kvm_arch_destroy_vm(struct kvm *kvm)
+void kvm_arch_free_vcpus(struct kvm *kvm)
 {
-	unsigned int i;
+	int i;
 	struct kvm_vcpu *vcpu;
 
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		kvm_arch_vcpu_free(vcpu);
+}
+
+void kvm_arch_destroy_vm(struct kvm *kvm)
+{
 #ifdef CONFIG_KVM_XICS
 	/*
 	 * We call kick_all_cpus_sync() to ensure that all
@@ -471,14 +477,9 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 		kick_all_cpus_sync();
 #endif
 
-	kvm_for_each_vcpu(i, vcpu, kvm)
-		kvm_arch_vcpu_free(vcpu);
+	kvm_free_vcpus(kvm);
 
 	mutex_lock(&kvm->lock);
-	for (i = 0; i < atomic_read(&kvm->online_vcpus); i++)
-		kvm->vcpus[i] = NULL;
-
-	atomic_set(&kvm->online_vcpus, 0);
 
 	kvmppc_core_destroy_vm(kvm);
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 1534778a3c66..bb6278d45a25 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1941,20 +1941,13 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kmem_cache_free(kvm_vcpu_cache, vcpu);
 }
 
-static void kvm_free_vcpus(struct kvm *kvm)
+void kvm_arch_free_vcpus(struct kvm *kvm)
 {
 	unsigned int i;
 	struct kvm_vcpu *vcpu;
 
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_arch_vcpu_destroy(vcpu);
-
-	mutex_lock(&kvm->lock);
-	for (i = 0; i < atomic_read(&kvm->online_vcpus); i++)
-		kvm->vcpus[i] = NULL;
-
-	atomic_set(&kvm->online_vcpus, 0);
-	mutex_unlock(&kvm->lock);
 }
 
 void kvm_arch_destroy_vm(struct kvm *kvm)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e10eda86bc7b..d021746f1fdf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8077,7 +8077,7 @@ static void kvm_unload_vcpu_mmu(struct kvm_vcpu *vcpu)
 	vcpu_put(vcpu);
 }
 
-static void kvm_free_vcpus(struct kvm *kvm)
+void kvm_arch_free_vcpus(struct kvm *kvm)
 {
 	unsigned int i;
 	struct kvm_vcpu *vcpu;
@@ -8091,13 +8091,6 @@ static void kvm_free_vcpus(struct kvm *kvm)
 	}
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_arch_vcpu_free(vcpu);
-
-	mutex_lock(&kvm->lock);
-	for (i = 0; i < atomic_read(&kvm->online_vcpus); i++)
-		kvm->vcpus[i] = NULL;
-
-	atomic_set(&kvm->online_vcpus, 0);
-	mutex_unlock(&kvm->lock);
 }
 
 void kvm_arch_sync_events(struct kvm *kvm)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index cfb3c0efdd51..a8b9aa563834 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -874,6 +874,8 @@ static inline bool kvm_arch_intc_initialized(struct kvm *kvm)
 int kvm_arch_init_vm(struct kvm *kvm, unsigned long type);
 void kvm_arch_destroy_vm(struct kvm *kvm);
 void kvm_arch_sync_events(struct kvm *kvm);
+void kvm_free_vcpus(struct kvm *kvm);
+void kvm_arch_free_vcpus(struct kvm *kvm);
 
 int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu);
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index b9f68e4add71..d63aa1107fdb 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -166,6 +166,14 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
 	return VM_FAULT_SIGBUS;
 }
 
+void kvm_arch_free_vcpus(struct kvm *kvm)
+{
+	int i;
+	struct kvm_vcpu *vcpu;
+
+	for_each_online_vcpu(i, vcpu, kvm)
+		kvm_arch_vcpu_free(vcpu);
+}
 
 /**
  * kvm_arch_destroy_vm - destroy the VM data structure
@@ -173,17 +181,10 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
  */
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
-	int i;
-
 	free_percpu(kvm->arch.last_vcpu_ran);
 	kvm->arch.last_vcpu_ran = NULL;
 
-	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
-		if (kvm->vcpus[i]) {
-			kvm_arch_vcpu_free(kvm->vcpus[i]);
-			kvm->vcpus[i] = NULL;
-		}
-	}
+	kvm_free_vcpus(kvm);
 
 	kvm_vgic_destroy(kvm);
 }
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 33a15e176927..0d2d8b0c785c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -750,6 +750,23 @@ static void kvm_destroy_devices(struct kvm *kvm)
 	}
 }
 
+void kvm_free_vcpus(struct kvm *kvm)
+{
+	int i;
+
+	kvm_arch_free_vcpus(kvm);
+
+	mutex_lock(&kvm->lock);
+
+	i = atomic_read(&kvm->online_vcpus);
+	atomic_set(&kvm->online_vcpus, 0);
+
+	while (i--)
+		kvm->vcpus[i] = NULL;
+
+	mutex_unlock(&kvm->lock);
+}
+
 static void kvm_destroy_vm(struct kvm *kvm)
 {
 	int i;
-- 
2.13.3

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

#1717451 — Re: [PATCH RFC v3 7/9] KVM: add kvm_free_vcpus and kvm_arch_free_vcpus

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 33a15e176927..0d2d8b0c785c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -750,6 +750,23 @@ static void kvm_destroy_devices(struct kvm *kvm)
>  	}
>  }
>  
> +void kvm_free_vcpus(struct kvm *kvm)
> +{
> +	int i;
> +
> +	kvm_arch_free_vcpus(kvm);

I wonder if it would be possible to get rid of kvm_arch_free_vcpus(kvm)
completely and simply call

kvm_for_each_vcpu(i, vcpu, kvm)
	kvm_arch_vcpu_free(vcpu);

at that point.

Would certainly require some refactoring, and I am not sure if we could
modify the special mmu handling for x86 ("Unpin any mmu pages first.").
But if in doubt, that part could be moved to kvm_arch_destroy_vm(), just
before calling kvm_free_vcpus().

> +
> +	mutex_lock(&kvm->lock);
> +
> +	i = atomic_read(&kvm->online_vcpus);
> +	atomic_set(&kvm->online_vcpus, 0);

i = atomic_xchg(&kvm->online_vcpus, 0);

> +
> +	while (i--)
> +		kvm->vcpus[i] = NULL;
> +
> +	mutex_unlock(&kvm->lock);
> +}
> +
>  static void kvm_destroy_vm(struct kvm *kvm)
>  {
>  	int i;
> 


-- 

Thanks,

David

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

#1716864 — [PATCH RFC v3 4/9] KVM: arm/arm64: use locking helpers in kvm_vgic_create()

No new VCPUs can be created because we are holding the kvm->lock.
This means that if we successfuly lock all VCPUs, we'll be unlocking the
same set and there is no need to do extra bookkeeping.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 virt/kvm/arm/vgic/vgic-init.c       | 24 +++++++++---------------
 virt/kvm/arm/vgic/vgic-kvm-device.c |  6 +++++-
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 5801261f3add..feb766f74c34 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -119,7 +119,7 @@ void kvm_vgic_vcpu_early_init(struct kvm_vcpu *vcpu)
  */
 int kvm_vgic_create(struct kvm *kvm, u32 type)
 {
-	int i, vcpu_lock_idx = -1, ret;
+	int i, ret;
 	struct kvm_vcpu *vcpu;
 
 	if (irqchip_in_kernel(kvm))
@@ -140,18 +140,14 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 	 * vcpu->mutex.  By grabbing the vcpu->mutex of all VCPUs we ensure
 	 * that no other VCPUs are run while we create the vgic.
 	 */
-	ret = -EBUSY;
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		if (!mutex_trylock(&vcpu->mutex))
-			goto out_unlock;
-		vcpu_lock_idx = i;
-	}
+	if (!lock_all_vcpus(kvm))
+		return -EBUSY;
 
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		if (vcpu->arch.has_run_once)
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		if (vcpu->arch.has_run_once) {
+			ret = -EBUSY;
 			goto out_unlock;
-	}
-	ret = 0;
+		}
 
 	if (type == KVM_DEV_TYPE_ARM_VGIC_V2)
 		kvm->arch.max_vcpus = VGIC_V2_MAX_CPUS;
@@ -176,11 +172,9 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
 	kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
 
+	ret = 0;
 out_unlock:
-	for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
-		vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx);
-		mutex_unlock(&vcpu->mutex);
-	}
+	unlock_all_vcpus(kvm);
 	return ret;
 }
 
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 10ae6f394b71..c5124737c7fc 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -270,7 +270,11 @@ static void unlock_vcpus(struct kvm *kvm, int vcpu_lock_idx)
 
 void unlock_all_vcpus(struct kvm *kvm)
 {
-	unlock_vcpus(kvm, atomic_read(&kvm->online_vcpus) - 1);
+	int i;
+	struct kvm_vcpu *tmp_vcpu;
+
+	kvm_for_each_vcpu(i, tmp_vcpu, kvm)
+		mutex_unlock(&tmp_vcpu->mutex);
 }
 
 /* Returns true if all vcpus were locked, false otherwise */
-- 
2.13.3

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

#1717342 — Re: [PATCH RFC v3 4/9] KVM: arm/arm64: use locking helpers in kvm_vgic_create()

On 21.08.2017 22:35, Radim Krčmář wrote:
> No new VCPUs can be created because we are holding the kvm->lock.
> This means that if we successfuly lock all VCPUs, we'll be unlocking the
> same set and there is no need to do extra bookkeeping.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  virt/kvm/arm/vgic/vgic-init.c       | 24 +++++++++---------------
>  virt/kvm/arm/vgic/vgic-kvm-device.c |  6 +++++-
>  2 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 5801261f3add..feb766f74c34 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -119,7 +119,7 @@ void kvm_vgic_vcpu_early_init(struct kvm_vcpu *vcpu)
>   */
>  int kvm_vgic_create(struct kvm *kvm, u32 type)
>  {
> -	int i, vcpu_lock_idx = -1, ret;
> +	int i, ret;
>  	struct kvm_vcpu *vcpu;
>  
>  	if (irqchip_in_kernel(kvm))
> @@ -140,18 +140,14 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>  	 * vcpu->mutex.  By grabbing the vcpu->mutex of all VCPUs we ensure
>  	 * that no other VCPUs are run while we create the vgic.
>  	 */
> -	ret = -EBUSY;
> -	kvm_for_each_vcpu(i, vcpu, kvm) {
> -		if (!mutex_trylock(&vcpu->mutex))
> -			goto out_unlock;
> -		vcpu_lock_idx = i;
> -	}
> +	if (!lock_all_vcpus(kvm))
> +		return -EBUSY;

Yes, this makes sense.

>  
> -	kvm_for_each_vcpu(i, vcpu, kvm) {
> -		if (vcpu->arch.has_run_once)
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		if (vcpu->arch.has_run_once) {
> +			ret = -EBUSY;
>  			goto out_unlock;
> -	}
> -	ret = 0;
> +		}

somehow I prefer keeping the {}

>  
>  	if (type == KVM_DEV_TYPE_ARM_VGIC_V2)
>  		kvm->arch.max_vcpus = VGIC_V2_MAX_CPUS;
> @@ -176,11 +172,9 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>  	kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>  	kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
>  
> +	ret = 0;
>  out_unlock:
> -	for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
> -		vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx);
> -		mutex_unlock(&vcpu->mutex);
> -	}
> +	unlock_all_vcpus(kvm);
>  	return ret;
>  }
>  
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 10ae6f394b71..c5124737c7fc 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -270,7 +270,11 @@ static void unlock_vcpus(struct kvm *kvm, int vcpu_lock_idx)
>  
>  void unlock_all_vcpus(struct kvm *kvm)
>  {
> -	unlock_vcpus(kvm, atomic_read(&kvm->online_vcpus) - 1);
> +	int i;
> +	struct kvm_vcpu *tmp_vcpu;
> +
> +	kvm_for_each_vcpu(i, tmp_vcpu, kvm)
> +		mutex_unlock(&tmp_vcpu->mutex);
>  }
>  
>  /* Returns true if all vcpus were locked, false otherwise */
> 

Looks sane to me.

-- 

Thanks,

David

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

#1716869 — [PATCH RFC v3 3/9] KVM: remember position in kvm->vcpus array

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

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6882538eda32..a8ff956616d2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -220,7 +220,8 @@ struct kvm_vcpu {
 	struct preempt_notifier preempt_notifier;
 #endif
 	int cpu;
-	int vcpu_id;
+	int vcpu_id; /* id given by userspace at creation */
+	int vcpus_idx; /* index in kvm->vcpus array */
 	int srcu_idx;
 	int mode;
 	unsigned long requests;
@@ -516,13 +517,7 @@ static inline struct kvm_vcpu *kvm_get_vcpu_by_id(struct kvm *kvm, int id)
 
 static inline int kvm_vcpu_get_idx(struct kvm_vcpu *vcpu)
 {
-	struct kvm_vcpu *tmp;
-	int idx;
-
-	kvm_for_each_vcpu(idx, tmp, vcpu->kvm)
-		if (tmp == vcpu)
-			return idx;
-	BUG();
+	return vcpu->vcpus_idx;
 }
 
 #define kvm_for_each_memslot(memslot, slots)	\
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e17c40d986f3..caf8323f7df7 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2498,7 +2498,10 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 		goto unlock_vcpu_destroy;
 	}
 
-	BUG_ON(kvm->vcpus[atomic_read(&kvm->online_vcpus)]);
+	vcpu->vcpus_idx = atomic_read(&kvm->online_vcpus);
+
+	BUG_ON(kvm->vcpus[vcpu->vcpus_idx]);
+
 
 	/* Now it's all set up, let userspace reach it */
 	kvm_get_kvm(kvm);
-- 
2.13.3

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

#1717338 — Re: [PATCH RFC v3 3/9] KVM: remember position in kvm->vcpus array

On 21.08.2017 22:35, Radim Krčmář wrote:
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  include/linux/kvm_host.h | 11 +++--------
>  virt/kvm/kvm_main.c      |  5 ++++-
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 6882538eda32..a8ff956616d2 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -220,7 +220,8 @@ struct kvm_vcpu {
>  	struct preempt_notifier preempt_notifier;
>  #endif
>  	int cpu;
> -	int vcpu_id;
> +	int vcpu_id; /* id given by userspace at creation */
> +	int vcpus_idx; /* index in kvm->vcpus array */
>  	int srcu_idx;
>  	int mode;
>  	unsigned long requests;
> @@ -516,13 +517,7 @@ static inline struct kvm_vcpu *kvm_get_vcpu_by_id(struct kvm *kvm, int id)
>  
>  static inline int kvm_vcpu_get_idx(struct kvm_vcpu *vcpu)
>  {
> -	struct kvm_vcpu *tmp;
> -	int idx;
> -
> -	kvm_for_each_vcpu(idx, tmp, vcpu->kvm)
> -		if (tmp == vcpu)
> -			return idx;
> -	BUG();
> +	return vcpu->vcpus_idx;
>  }
>  
>  #define kvm_for_each_memslot(memslot, slots)	\
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e17c40d986f3..caf8323f7df7 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2498,7 +2498,10 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
>  		goto unlock_vcpu_destroy;
>  	}
>  
> -	BUG_ON(kvm->vcpus[atomic_read(&kvm->online_vcpus)]);
> +	vcpu->vcpus_idx = atomic_read(&kvm->online_vcpus);
> +
> +	BUG_ON(kvm->vcpus[vcpu->vcpus_idx]);
> +
>  
>  	/* Now it's all set up, let userspace reach it */
>  	kvm_get_kvm(kvm);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web