[PATCH RFC 00/21] pic/ioapic/irqchip cleanups + minor fixes

[PATCH RFC 00/21] 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.

Feel free to nack if a a certain cleanup is not worth it.

David Hildenbrand (21):
  KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP
  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: 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()

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/i8259.c            | 70 +++++++++++++++--------------------------
 arch/x86/kvm/ioapic.c           | 28 ++++++-----------
 arch/x86/kvm/ioapic.h           | 14 ++-------
 arch/x86/kvm/irq.c              |  2 +-
 arch/x86/kvm/irq.h              | 20 +++++-------
 arch/x86/kvm/irq_comm.c         | 39 +++++++++--------------
 arch/x86/kvm/x86.c              | 36 +++++++++------------
 include/linux/kvm_host.h        |  4 +--
 virt/kvm/eventfd.c              |  4 +--
 virt/kvm/kvm_main.c             |  3 ++
 11 files changed, 85 insertions(+), 136 deletions(-)

-- 
2.9.3

[PATCH RFC 01/21] 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.

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 c6b7aff..9cd6a34 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3077,8 +3077,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 RFC 02/21] 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. Add another state, indicating that the
caller is currently initializing 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/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/irq.h              | 8 +++++++-
 arch/x86/kvm/irq_comm.c         | 8 ++------
 arch/x86/kvm/x86.c              | 2 ++
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 74ef58c..c8cdcc3 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_KERNEL_INIT,  /* KVM_CREATE_IRQCHIP in progress */
 	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..9ebb6f5 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -96,6 +96,11 @@ static inline int irqchip_split(struct kvm *kvm)
 	return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
 }
 
+static inline int irqchip_kernel_init(struct kvm *kvm)
+{
+	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL_INIT;
+}
+
 static inline int irqchip_kernel(struct kvm *kvm)
 {
 	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
@@ -103,7 +108,8 @@ 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_KERNEL ||
+		   kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
 
 	/* Matches with wmb after initializing kvm->irq_routing. */
 	smp_rmb();
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index b96d389..4e4a67a 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -282,22 +282,18 @@ int kvm_set_routing_entry(struct kvm *kvm,
 
 	switch (ue->type) {
 	case KVM_IRQ_ROUTING_IRQCHIP:
+		if (!irqchip_kernel(kvm) && !irqchip_kernel_init(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;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b2a4b11..c69940c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4022,8 +4022,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto create_irqchip_unlock;
 		}
 
+		kvm->arch.irqchip_mode = KVM_IRQCHIP_KERNEL_INIT;
 		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 RFC 03/21] 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 9ebb6f5..150c3b3 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 RFC 04/21] 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 RFC 05/21] 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 150c3b3..d083876 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 4e4a67a..99f68c2 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -40,7 +40,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);
 }
 
@@ -234,7 +234,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 c69940c..52525e4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3719,18 +3719,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:
@@ -3745,23 +3744,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);
@@ -3770,7 +3768,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 RFC 06/21] 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 RFC 07/21] 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 RFC 08/21] 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 RFC 09/21] 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 52525e4..95372e3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3733,7 +3733,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;
@@ -3762,7 +3762,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 RFC 10/21] 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 95372e3..6ac81f0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4025,10 +4025,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

[PATCH RFC 11/21] 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 6ac81f0..7219b18 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4014,9 +4014,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;
 		}
 
@@ -4024,10 +4022,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 RFC 12/21] 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 99f68c2..7535965 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -277,33 +277,30 @@ int kvm_set_routing_entry(struct kvm *kvm,
 			  const struct kvm_irq_routing_entry *ue)
 {
 	int r = -EINVAL;
-	int delta;
-	unsigned max_pin;
 
 	switch (ue->type) {
 	case KVM_IRQ_ROUTING_IRQCHIP:
 		if (!irqchip_kernel(kvm) && !irqchip_kernel_init(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 RFC 13/21] 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 RFC 14/21] 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 RFC 15/21] 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 RFC 16/21] KVM: x86: drop goto label in kvm_set_routing_entry()

