[PATCH v2 00/24] pic/ioapic/irqchip cleanups + minor fixes

[PATCH v2 00/24] pic/ioapic/irqchip cleanups + minor fixes

[David Hildenbrand]

My attempt to further cleanup pic/ioapic checks using irqchip_mode + other
cleanups in that area.

The goal is to only check against irqchip_mode and not to rely on
vpic/vioapic variables anymore to test for existence of pic/ioapic. This
will avoid any possible races when creating the kernel irqchip fails.

v1 -> v2:
- Changed commit message of "KVM: x86: race between KVM_SET_GSI_ROUTING and
  KVM_CREATE_IRQCHIP" to also include a hint about SPLIT IRQCHIP
- Split "KVM: x86: check against irqchip_mode in kvm_set_routing_entry()"
- return; instead of break; in "KVM: x86: simplify pic_unlock()"
- Added "KVM: x86: cleanup return handling in setup_routing_entry()"

David Hildenbrand (24):
  KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP
  KVM: x86: new irqchip mode KVM_IRQCHIP_INIT_IN_PROGRESS
  KVM: x86: check against irqchip_mode in kvm_set_routing_entry()
  KVM: x86: check against irqchip_mode in pic_in_kernel()
  KVM: x86: check against irqchip_mode in ioapic_in_kernel()
  KVM: x86: get rid of pic_irqchip()
  KVM: x86: get rid of ioapic_irqchip()
  KVM: x86: use ioapic_in_kernel() to check for ioapic existence
  KVM: x86: remove duplicate checks for ioapic
  KVM: x86: convert kvm_(set|get)_ioapic() into void
  KVM: x86: don't take kvm->irq_lock when creating IRQCHIP
  KVM: x86: push usage of slots_lock down
  KVM: x86: KVM_IRQCHIP_PIC_MASTER only has 8 pins
  KVM: x86: remove all-vcpu request from kvm_ioapic_init()
  KVM: x86: directly call kvm_make_scan_ioapic_request() in ioapic.c
  KVM: x86: rename kvm_vcpu_request_scan_ioapic()
  KVM: x86: drop goto label in kvm_set_routing_entry()
  KVM: x86: cleanup return handling in setup_routing_entry()
  KVM: x86: simplify pic_unlock()
  KVM: x86: make kvm_pic_reset() static
  KVM: x86: drop picdev_in_range()
  KVM: x86: set data directly in picdev_read()
  KVM: x86: simplify pic_ioport_read()
  KVM: x86: use irqchip_kernel() to check for pic+ioapic

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/i8259.c            | 72 ++++++++++++++++-------------------------
 arch/x86/kvm/ioapic.c           | 28 ++++++----------
 arch/x86/kvm/ioapic.h           | 14 ++------
 arch/x86/kvm/irq.c              |  2 +-
 arch/x86/kvm/irq.h              | 14 ++------
 arch/x86/kvm/irq_comm.c         | 43 +++++++++++-------------
 arch/x86/kvm/x86.c              | 41 +++++++++++------------
 include/linux/kvm_host.h        |  4 +--
 virt/kvm/eventfd.c              |  4 +--
 virt/kvm/irqchip.c              | 11 +++----
 virt/kvm/kvm_main.c             |  3 ++
 12 files changed, 93 insertions(+), 144 deletions(-)

-- 
2.9.3

[PATCH v2 01/24] KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP

[David Hildenbrand]

Avoid races between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP by taking
the kvm->lock when setting up routes.

If KVM_CREATE_IRQCHIP fails, KVM_SET_GSI_ROUTING could have already set
up routes pointing at pic/ioapic, being silently removed already.

Also, as a side effect, this patch makes sure that KVM_SET_GSI_ROUTING
and KVM_CAP_SPLIT_IRQCHIP cannot run in parallel.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 virt/kvm/kvm_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a17d787..ad0f8b2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3079,8 +3079,11 @@ static long kvm_vm_ioctl(struct file *filp,
 					   routing.nr * sizeof(*entries)))
 				goto out_free_irq_routing;
 		}
+		/* avoid races with KVM_CREATE_IRQCHIP on x86 */
+		mutex_lock(&kvm->lock);
 		r = kvm_set_irq_routing(kvm, entries, routing.nr,
 					routing.flags);
+		mutex_unlock(&kvm->lock);
 out_free_irq_routing:
 		vfree(entries);
 		break;
-- 
2.9.3

[PATCH v2 02/24] KVM: x86: new irqchip mode KVM_IRQCHIP_INIT_IN_PROGRESS

[David Hildenbrand]

Let's add a new mode and set it while we create the irqchip via
KVM_CREATE_IRQCHIP and KVM_CAP_SPLIT_IRQCHIP.