[David Hildenbrand]

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

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

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 7535965..04c3561 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -276,12 +276,10 @@ 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;
-
 	switch (ue->type) {
 	case KVM_IRQ_ROUTING_IRQCHIP:
 		if (!irqchip_kernel(kvm) && !irqchip_kernel_init(kvm))
-			goto out;
+			return -EINVAL;
 		e->irqchip.pin = ue->u.irqchip.pin;
 		switch (ue->u.irqchip.irqchip) {
 		case KVM_IRQCHIP_PIC_SLAVE:
@@ -289,16 +287,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;
@@ -309,7 +307,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;
@@ -317,12 +315,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 RFC 17/21] KVM: x86: simplify pic_unlock()

[David Hildenbrand]

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

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

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index e77e8c3..c50b839 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;
+				kvm_make_request(KVM_REQ_EVENT, vcpu);
+				kvm_vcpu_kick(vcpu);
 				break;
 			}
 		}
-
-		if (!found)
-			return;
-
-		kvm_make_request(KVM_REQ_EVENT, found);
-		kvm_vcpu_kick(found);
 	}
 }
 
-- 
2.9.3

[PATCH RFC 18/21] 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 c50b839..c29a7e1 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 d083876..ebb5893 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -108,8 +108,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 RFC 19/21] 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 c29a7e1..53c4c6a 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

[PATCH RFC 20/21] 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 53c4c6a..52d21f2 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 RFC 21/21] 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 52d21f2..1abd110 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

Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry()

[Paolo Bonzini]

On 06/03/2017 14:17, David Hildenbrand wrote:
>  	KVM_IRQCHIP_NONE,
> +	KVM_IRQCHIP_KERNEL_INIT,  /* KVM_CREATE_IRQCHIP in progress */

Maybe KVM_IRQCHIP_INIT_IN_PROGRESS?

> -	bool ret = kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE;
> +	bool ret = kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL ||
> +		   kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;

I suspect that if you phrase it the other way round (!= NONE && !=
KERNEL_INIT) you'll get infinitesimally better code, because it can be
compiled to an unsigned comparison with 1.

>  	/* Matches with wmb after initializing kvm->irq_routing. */
>  	smp_rmb();
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index b96d389..4e4a67a 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -282,22 +282,18 @@ int kvm_set_routing_entry(struct kvm *kvm,
>  
>  	switch (ue->type) {
>  	case KVM_IRQ_ROUTING_IRQCHIP:
> +		if (!irqchip_kernel(kvm) && !irqchip_kernel_init(kvm))
> +			goto out;
>  		delta = 0;

This can be irqchip_in_kernel, after which irqchip_kernel_init can be
removed.

Should the code to enable split irqchip also use KVM_IRQCHIP_KERNEL_INIT?

Paolo

Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry()

[David Hildenbrand]

Am 06.03.2017 um 19:08 schrieb Paolo Bonzini:
> 
> 
> On 06/03/2017 14:17, David Hildenbrand wrote:
>>  	KVM_IRQCHIP_NONE,
>> +	KVM_IRQCHIP_KERNEL_INIT,  /* KVM_CREATE_IRQCHIP in progress */
> 
> Maybe KVM_IRQCHIP_INIT_IN_PROGRESS?

I tried to make it short but I agree, that is more self explaining.

> 
>> -	bool ret = kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE;
>> +	bool ret = kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL ||
>> +		   kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
> 
> I suspect that if you phrase it the other way round (!= NONE && !=
> KERNEL_INIT) you'll get infinitesimally better code, because it can be
> compiled to an unsigned comparison with 1.

However, adding new modes can silently make this check wrong (e.g.
grepping for KVM_IRQCHIP_KERNEL will no longer identify all users). Do
you think the optimization is worth it?

> 
>>  	/* Matches with wmb after initializing kvm->irq_routing. */
>>  	smp_rmb();
>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>> index b96d389..4e4a67a 100644
>> --- a/arch/x86/kvm/irq_comm.c
>> +++ b/arch/x86/kvm/irq_comm.c
>> @@ -282,22 +282,18 @@ int kvm_set_routing_entry(struct kvm *kvm,
>>  
>>  	switch (ue->type) {
>>  	case KVM_IRQ_ROUTING_IRQCHIP:
>> +		if (!irqchip_kernel(kvm) && !irqchip_kernel_init(kvm))
>> +			goto out;
>>  		delta = 0;
> 
> This can be irqchip_in_kernel, after which irqchip_kernel_init can be
> removed.

irqchip_in_kernel in its current form would allow KVM_IRQCHIP_SPLIT,
which is not what we want here, or am I missing something?

> 
> Should the code to enable split irqchip also use KVM_IRQCHIP_KERNEL_INIT?
> 

a) checking against KVM_IRQCHIP_KERNEL_INIT shouldn't be necessary due
to the kvm->lock (this code path will never see KVM_IRQCHIP_KERNEL_INIT).

b) setting KVM_IRQCHIP_KERNEL_INIT could be done. We only initialize an
empty irq routing. But also that should never be allowed to set up
routings targeted at pic/ioapic. However that would in its current form
never happen.

> Paolo
> 

Thanks!

David

Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry()

[Paolo Bonzini]

On 07/03/2017 10:55, David Hildenbrand wrote:
> Am 06.03.2017 um 19:08 schrieb Paolo Bonzini:
>>> -	bool ret = kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE;
>>> +	bool ret = kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL ||
>>> +		   kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
>>
>> I suspect that if you phrase it the other way round (!= NONE && !=
>> KERNEL_INIT) you'll get infinitesimally better code, because it can be
>> compiled to an unsigned comparison with 1.
> 
> However, adding new modes can silently make this check wrong (e.g.
> grepping for KVM_IRQCHIP_KERNEL will no longer identify all users). Do
> you think the optimization is worth it?

I don't think we want to add new modes.

>>
>>>  	/* Matches with wmb after initializing kvm->irq_routing. */
>>>  	smp_rmb();
>>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>>> index b96d389..4e4a67a 100644
>>> --- a/arch/x86/kvm/irq_comm.c
>>> +++ b/arch/x86/kvm/irq_comm.c
>>> @@ -282,22 +282,18 @@ int kvm_set_routing_entry(struct kvm *kvm,
>>>  
>>>  	switch (ue->type) {
>>>  	case KVM_IRQ_ROUTING_IRQCHIP:
>>> +		if (!irqchip_kernel(kvm) && !irqchip_kernel_init(kvm))
>>> +			goto out;
>>>  		delta = 0;
>>
>> This can be irqchip_in_kernel, after which irqchip_kernel_init can be
>> removed.
> 
> irqchip_in_kernel in its current form would allow KVM_IRQCHIP_SPLIT,
> which is not what we want here, or am I missing something?

Hmm, perhaps we can split the checks to rule out KVM_IRQCHIP_NONE
outside the switch, and KVM_IRQCHIP_SPLIT here?

>> Should the code to enable split irqchip also use KVM_IRQCHIP_KERNEL_INIT?
> 
> a) checking against KVM_IRQCHIP_KERNEL_INIT shouldn't be necessary due
> to the kvm->lock (this code path will never see KVM_IRQCHIP_KERNEL_INIT).
> 
> b) setting KVM_IRQCHIP_KERNEL_INIT could be done. We only initialize an
> empty irq routing. But also that should never be allowed to set up
> routings targeted at pic/ioapic. However that would in its current form
> never happen.

You're right, it'd be just a matter of keeping the code similar between
the two cases.  You can try it and decide when you post the next version.

Thanks!

Paolo

Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry()

[Radim Krčmář]

2017-03-07 11:53+0100, Paolo Bonzini:
> On 07/03/2017 10:55, David Hildenbrand wrote:
>> Am 06.03.2017 um 19:08 schrieb Paolo Bonzini:
>>>> -	bool ret = kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE;
>>>> +	bool ret = kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL ||
>>>> +		   kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
>>>
>>> I suspect that if you phrase it the other way round (!= NONE && !=
>>> KERNEL_INIT) you'll get infinitesimally better code, because it can be
>>> compiled to an unsigned comparison with 1.

Yes, it seems that the compiler must assume that an enum can also be a
value that is not enumerated.

>> However, adding new modes can silently make this check wrong (e.g.
>> grepping for KVM_IRQCHIP_KERNEL will no longer identify all users). Do
>> you think the optimization is worth it?
> 
> I don't think we want to add new modes.

I would prefer to write it like:

  kvm->arch.irqchip_mode > KVM_IRQCHIP_KERNEL_INIT;

Same assembly with simpler code.  Setting KVM_IRQCHIP_KERNEL_INIT before
KVM_IRQCHIP_SPLIT would make it a bit more descriprive (and would allow
the check below as well).

>>>>  	/* Matches with wmb after initializing kvm->irq_routing. */
>>>>  	smp_rmb();
>>>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>>>> index b96d389..4e4a67a 100644
>>>> --- a/arch/x86/kvm/irq_comm.c
>>>> +++ b/arch/x86/kvm/irq_comm.c
>>>> @@ -282,22 +282,18 @@ int kvm_set_routing_entry(struct kvm *kvm,
>>>>  
>>>>  	switch (ue->type) {
>>>>  	case KVM_IRQ_ROUTING_IRQCHIP:
>>>> +		if (!irqchip_kernel(kvm) && !irqchip_kernel_init(kvm))
>>>> +			goto out;
>>>>  		delta = 0;
>>>
>>> This can be irqchip_in_kernel, after which irqchip_kernel_init can be
>>> removed.
>> 
>> irqchip_in_kernel in its current form would allow KVM_IRQCHIP_SPLIT,
>> which is not what we want here, or am I missing something?
> 
> Hmm, perhaps we can split the checks to rule out KVM_IRQCHIP_NONE
> outside the switch, and KVM_IRQCHIP_SPLIT here?

Right, we don't want MSI or HV without LAPIC in kernel either.

Re: [PATCH RFC 02/21] KVM: x86: check against irqchip_mode in kvm_set_routing_entry()

[David Hildenbrand]

>>> However, adding new modes can silently make this check wrong (e.g.
>>> grepping for KVM_IRQCHIP_KERNEL will no longer identify all users). Do
>>> you think the optimization is worth it?
>>
>> I don't think we want to add new modes.
> 
> I would prefer to write it like:
> 
>   kvm->arch.irqchip_mode > KVM_IRQCHIP_KERNEL_INIT;

Agreed.

> 
> Same assembly with simpler code.  Setting KVM_IRQCHIP_KERNEL_INIT before
> KVM_IRQCHIP_SPLIT would make it a bit more descriprive (and would allow
> the check below as well).

Okay, I added that.

> 
>>>>>  	/* Matches with wmb after initializing kvm->irq_routing. */
>>>>>  	smp_rmb();
>>>>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>>>>> index b96d389..4e4a67a 100644
>>>>> --- a/arch/x86/kvm/irq_comm.c
>>>>> +++ b/arch/x86/kvm/irq_comm.c
>>>>> @@ -282,22 +282,18 @@ int kvm_set_routing_entry(struct kvm *kvm,
>>>>>  
>>>>>  	switch (ue->type) {
>>>>>  	case KVM_IRQ_ROUTING_IRQCHIP:
>>>>> +		if (!irqchip_kernel(kvm) && !irqchip_kernel_init(kvm))
>>>>> +			goto out;
>>>>>  		delta = 0;
>>>>
>>>> This can be irqchip_in_kernel, after which irqchip_kernel_init can be
>>>> removed.
>>>
>>> irqchip_in_kernel in its current form would allow KVM_IRQCHIP_SPLIT,
>>> which is not what we want here, or am I missing something?
>>
>> Hmm, perhaps we can split the checks to rule out KVM_IRQCHIP_NONE
>> outside the switch, and KVM_IRQCHIP_SPLIT here?
> 
> Right, we don't want MSI or HV without LAPIC in kernel either.
> 

Ok, I will see how this turns out!

Thanks!

-- 
Thanks,

David