This mode will be used later to test if adding routes
(in kvm_set_routing_entry()) is already allowed.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/irq.h              | 2 +-
 arch/x86/kvm/x86.c              | 7 ++++++-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 74ef58c..164e544 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -727,6 +727,7 @@ struct kvm_hv {
 
 enum kvm_irqchip_mode {
 	KVM_IRQCHIP_NONE,
+	KVM_IRQCHIP_INIT_IN_PROGRESS, /* temporarily set during creation */
 	KVM_IRQCHIP_KERNEL,       /* created with KVM_CREATE_IRQCHIP */
 	KVM_IRQCHIP_SPLIT,        /* created with KVM_CAP_SPLIT_IRQCHIP */
 };
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 40d5b2c..7fa2480 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -103,7 +103,7 @@ static inline int irqchip_kernel(struct kvm *kvm)
 
 static inline int irqchip_in_kernel(struct kvm *kvm)
 {
-	bool ret = kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE;
+	bool ret = kvm->arch.irqchip_mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
 
 	/* Matches with wmb after initializing kvm->irq_routing. */
 	smp_rmb();
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1faf620..fa8cceb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3934,9 +3934,12 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 			goto split_irqchip_unlock;
 		if (kvm->created_vcpus)
 			goto split_irqchip_unlock;
+		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
 		r = kvm_setup_empty_irq_routing(kvm);
-		if (r)
+		if (r) {
+			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
 			goto split_irqchip_unlock;
+		}
 		/* Pairs with irqchip_in_kernel. */
 		smp_wmb();
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
@@ -4024,8 +4027,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto create_irqchip_unlock;
 		}
 
+		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
+			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
 			mutex_lock(&kvm->slots_lock);
 			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
-- 
2.9.3

[PATCH v2 03/24] KVM: x86: check against irqchip_mode in kvm_set_routing_entry()

[David Hildenbrand]

Let's replace the checks for pic_in_kernel() and ioapic_in_kernel() by
checks against irqchip_mode.

Also make sure that creation of any route is only possible if we have
an lapic in kernel (irqchip_in_kernel()) or if we are currently
inititalizing the irqchip.

This is necessary to switch pic_in_kernel() and ioapic_in_kernel() to
irqchip_mode, too.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/irq_comm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 6825cd3..0a026f8 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -282,24 +282,24 @@ int kvm_set_routing_entry(struct kvm *kvm,
 	int delta;
 	unsigned max_pin;
 
+	/* also allow creation of routes during KVM_IRQCHIP_INIT_IN_PROGRESS */
+	if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
+		goto out;
+
 	switch (ue->type) {
 	case KVM_IRQ_ROUTING_IRQCHIP:
+		if (irqchip_split(kvm))
+			goto out;
 		delta = 0;
 		switch (ue->u.irqchip.irqchip) {
 		case KVM_IRQCHIP_PIC_SLAVE:
 			delta = 8;
 			/* fall through */
 		case KVM_IRQCHIP_PIC_MASTER:
-			if (!pic_in_kernel(kvm))
-				goto out;
-
 			e->set = kvm_set_pic_irq;
 			max_pin = PIC_NUM_PINS;
 			break;
 		case KVM_IRQCHIP_IOAPIC:
-			if (!ioapic_in_kernel(kvm))
-				goto out;
-
 			max_pin = KVM_IOAPIC_NUM_PINS;
 			e->set = kvm_set_ioapic_irq;
 			break;
-- 
2.9.3

[PATCH v2 04/24] KVM: x86: check against irqchip_mode in pic_in_kernel()

[David Hildenbrand]

Let's avoid checking against kvm->arch.vpic. We have kvm->arch.irqchip_mode
for that now.

KVM_IRQCHIP_KERNEL implies a fully inititalized pic, while kvm->arch.vpic
might temporarily be set but invalidated again if e.g. kvm_ioapic_init()
fails when setting KVM_CREATE_IRQCHIP. Although current users seem to be
fine, this avoids future bugs.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/irq.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 7fa2480..184306e 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -85,10 +85,7 @@ static inline struct kvm_pic *pic_irqchip(struct kvm *kvm)
 
 static inline int pic_in_kernel(struct kvm *kvm)
 {
-	int ret;
-
-	ret = (pic_irqchip(kvm) != NULL);
-	return ret;
+	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
 }
 
 static inline int irqchip_split(struct kvm *kvm)
-- 
2.9.3

[PATCH v2 05/24] KVM: x86: check against irqchip_mode in ioapic_in_kernel()

[David Hildenbrand]

KVM_IRQCHIP_KERNEL implies a fully inititalized ioapic, while
kvm->arch.vioapic might temporarily be set but invalidated again if e.g.
setting of default routing fails when setting KVM_CREATE_IRQCHIP.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/ioapic.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 1cc6e54..9a62fe1 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -112,10 +112,7 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
 
 static inline int ioapic_in_kernel(struct kvm *kvm)
 {
-	int ret;
-
-	ret = (ioapic_irqchip(kvm) != NULL);
-	return ret;
+	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
 }
 
 void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
-- 
2.9.3

[PATCH v2 06/24] KVM: x86: get rid of pic_irqchip()

[David Hildenbrand]

It seemed like a nice idea to encapsulate access to kvm->arch.vpic. But
as the usage is already mixed, internal locks are taken outside of i8259.c
and grepping for "vpic" only is much easier, let's just get rid of
pic_irqchip().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/i8259.c    |  4 ++--
 arch/x86/kvm/irq.c      |  2 +-
 arch/x86/kvm/irq.h      |  5 -----
 arch/x86/kvm/irq_comm.c |  4 ++--
 arch/x86/kvm/x86.c      | 24 +++++++++++-------------
 5 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 73ea24d..64e4b92 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -239,7 +239,7 @@ static inline void pic_intack(struct kvm_kpic_state *s, int irq)
 int kvm_pic_read_irq(struct kvm *kvm)
 {
 	int irq, irq2, intno;
-	struct kvm_pic *s = pic_irqchip(kvm);
+	struct kvm_pic *s = kvm->arch.vpic;
 
 	s->output = 0;
 
@@ -576,7 +576,7 @@ static int picdev_eclr_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
  */
 static void pic_irq_request(struct kvm *kvm, int level)
 {
-	struct kvm_pic *s = pic_irqchip(kvm);
+	struct kvm_pic *s = kvm->arch.vpic;
 
 	if (!s->output)
 		s->wakeup_needed = true;
diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 60d91c9..5c24811 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -60,7 +60,7 @@ static int kvm_cpu_has_extint(struct kvm_vcpu *v)
 		if (irqchip_split(v->kvm))
 			return pending_userspace_extint(v);
 		else
-			return pic_irqchip(v->kvm)->output;
+			return v->kvm->arch.vpic->output;
 	} else
 		return 0;
 }
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 184306e..2455f16 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -78,11 +78,6 @@ void kvm_pic_destroy(struct kvm *kvm);
 int kvm_pic_read_irq(struct kvm *kvm);
 void kvm_pic_update_irq(struct kvm_pic *s);
 
-static inline struct kvm_pic *pic_irqchip(struct kvm *kvm)
-{
-	return kvm->arch.vpic;
-}
-
 static inline int pic_in_kernel(struct kvm *kvm)
 {
 	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 0a026f8..45ab43e 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -42,7 +42,7 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 			   struct kvm *kvm, int irq_source_id, int level,
 			   bool line_status)
 {
-	struct kvm_pic *pic = pic_irqchip(kvm);
+	struct kvm_pic *pic = kvm->arch.vpic;
 	return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
 }
 
@@ -236,7 +236,7 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
 		goto unlock;
 
 	kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
-	kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
+	kvm_pic_clear_all(kvm->arch.vpic, irq_source_id);
 unlock:
 	mutex_unlock(&kvm->irq_lock);
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fa8cceb..079bf6b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3721,18 +3721,17 @@ static int kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
 
 static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 {
+	struct kvm_pic *pic = kvm->arch.vpic;
 	int r;
 
 	r = 0;
 	switch (chip->chip_id) {
 	case KVM_IRQCHIP_PIC_MASTER:
-		memcpy(&chip->chip.pic,
-			&pic_irqchip(kvm)->pics[0],
+		memcpy(&chip->chip.pic, &pic->pics[0],
 			sizeof(struct kvm_pic_state));
 		break;
 	case KVM_IRQCHIP_PIC_SLAVE:
-		memcpy(&chip->chip.pic,
-			&pic_irqchip(kvm)->pics[1],
+		memcpy(&chip->chip.pic, &pic->pics[1],
 			sizeof(struct kvm_pic_state));
 		break;
 	case KVM_IRQCHIP_IOAPIC:
@@ -3747,23 +3746,22 @@ static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 
 static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 {
+	struct kvm_pic *pic = kvm->arch.vpic;
 	int r;
 
 	r = 0;
 	switch (chip->chip_id) {
 	case KVM_IRQCHIP_PIC_MASTER:
-		spin_lock(&pic_irqchip(kvm)->lock);
-		memcpy(&pic_irqchip(kvm)->pics[0],
-			&chip->chip.pic,
+		spin_lock(&pic->lock);
+		memcpy(&pic->pics[0], &chip->chip.pic,
 			sizeof(struct kvm_pic_state));
-		spin_unlock(&pic_irqchip(kvm)->lock);
+		spin_unlock(&pic->lock);
 		break;
 	case KVM_IRQCHIP_PIC_SLAVE:
-		spin_lock(&pic_irqchip(kvm)->lock);
-		memcpy(&pic_irqchip(kvm)->pics[1],
-			&chip->chip.pic,
+		spin_lock(&pic->lock);
+		memcpy(&pic->pics[1], &chip->chip.pic,
 			sizeof(struct kvm_pic_state));
-		spin_unlock(&pic_irqchip(kvm)->lock);
+		spin_unlock(&pic->lock);
 		break;
 	case KVM_IRQCHIP_IOAPIC:
 		r = kvm_set_ioapic(kvm, &chip->chip.ioapic);
@@ -3772,7 +3770,7 @@ static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 		r = -EINVAL;
 		break;
 	}
-	kvm_pic_update_irq(pic_irqchip(kvm));
+	kvm_pic_update_irq(pic);
 	return r;
 }
 
-- 
2.9.3

[PATCH v2 07/24] KVM: x86: get rid of ioapic_irqchip()

[David Hildenbrand]

Let's just use kvm->arch.vioapic directly.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/ioapic.c | 4 ++--
 arch/x86/kvm/ioapic.h | 5 -----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 6e219e5..3d9dc98 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -643,7 +643,7 @@ void kvm_ioapic_destroy(struct kvm *kvm)
 
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
-	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 	if (!ioapic)
 		return -EINVAL;
 
@@ -656,7 +656,7 @@ int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
-	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 	if (!ioapic)
 		return -EINVAL;
 
diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 9a62fe1..717d98a 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -105,11 +105,6 @@ do {									\
 #define ASSERT(x) do { } while (0)
 #endif
 
-static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
-{
-	return kvm->arch.vioapic;
-}
-
 static inline int ioapic_in_kernel(struct kvm *kvm)
 {
 	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
-- 
2.9.3

[PATCH v2 08/24] KVM: x86: use ioapic_in_kernel() to check for ioapic existence

[David Hildenbrand]

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/ioapic.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 3d9dc98..4fd234d 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -268,9 +268,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)
 
 void kvm_vcpu_request_scan_ioapic(struct kvm *kvm)
 {
-	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
-
-	if (!ioapic)
+	if (!ioapic_in_kernel(kvm))
 		return;
 	kvm_make_scan_ioapic_request(kvm);
 }
-- 
2.9.3

[PATCH v2 09/24] KVM: x86: remove duplicate checks for ioapic

[David Hildenbrand]

When handling KVM_GET_IRQCHIP, we already check irqchip_kernel(), which
implies a fully inititalized ioapic.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/ioapic.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 4fd234d..9b2a7f0 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -642,8 +642,6 @@ void kvm_ioapic_destroy(struct kvm *kvm)
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
-	if (!ioapic)
-		return -EINVAL;
 
 	spin_lock(&ioapic->lock);
 	memcpy(state, ioapic, sizeof(struct kvm_ioapic_state));
@@ -655,8 +653,6 @@ int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
-	if (!ioapic)
-		return -EINVAL;
 
 	spin_lock(&ioapic->lock);
 	memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
-- 
2.9.3

[PATCH v2 10/24] KVM: x86: convert kvm_(set|get)_ioapic() into void

[David Hildenbrand]

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/ioapic.c | 6 ++----
 arch/x86/kvm/ioapic.h | 4 ++--
 arch/x86/kvm/x86.c    | 4 ++--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 9b2a7f0..427e5f9 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -639,7 +639,7 @@ void kvm_ioapic_destroy(struct kvm *kvm)
 	kfree(ioapic);
 }
 
-int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
+void kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 
@@ -647,10 +647,9 @@ int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 	memcpy(state, ioapic, sizeof(struct kvm_ioapic_state));
 	state->irr &= ~ioapic->irr_delivered;
 	spin_unlock(&ioapic->lock);
-	return 0;
 }
 
-int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
+void kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 
@@ -661,5 +660,4 @@ int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 	kvm_vcpu_request_scan_ioapic(kvm);
 	kvm_ioapic_inject_all(ioapic, state->irr);
 	spin_unlock(&ioapic->lock);
-	return 0;
 }
diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 717d98a..3eb140b 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -124,8 +124,8 @@ void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 			     struct kvm_lapic_irq *irq,
 			     struct dest_map *dest_map);
-int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
-int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
+void kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
+void kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
 			   ulong *ioapic_handled_vectors);
 void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 079bf6b..1636da0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3735,7 +3735,7 @@ static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 			sizeof(struct kvm_pic_state));
 		break;
 	case KVM_IRQCHIP_IOAPIC:
-		r = kvm_get_ioapic(kvm, &chip->chip.ioapic);
+		kvm_get_ioapic(kvm, &chip->chip.ioapic);
 		break;
 	default:
 		r = -EINVAL;
@@ -3764,7 +3764,7 @@ static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 		spin_unlock(&pic->lock);
 		break;
 	case KVM_IRQCHIP_IOAPIC:
-		r = kvm_set_ioapic(kvm, &chip->chip.ioapic);
+		kvm_set_ioapic(kvm, &chip->chip.ioapic);
 		break;
 	default:
 		r = -EINVAL;
-- 
2.9.3

[PATCH v2 11/24] KVM: x86: don't take kvm->irq_lock when creating IRQCHIP

[David Hildenbrand]

I don't see any reason any more for this lock, seemed to be used to protect
removal of kvm->arch.vpic / kvm->arch.vioapic when already partially
inititalized, now access is properly protected using kvm->arch.irqchip_mode
and this shouldn't be necessary anymore.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/x86.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1636da0..db1a5ff 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4030,10 +4030,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (r) {
 			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
 			mutex_lock(&kvm->slots_lock);
-			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
 			kvm_pic_destroy(kvm);
-			mutex_unlock(&kvm->irq_lock);
 			mutex_unlock(&kvm->slots_lock);
 			goto create_irqchip_unlock;
 		}
-- 
2.9.3

Re: [PATCH v2 11/24] KVM: x86: don't take kvm->irq_lock when creating IRQCHIP

[Paolo Bonzini]

On 23/03/2017 15:13, David Hildenbrand wrote:
> I don't see any reason any more for this lock, seemed to be used to protect
> removal of kvm->arch.vpic / kvm->arch.vioapic when already partially
> inititalized, now access is properly protected using kvm->arch.irqchip_mode
> and this shouldn't be necessary anymore.

Do more readers of irqchip_mode need memory barriers now?  Definitely we need an smp_wmb() after setting INIT_IN_PROGRESS.

Something like this:

diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 3eb140ba57b6..b1df4d53fad1 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -107,7 +107,9 @@ struct kvm_ioapic {
 
 static inline int ioapic_in_kernel(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
+	int mode = kvm->arch.irqchip_mode; 
+	smp_rmb();
+	return mode == KVM_IRQCHIP_KERNEL;
 }
 
 void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 2455f16ee98a..6f7b3fb92dd4 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -80,26 +80,30 @@ struct kvm_pic {
 
 static inline int pic_in_kernel(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
+	int mode = kvm->arch.irqchip_mode; 
+	smp_rmb();
+	return mode == KVM_IRQCHIP_KERNEL;
 }
 
 static inline int irqchip_split(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
+	int mode = kvm->arch.irqchip_mode; 
+	smp_rmb();
+	return mode == KVM_IRQCHIP_SPLIT;
 }
 
 static inline int irqchip_kernel(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
+	int mode = kvm->arch.irqchip_mode; 
+	smp_rmb();
+	return mode == KVM_IRQCHIP_KERNEL;
 }
 
 static inline int irqchip_in_kernel(struct kvm *kvm)
 {
-	bool ret = kvm->arch.irqchip_mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
-
-	/* Matches with wmb after initializing kvm->irq_routing. */
+	int mode = kvm->arch.irqchip_mode; 
 	smp_rmb();
-	return ret;
+	return mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
 }
 
 void kvm_pic_reset(struct kvm_kpic_state *s);
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 45ab43e5fbc5..e8306eeb5613 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -286,6 +286,7 @@ int kvm_set_routing_entry(struct kvm *kvm,
 	if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
 		goto out;
 
+	smp_rmb();
 	switch (ue->type) {
 	case KVM_IRQ_ROUTING_IRQCHIP:
 		if (irqchip_split(kvm))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5dca5aed9c93..ed9143f305f9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3933,6 +3933,8 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		if (kvm->created_vcpus)
 			goto split_irqchip_unlock;
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
+		/* Pairs with pic_in_kernel/ioapic_in_kernel/etc. */
+		smp_wmb();
 		r = kvm_setup_empty_irq_routing(kvm);
 		if (r) {
 			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
@@ -4026,6 +4028,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
+		/* Pairs with pic_in_kernel/ioapic_in_kernel/etc. */
+		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
 			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;

but with each part properly squashed into the right patch.

Paolo

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1636da0..db1a5ff 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4030,10 +4030,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		if (r) {
>  			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
>  			mutex_lock(&kvm->slots_lock);
> -			mutex_lock(&kvm->irq_lock);
>  			kvm_ioapic_destroy(kvm);
>  			kvm_pic_destroy(kvm);
> -			mutex_unlock(&kvm->irq_lock);
>  			mutex_unlock(&kvm->slots_lock);
>  			goto create_irqchip_unlock;
>  		}
> 

Re: [PATCH v2 11/24] KVM: x86: don't take kvm->irq_lock when creating IRQCHIP

[David Hildenbrand]

On 28.03.2017 10:28, Paolo Bonzini wrote:
> 
> 
> On 23/03/2017 15:13, David Hildenbrand wrote:
>> I don't see any reason any more for this lock, seemed to be used to protect
>> removal of kvm->arch.vpic / kvm->arch.vioapic when already partially
>> inititalized, now access is properly protected using kvm->arch.irqchip_mode
>> and this shouldn't be necessary anymore.
> 
> Do more readers of irqchip_mode need memory barriers now?  Definitely we need an smp_wmb() after setting INIT_IN_PROGRESS.
> 

My assumption was that if vpic/vioapic checks worked until now without
memory barriers, the same should be true when checking against
irqchip_mode. But as we are dropping some implicit barriers and
irqchip_mode is updated more frequently, you may be right.

Also, using rmb/wmb in a uniform fashion with irqchip_mode looks
cleaner, as we are removing all implicit "cannot happen because of XXX"
special cases.

> Something like this:
> 
> diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> index 3eb140ba57b6..b1df4d53fad1 100644
> --- a/arch/x86/kvm/ioapic.h
> +++ b/arch/x86/kvm/ioapic.h
> @@ -107,7 +107,9 @@ struct kvm_ioapic {
>  
>  static inline int ioapic_in_kernel(struct kvm *kvm)
>  {
> -	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
> +	int mode = kvm->arch.irqchip_mode; 
> +	smp_rmb();
> +	return mode == KVM_IRQCHIP_KERNEL;
>  }
>  
>  void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
> index 2455f16ee98a..6f7b3fb92dd4 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -80,26 +80,30 @@ struct kvm_pic {
>  
>  static inline int pic_in_kernel(struct kvm *kvm)
>  {
> -	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
> +	int mode = kvm->arch.irqchip_mode; 
> +	smp_rmb();
> +	return mode == KVM_IRQCHIP_KERNEL;
>  }
>  
>  static inline int irqchip_split(struct kvm *kvm)
>  {
> -	return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
> +	int mode = kvm->arch.irqchip_mode; 
> +	smp_rmb();
> +	return mode == KVM_IRQCHIP_SPLIT;
>  }
>  
>  static inline int irqchip_kernel(struct kvm *kvm)
>  {
> -	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
> +	int mode = kvm->arch.irqchip_mode; 
> +	smp_rmb();
> +	return mode == KVM_IRQCHIP_KERNEL;
>  }
>  
>  static inline int irqchip_in_kernel(struct kvm *kvm)
>  {
> -	bool ret = kvm->arch.irqchip_mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
> -
> -	/* Matches with wmb after initializing kvm->irq_routing. */
> +	int mode = kvm->arch.irqchip_mode; 
>  	smp_rmb();
> -	return ret;
> +	return mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
>  }
>  
>  void kvm_pic_reset(struct kvm_kpic_state *s);
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 45ab43e5fbc5..e8306eeb5613 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -286,6 +286,7 @@ int kvm_set_routing_entry(struct kvm *kvm,
>  	if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
>  		goto out;
>  
> +	smp_rmb();
>  	switch (ue->type) {
>  	case KVM_IRQ_ROUTING_IRQCHIP:
>  		if (irqchip_split(kvm))
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5dca5aed9c93..ed9143f305f9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3933,6 +3933,8 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		if (kvm->created_vcpus)
>  			goto split_irqchip_unlock;
>  		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
> +		/* Pairs with pic_in_kernel/ioapic_in_kernel/etc. */
> +		smp_wmb();
>  		r = kvm_setup_empty_irq_routing(kvm);
>  		if (r) {
>  			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
> @@ -4026,6 +4028,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		}
>  
>  		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
> +		/* Pairs with pic_in_kernel/ioapic_in_kernel/etc. */
> +		smp_wmb();
>  		r = kvm_setup_default_irq_routing(kvm);
>  		if (r) {
>  			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
> 
> but with each part properly squashed into the right patch.

Sure, thanks!

> 
> Paolo

-- 

Thanks,

David

Re: [PATCH v2 11/24] KVM: x86: don't take kvm->irq_lock when creating IRQCHIP

[Paolo Bonzini]

On 03/04/2017 09:27, David Hildenbrand wrote:
>>> I don't see any reason any more for this lock, seemed to be used to protect
>>> removal of kvm->arch.vpic / kvm->arch.vioapic when already partially
>>> inititalized, now access is properly protected using kvm->arch.irqchip_mode
>>> and this shouldn't be necessary anymore.
>> Do more readers of irqchip_mode need memory barriers now?  Definitely we need an smp_wmb() after setting INIT_IN_PROGRESS.
>>
> My assumption was that if vpic/vioapic checks worked until now without
> memory barriers, the same should be true when checking against
> irqchip_mode.

All that was missing previously was smp_read_barrier_depends().  While a
compiler _can_ do things that are prohibited by
smp_read_barrier_depends(), it's much harder to trigger than for
smp_rmb().  For smp_rmb(), the scheduler's whims may cause it to break
the intended semantics.

Paolo

> But as we are dropping some implicit barriers and
> irqchip_mode is updated more frequently, you may be right.
> 
> Also, using rmb/wmb in a uniform fashion with irqchip_mode looks
> cleaner, as we are removing all implicit "cannot happen because of XXX"
> special cases.

[PATCH v2 12/24] KVM: x86: push usage of slots_lock down

[David Hildenbrand]

Let's just move it to the place where it is actually needed.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/i8259.c  | 2 ++
 arch/x86/kvm/ioapic.c | 2 ++
 arch/x86/kvm/x86.c    | 4 ----
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 64e4b92..e77e8c3 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -657,9 +657,11 @@ void kvm_pic_destroy(struct kvm *kvm)
 {
 	struct kvm_pic *vpic = kvm->arch.vpic;
 
+	mutex_lock(&kvm->slots_lock);
 	kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_master);
 	kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_slave);
 	kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_eclr);
+	mutex_unlock(&kvm->slots_lock);
 
 	kvm->arch.vpic = NULL;
 	kfree(vpic);
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 427e5f9..1a30320 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -634,7 +634,9 @@ void kvm_ioapic_destroy(struct kvm *kvm)
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 
 	cancel_delayed_work_sync(&ioapic->eoi_inject);
+	mutex_lock(&kvm->slots_lock);
 	kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
+	mutex_unlock(&kvm->slots_lock);
 	kvm->arch.vioapic = NULL;
 	kfree(ioapic);
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index db1a5ff..8d6418e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4019,9 +4019,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 
 		r = kvm_ioapic_init(kvm);
 		if (r) {
-			mutex_lock(&kvm->slots_lock);
 			kvm_pic_destroy(kvm);
-			mutex_unlock(&kvm->slots_lock);
 			goto create_irqchip_unlock;
 		}
 
@@ -4029,10 +4027,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
 			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
-			mutex_lock(&kvm->slots_lock);
 			kvm_ioapic_destroy(kvm);
 			kvm_pic_destroy(kvm);
-			mutex_unlock(&kvm->slots_lock);
 			goto create_irqchip_unlock;
 		}
 		/* Write kvm->irq_routing before enabling irqchip_in_kernel. */
-- 
2.9.3

[PATCH v2 13/24] KVM: x86: KVM_IRQCHIP_PIC_MASTER only has 8 pins

[David Hildenbrand]

Currently, one could set pin 8-15, implicitly referring to
KVM_IRQCHIP_PIC_SLAVE.

Get rid of the two local variables max_pin and delta on the way.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/irq_comm.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 45ab43e..34acbec 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -279,8 +279,6 @@ int kvm_set_routing_entry(struct kvm *kvm,
 			  const struct kvm_irq_routing_entry *ue)
 {
 	int r = -EINVAL;
-	int delta;
-	unsigned max_pin;
 
 	/* also allow creation of routes during KVM_IRQCHIP_INIT_IN_PROGRESS */
 	if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
@@ -290,26 +288,25 @@ int kvm_set_routing_entry(struct kvm *kvm,
 	case KVM_IRQ_ROUTING_IRQCHIP:
 		if (irqchip_split(kvm))
 			goto out;
-		delta = 0;
+		e->irqchip.pin = ue->u.irqchip.pin;
 		switch (ue->u.irqchip.irqchip) {
 		case KVM_IRQCHIP_PIC_SLAVE:
-			delta = 8;
+			e->irqchip.pin += PIC_NUM_PINS / 2;
 			/* fall through */
 		case KVM_IRQCHIP_PIC_MASTER:
+			if (ue->u.irqchip.pin >= PIC_NUM_PINS / 2)
+				goto out;
 			e->set = kvm_set_pic_irq;
-			max_pin = PIC_NUM_PINS;
 			break;
 		case KVM_IRQCHIP_IOAPIC:
-			max_pin = KVM_IOAPIC_NUM_PINS;
+			if (ue->u.irqchip.pin >= KVM_IOAPIC_NUM_PINS)
+				goto out;
 			e->set = kvm_set_ioapic_irq;
 			break;
 		default:
 			goto out;
 		}
 		e->irqchip.irqchip = ue->u.irqchip.irqchip;
-		e->irqchip.pin = ue->u.irqchip.pin + delta;
-		if (e->irqchip.pin >= max_pin)
-			goto out;
 		break;
 	case KVM_IRQ_ROUTING_MSI:
 		e->set = kvm_set_msi;
-- 
2.9.3

[PATCH v2 14/24] KVM: x86: remove all-vcpu request from kvm_ioapic_init()

[David Hildenbrand]

kvm_ioapic_init() is guaranteed to be called without any created VCPUs,
so doing an all-vcpu request results in a NOP.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/ioapic.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 1a30320..0eb7847 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -622,10 +622,8 @@ int kvm_ioapic_init(struct kvm *kvm)
 	if (ret < 0) {
 		kvm->arch.vioapic = NULL;
 		kfree(ioapic);
-		return ret;
 	}
 
-	kvm_vcpu_request_scan_ioapic(kvm);
 	return ret;
 }
 
-- 
2.9.3

[PATCH v2 15/24] KVM: x86: directly call kvm_make_scan_ioapic_request() in ioapic.c

[David Hildenbrand]

We know there is an ioapic, so let's call it directly.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/ioapic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 0eb7847..d1c30c0 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -313,7 +313,7 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 		if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG
 		    && ioapic->irr & (1 << index))
 			ioapic_service(ioapic, index, false);
-		kvm_vcpu_request_scan_ioapic(ioapic->kvm);
+		kvm_make_scan_ioapic_request(ioapic->kvm);
 		break;
 	}
 }
@@ -657,7 +657,7 @@ void kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 	memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
 	ioapic->irr = 0;
 	ioapic->irr_delivered = 0;
-	kvm_vcpu_request_scan_ioapic(kvm);
+	kvm_make_scan_ioapic_request(kvm);
 	kvm_ioapic_inject_all(ioapic, state->irr);
 	spin_unlock(&ioapic->lock);
 }
-- 
2.9.3

[PATCH v2 16/24] KVM: x86: rename kvm_vcpu_request_scan_ioapic()

[David Hildenbrand]

Let's rename it into a proper arch specific callback.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/ioapic.c    | 2 +-
 include/linux/kvm_host.h | 4 ++--
 virt/kvm/eventfd.c       | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index d1c30c0..58c7993 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -266,7 +266,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)
 	spin_unlock(&ioapic->lock);
 }
 
-void kvm_vcpu_request_scan_ioapic(struct kvm *kvm)
+void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
 {
 	if (!ioapic_in_kernel(kvm))
 		return;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 2c14ad9..7aa2f9b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -502,10 +502,10 @@ int __must_check vcpu_load(struct kvm_vcpu *vcpu);
 void vcpu_put(struct kvm_vcpu *vcpu);
 
 #ifdef __KVM_HAVE_IOAPIC
-void kvm_vcpu_request_scan_ioapic(struct kvm *kvm);
+void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
 void kvm_arch_post_irq_routing_update(struct kvm *kvm);
 #else
-static inline void kvm_vcpu_request_scan_ioapic(struct kvm *kvm)
+static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
 {
 }
 static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index a29786d..c2fbecb 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -490,7 +490,7 @@ void kvm_register_irq_ack_notifier(struct kvm *kvm,
 	mutex_lock(&kvm->irq_lock);
 	hlist_add_head_rcu(&kian->link, &kvm->irq_ack_notifier_list);
 	mutex_unlock(&kvm->irq_lock);
-	kvm_vcpu_request_scan_ioapic(kvm);
+	kvm_arch_post_irq_ack_notifier_list_update(kvm);
 }
 
 void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
@@ -500,7 +500,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
 	hlist_del_init_rcu(&kian->link);
 	mutex_unlock(&kvm->irq_lock);
 	synchronize_srcu(&kvm->irq_srcu);
-	kvm_vcpu_request_scan_ioapic(kvm);
+	kvm_arch_post_irq_ack_notifier_list_update(kvm);
 }
 #endif
 
-- 
2.9.3

[PATCH v2 17/24] KVM: x86: drop goto label in kvm_set_routing_entry()

[David Hildenbrand]

No need for the goto label + local variable "r".

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/irq_comm.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 34acbec..1728ff1 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -278,16 +278,14 @@ int kvm_set_routing_entry(struct kvm *kvm,
 			  struct kvm_kernel_irq_routing_entry *e,
 			  const struct kvm_irq_routing_entry *ue)
 {
-	int r = -EINVAL;
-
 	/* also allow creation of routes during KVM_IRQCHIP_INIT_IN_PROGRESS */
 	if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
-		goto out;
+		return -EINVAL;
 
 	switch (ue->type) {
 	case KVM_IRQ_ROUTING_IRQCHIP:
 		if (irqchip_split(kvm))
-			goto out;
+			return -EINVAL;
 		e->irqchip.pin = ue->u.irqchip.pin;
 		switch (ue->u.irqchip.irqchip) {
 		case KVM_IRQCHIP_PIC_SLAVE:
@@ -295,16 +293,16 @@ int kvm_set_routing_entry(struct kvm *kvm,
 			/* fall through */
 		case KVM_IRQCHIP_PIC_MASTER:
 			if (ue->u.irqchip.pin >= PIC_NUM_PINS / 2)
-				goto out;
+				return -EINVAL;
 			e->set = kvm_set_pic_irq;
 			break;
 		case KVM_IRQCHIP_IOAPIC:
 			if (ue->u.irqchip.pin >= KVM_IOAPIC_NUM_PINS)
-				goto out;
+				return -EINVAL;
 			e->set = kvm_set_ioapic_irq;
 			break;
 		default:
-			goto out;
+			return -EINVAL;
 		}
 		e->irqchip.irqchip = ue->u.irqchip.irqchip;
 		break;
@@ -315,7 +313,7 @@ int kvm_set_routing_entry(struct kvm *kvm,
 		e->msi.data = ue->u.msi.data;
 
 		if (kvm_msi_route_invalid(kvm, e))
-			goto out;
+			return -EINVAL;
 		break;
 	case KVM_IRQ_ROUTING_HV_SINT:
 		e->set = kvm_hv_set_sint;
@@ -323,12 +321,10 @@ int kvm_set_routing_entry(struct kvm *kvm,
 		e->hv_sint.sint = ue->u.hv_sint.sint;
 		break;
 	default:
-		goto out;
+		return -EINVAL;
 	}
 
-	r = 0;
-out:
-	return r;
+	return 0;
 }
 
 bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
-- 
2.9.3

[PATCH v2 18/24] KVM: x86: cleanup return handling in setup_routing_entry()

[David Hildenbrand]

Let's drop the goto and return the error value directly.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 virt/kvm/irqchip.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 3bcc999..cc30d01 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -142,8 +142,8 @@ static int setup_routing_entry(struct kvm *kvm,
 			       struct kvm_kernel_irq_routing_entry *e,
 			       const struct kvm_irq_routing_entry *ue)
 {
-	int r = -EINVAL;
 	struct kvm_kernel_irq_routing_entry *ei;
+	int r;
 
 	/*
 	 * Do not allow GSI to be mapped to the same irqchip more than once.
@@ -153,20 +153,19 @@ static int setup_routing_entry(struct kvm *kvm,
 		if (ei->type != KVM_IRQ_ROUTING_IRQCHIP ||
 		    ue->type != KVM_IRQ_ROUTING_IRQCHIP ||
 		    ue->u.irqchip.irqchip == ei->irqchip.irqchip)
-			return r;
+			return -EINVAL;
 
 	e->gsi = ue->gsi;
 	e->type = ue->type;
 	r = kvm_set_routing_entry(kvm, e, ue);
 	if (r)
-		goto out;
+		return r;
 	if (e->type == KVM_IRQ_ROUTING_IRQCHIP)
 		rt->chip[e->irqchip.irqchip][e->irqchip.pin] = e->gsi;
 
 	hlist_add_head(&e->link, &rt->map[e->gsi]);
-	r = 0;
-out:
-	return r;
+
+	return 0;
 }
 
 void __attribute__((weak)) kvm_arch_irq_routing_update(struct kvm *kvm)
-- 
2.9.3

[PATCH v2 19/24] KVM: x86: simplify pic_unlock()

[David Hildenbrand]

We can easily compact this code and get rid of one local variable.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/i8259.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index e77e8c3..34fa982 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -49,7 +49,7 @@ static void pic_unlock(struct kvm_pic *s)
 	__releases(&s->lock)
 {
 	bool wakeup = s->wakeup_needed;
-	struct kvm_vcpu *vcpu, *found = NULL;
+	struct kvm_vcpu *vcpu;
 	int i;
 
 	s->wakeup_needed = false;
@@ -59,16 +59,11 @@ static void pic_unlock(struct kvm_pic *s)
 	if (wakeup) {
 		kvm_for_each_vcpu(i, vcpu, s->kvm) {
 			if (kvm_apic_accept_pic_intr(vcpu)) {
-				found = vcpu;
-				break;
+				kvm_make_request(KVM_REQ_EVENT, vcpu);
+				kvm_vcpu_kick(vcpu);
+				return;
 			}
 		}
-
-		if (!found)
-			return;
-
-		kvm_make_request(KVM_REQ_EVENT, found);
-		kvm_vcpu_kick(found);
 	}
 }
 
-- 
2.9.3

[PATCH v2 20/24] KVM: x86: make kvm_pic_reset() static

[David Hildenbrand]

Not used outside of i8259.c, so let's make it static.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/i8259.c | 2 +-
 arch/x86/kvm/irq.h   | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 34fa982..a871e08 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -268,7 +268,7 @@ int kvm_pic_read_irq(struct kvm *kvm)
 	return intno;
 }
 
-void kvm_pic_reset(struct kvm_kpic_state *s)
+static void kvm_pic_reset(struct kvm_kpic_state *s)
 {
 	int irq, i;
 	struct kvm_vcpu *vcpu;
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 2455f16..ae29332 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -102,8 +102,6 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 	return ret;
 }
 
-void kvm_pic_reset(struct kvm_kpic_state *s);
-
 void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu);
-- 
2.9.3

[PATCH v2 21/24] KVM: x86: drop picdev_in_range()

[David Hildenbrand]

We already have the exact same checks a couple of lines below.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/i8259.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index a871e08..dda4abd 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -451,46 +451,33 @@ static u32 elcr_ioport_read(void *opaque, u32 addr1)
 	return s->elcr;
 }
 
-static int picdev_in_range(gpa_t addr)
-{
-	switch (addr) {
-	case 0x20:
-	case 0x21:
-	case 0xa0:
-	case 0xa1:
-	case 0x4d0:
-	case 0x4d1:
-		return 1;
-	default:
-		return 0;
-	}
-}
-
 static int picdev_write(struct kvm_pic *s,
 			 gpa_t addr, int len, const void *val)
 {
 	unsigned char data = *(unsigned char *)val;
-	if (!picdev_in_range(addr))
-		return -EOPNOTSUPP;
 
 	if (len != 1) {
 		pr_pic_unimpl("non byte write\n");
 		return 0;
 	}
-	pic_lock(s);
 	switch (addr) {
 	case 0x20:
 	case 0x21:
 	case 0xa0:
 	case 0xa1:
+		pic_lock(s);
 		pic_ioport_write(&s->pics[addr >> 7], addr, data);
+		pic_unlock(s);
 		break;
 	case 0x4d0:
 	case 0x4d1:
+		pic_lock(s);
 		elcr_ioport_write(&s->pics[addr & 1], addr, data);
+		pic_unlock(s);
 		break;
+	default:
+		return -EOPNOTSUPP;
 	}
-	pic_unlock(s);
 	return 0;
 }
 
@@ -498,29 +485,31 @@ static int picdev_read(struct kvm_pic *s,
 		       gpa_t addr, int len, void *val)
 {
 	unsigned char data = 0;
-	if (!picdev_in_range(addr))
-		return -EOPNOTSUPP;
 
 	if (len != 1) {
 		memset(val, 0, len);
 		pr_pic_unimpl("non byte read\n");
 		return 0;
 	}
-	pic_lock(s);
 	switch (addr) {
 	case 0x20:
 	case 0x21:
 	case 0xa0:
 	case 0xa1:
+		pic_lock(s);
 		data = pic_ioport_read(&s->pics[addr >> 7], addr);
+		pic_unlock(s);
 		break;
 	case 0x4d0:
 	case 0x4d1:
+		pic_lock(s);
 		data = elcr_ioport_read(&s->pics[addr & 1], addr);
+		pic_unlock(s);
 		break;
+	default:
+		return -EOPNOTSUPP;
 	}
 	*(unsigned char *)val = data;
-	pic_unlock(s);
 	return 0;
 }
 
-- 
2.9.3

Re: [PATCH v2 21/24] KVM: x86: drop picdev_in_range()

[Paolo Bonzini]

On 29/03/2017 02:28, Steve Rutherford wrote:
> Not sure this matters, but this changes behavior slightly. This reorders
> validation of the length and and validation of the operation.

This would change behavior if for example:

- KVM registered port 0x22

- the guest wrote 4 bytes to port 0x1f

Before, picdev_write would return -EOPNOTSUPP and KVM would invoke the
handler for port 0x22; now it would return 0.

Since the PIT ports are far from anything else registered by the kernel,
this should not matter.

Paolo

[PATCH v2 22/24] KVM: x86: set data directly in picdev_read()

[David Hildenbrand]

Now it looks almost as picdev_write().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/i8259.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index dda4abd..3a8be26 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -484,7 +484,7 @@ static int picdev_write(struct kvm_pic *s,
 static int picdev_read(struct kvm_pic *s,
 		       gpa_t addr, int len, void *val)
 {
-	unsigned char data = 0;
+	unsigned char *data = (unsigned char *)val;
 
 	if (len != 1) {
 		memset(val, 0, len);
@@ -497,19 +497,18 @@ static int picdev_read(struct kvm_pic *s,
 	case 0xa0:
 	case 0xa1:
 		pic_lock(s);
-		data = pic_ioport_read(&s->pics[addr >> 7], addr);
+		*data = pic_ioport_read(&s->pics[addr >> 7], addr);
 		pic_unlock(s);
 		break;
 	case 0x4d0:
 	case 0x4d1:
 		pic_lock(s);
-		data = elcr_ioport_read(&s->pics[addr & 1], addr);
+		*data = elcr_ioport_read(&s->pics[addr & 1], addr);
 		pic_unlock(s);
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
-	*(unsigned char *)val = data;
 	return 0;
 }
 
-- 
2.9.3

[PATCH v2 23/24] KVM: x86: simplify pic_ioport_read()

[David Hildenbrand]

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/i8259.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 3a8be26..d07a629 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -417,19 +417,16 @@ static u32 pic_poll_read(struct kvm_kpic_state *s, u32 addr1)
 	return ret;
 }
 
-static u32 pic_ioport_read(void *opaque, u32 addr1)
+static u32 pic_ioport_read(void *opaque, u32 addr)
 {
 	struct kvm_kpic_state *s = opaque;
-	unsigned int addr;
 	int ret;
 
-	addr = addr1;
-	addr &= 1;
 	if (s->poll) {
-		ret = pic_poll_read(s, addr1);
+		ret = pic_poll_read(s, addr);
 		s->poll = 0;
 	} else
-		if (addr == 0)
+		if ((addr & 1) == 0)
 			if (s->read_reg_select)
 				ret = s->isr;
 			else
-- 
2.9.3

[PATCH v2 24/24] KVM: x86: use irqchip_kernel() to check for pic+ioapic

[David Hildenbrand]

Although the current check is not wrong, this check explicitly includes
the pic.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/irq_comm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 1728ff1..5494395 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -232,7 +232,7 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
 		goto unlock;
 	}
 	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
-	if (!ioapic_in_kernel(kvm))
+	if (!irqchip_kernel(kvm))
 		goto unlock;
 
 	kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
-- 
2.9.3