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

[PATCH v3 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.

v2 -> v3:
- Added memory barriers as requested by Paolo to patch 2-4

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           | 16 ++++-----
 arch/x86/kvm/irq.c              |  2 +-
 arch/x86/kvm/irq.h              | 32 +++++++++---------
 arch/x86/kvm/irq_comm.c         | 45 ++++++++++++--------------
 arch/x86/kvm/x86.c              | 45 +++++++++++++-------------
 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, 117 insertions(+), 146 deletions(-)

-- 
2.9.3

[PATCH v3 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 88257b3..a80e400 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3082,8 +3082,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

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

[Steve Rutherford]

Sorry to resurrect this thread. There's an srcu_synchronize_expedited
call in kvm_set_irq_routing, so this lock might get held for a fairly
long while. Wouldn't it be a better idea to check the irqchip mode?

On Fri, Apr 7, 2017 at 1:50 AM, David Hildenbrand <david@redhat.com> wrote:
> 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 88257b3..a80e400 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3082,8 +3082,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
>

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

[David Hildenbrand]

On 25.04.2017 00:39, Steve Rutherford wrote:
> Sorry to resurrect this thread. There's an srcu_synchronize_expedited
> call in kvm_set_irq_routing, so this lock might get held for a fairly
> long while. Wouldn't it be a better idea to check the irqchip mode?
> 

We could maybe check for kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE
here under lock instead of in the function itself. Maybe we then even
don't need patch NR 2. Will have a look!

Thanks!

> On Fri, Apr 7, 2017 at 1:50 AM, David Hildenbrand <david@redhat.com> wrote:
>> 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 88257b3..a80e400 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3082,8 +3082,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
>>


-- 

Thanks,

David

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

[Radim Krčmář]

2017-04-25 14:34+0200, David Hildenbrand:
> On 25.04.2017 00:39, Steve Rutherford wrote:
>> Sorry to resurrect this thread. There's an srcu_synchronize_expedited
>> call in kvm_set_irq_routing, so this lock might get held for a fairly
>> long while. Wouldn't it be a better idea to check the irqchip mode?
>> 
> 
> We could maybe check for kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE
> here under lock instead of in the function itself. Maybe we then even
> don't need patch NR 2. Will have a look!

Sounds good and the extra mode would be obsolete.  Checking in
kvm_main.c will just need a new arch hook.  The patches are already in
kvm/next, though, so please prepare it as a fix.

Thanks.

[PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[David Hildenbrand]

We needed the lock to avoid racing with creation of the irqchip on x86. As
kvm_set_irq_routing() calls srcu_synchronize_expedited(), this lock
might be held for a longer time.

Let's introduce an arch specific callback to check if we can actually
add irq routes. For x86, all we have to do is check if we have an
irqchip in the kernel. We don't need kvm->lock at that point as the
irqchip is marked as inititalized only when actually fully created.

Reported-by: Steve Rutherford <srutherford@google.com>
Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP")
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/irq_comm.c         | 15 +++++++++------
 arch/x86/kvm/x86.c              | 11 +----------
 include/linux/kvm_host.h        |  5 +++++
 virt/kvm/kvm_main.c             |  5 ++---
 6 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 2cc5ec7..d962fa9 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -726,7 +726,6 @@ 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 0edd22c..d5005cc 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -111,7 +111,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 
 	/* Matches smp_wmb() when setting irqchip_mode */
 	smp_rmb();
-	return mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
+	return mode != KVM_IRQCHIP_NONE;
 }
 
 void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 4517a4c..3cc3b2d 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -274,16 +274,19 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, unsigned irqchip, unsigned pin,
 	srcu_read_unlock(&kvm->irq_srcu, idx);
 }
 
+bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
+{
+	return irqchip_in_kernel(kvm);
+}
+
 int kvm_set_routing_entry(struct kvm *kvm,
 			  struct kvm_kernel_irq_routing_entry *e,
 			  const struct kvm_irq_routing_entry *ue)
 {
-	/* also allow creation of routes during KVM_IRQCHIP_INIT_IN_PROGRESS */
-	if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
-		return -EINVAL;
-
-	/* Matches smp_wmb() when setting irqchip_mode */
-	smp_rmb();
+	/* We can't check irqchip_in_kernel() here as some callers are
+	 * currently inititalizing the irqchip. Other callers should therefore
+	 * check kvm_arch_can_set_irq_routing() before calling this function.
+	 */
 	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 34bf64f..eae0e9c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3911,14 +3911,9 @@ 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) {
-			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
-			/* Pairs with smp_rmb() when reading irqchip_mode */
-			smp_wmb();
+		if (r)
 			goto split_irqchip_unlock;
-		}
 		/* Pairs with irqchip_in_kernel. */
 		smp_wmb();
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
@@ -4004,12 +3999,8 @@ 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;
-			/* Pairs with smp_rmb() when reading irqchip_mode */
-			smp_wmb();
 			kvm_ioapic_destroy(kvm);
 			kvm_pic_destroy(kvm);
 			goto create_irqchip_unlock;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 397b7b5..a9921b1 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu);
 #ifdef __KVM_HAVE_IOAPIC
 void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
 void kvm_arch_post_irq_routing_update(struct kvm *kvm);
+bool kvm_arch_can_set_irq_routing(struct kvm *kvm);
 #else
 static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
 {
@@ -511,6 +512,10 @@ 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)
 {
 }
+static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
+{
+	return true;
+}
 #endif
 
 #ifdef CONFIG_HAVE_KVM_IRQFD
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 357e67c..0a37ba4 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3054,6 +3054,8 @@ static long kvm_vm_ioctl(struct file *filp,
 		if (copy_from_user(&routing, argp, sizeof(routing)))
 			goto out;
 		r = -EINVAL;
+		if (!kvm_arch_can_set_irq_routing(kvm))
+			goto out;
 		if (routing.nr > KVM_MAX_IRQ_ROUTES)
 			goto out;
 		if (routing.flags)
@@ -3069,11 +3071,8 @@ 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

Re: [PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[Steve Rutherford]

Thanks for putting this together!

On Tue, Apr 25, 2017 at 12:03 PM, David Hildenbrand <david@redhat.com> wrote:
> We needed the lock to avoid racing with creation of the irqchip on x86. As
> kvm_set_irq_routing() calls srcu_synchronize_expedited(), this lock
> might be held for a longer time.
>
> Let's introduce an arch specific callback to check if we can actually
> add irq routes. For x86, all we have to do is check if we have an
> irqchip in the kernel. We don't need kvm->lock at that point as the
> irqchip is marked as inititalized only when actually fully created.
>
> Reported-by: Steve Rutherford <srutherford@google.com>
> Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP")
> 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/irq_comm.c         | 15 +++++++++------
>  arch/x86/kvm/x86.c              | 11 +----------
>  include/linux/kvm_host.h        |  5 +++++
>  virt/kvm/kvm_main.c             |  5 ++---
>  6 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 2cc5ec7..d962fa9 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -726,7 +726,6 @@ 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 0edd22c..d5005cc 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -111,7 +111,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
>
>         /* Matches smp_wmb() when setting irqchip_mode */
>         smp_rmb();
> -       return mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
> +       return mode != KVM_IRQCHIP_NONE;
>  }
>
>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 4517a4c..3cc3b2d 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -274,16 +274,19 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, unsigned irqchip, unsigned pin,
>         srcu_read_unlock(&kvm->irq_srcu, idx);
>  }
>
> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
> +{
> +       return irqchip_in_kernel(kvm);
> +}
> +
>  int kvm_set_routing_entry(struct kvm *kvm,
>                           struct kvm_kernel_irq_routing_entry *e,
>                           const struct kvm_irq_routing_entry *ue)
>  {
> -       /* also allow creation of routes during KVM_IRQCHIP_INIT_IN_PROGRESS */
> -       if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
> -               return -EINVAL;
> -
> -       /* Matches smp_wmb() when setting irqchip_mode */
> -       smp_rmb();
> +       /* We can't check irqchip_in_kernel() here as some callers are
> +        * currently inititalizing the irqchip. Other callers should therefore
> +        * check kvm_arch_can_set_irq_routing() before calling this function.
> +        */
>         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 34bf64f..eae0e9c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3911,14 +3911,9 @@ 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) {
> -                       kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
> -                       /* Pairs with smp_rmb() when reading irqchip_mode */
> -                       smp_wmb();
> +               if (r)
>                         goto split_irqchip_unlock;
> -               }
>                 /* Pairs with irqchip_in_kernel. */
>                 smp_wmb();
>                 kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
> @@ -4004,12 +3999,8 @@ 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;
> -                       /* Pairs with smp_rmb() when reading irqchip_mode */
> -                       smp_wmb();
>                         kvm_ioapic_destroy(kvm);
>                         kvm_pic_destroy(kvm);
>                         goto create_irqchip_unlock;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 397b7b5..a9921b1 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu);
>  #ifdef __KVM_HAVE_IOAPIC
>  void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
>  void kvm_arch_post_irq_routing_update(struct kvm *kvm);
> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm);
>  #else
>  static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
>  {
> @@ -511,6 +512,10 @@ 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)
>  {
>  }
> +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
> +{
> +       return true;
> +}
>  #endif
>
>  #ifdef CONFIG_HAVE_KVM_IRQFD
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 357e67c..0a37ba4 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3054,6 +3054,8 @@ static long kvm_vm_ioctl(struct file *filp,
>                 if (copy_from_user(&routing, argp, sizeof(routing)))
>                         goto out;
>                 r = -EINVAL;
> +               if (!kvm_arch_can_set_irq_routing(kvm))
> +                       goto out;
>                 if (routing.nr > KVM_MAX_IRQ_ROUTES)
>                         goto out;
>                 if (routing.flags)
> @@ -3069,11 +3071,8 @@ 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
>

Re: [PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[Radim Krčmář]

2017-04-25 21:03+0200, David Hildenbrand:
> We needed the lock to avoid racing with creation of the irqchip on x86. As
> kvm_set_irq_routing() calls srcu_synchronize_expedited(), this lock
> might be held for a longer time.
> 
> Let's introduce an arch specific callback to check if we can actually
> add irq routes. For x86, all we have to do is check if we have an
> irqchip in the kernel. We don't need kvm->lock at that point as the
> irqchip is marked as inititalized only when actually fully created.
> 
> Reported-by: Steve Rutherford <srutherford@google.com>
> Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  6 files changed, 18 insertions(+), 21 deletions(-)

Nice!

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu);
>  #ifdef __KVM_HAVE_IOAPIC
>  void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
>  void kvm_arch_post_irq_routing_update(struct kvm *kvm);
> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm);

(A nitpick: it might be useful even without __KVM_HAVE_IOAPIC so weak
 linking would probably be cleaner for a slow path.)

>  #else
>  static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
>  {
> @@ -511,6 +512,10 @@ 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)
>  {
>  }
> +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
> +{
> +	return true;
> +}
>  #endif
>  

Re: [PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[David Hildenbrand]

On 25.04.2017 21:59, Radim Krčmář wrote:
> 2017-04-25 21:03+0200, David Hildenbrand:
>> We needed the lock to avoid racing with creation of the irqchip on x86. As
>> kvm_set_irq_routing() calls srcu_synchronize_expedited(), this lock
>> might be held for a longer time.
>>
>> Let's introduce an arch specific callback to check if we can actually
>> add irq routes. For x86, all we have to do is check if we have an
>> irqchip in the kernel. We don't need kvm->lock at that point as the
>> irqchip is marked as inititalized only when actually fully created.
>>
>> Reported-by: Steve Rutherford <srutherford@google.com>
>> Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP")
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  6 files changed, 18 insertions(+), 21 deletions(-)
> 
> Nice!
> 
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> 
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu);
>>  #ifdef __KVM_HAVE_IOAPIC
>>  void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
>>  void kvm_arch_post_irq_routing_update(struct kvm *kvm);
>> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm);
> 
> (A nitpick: it might be useful even without __KVM_HAVE_IOAPIC so weak
>  linking would probably be cleaner for a slow path.)
> 
>>  #else
>>  static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
>>  {
>> @@ -511,6 +512,10 @@ 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)
>>  {
>>  }
>> +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
>> +{
>> +	return true;
>> +}
>>  #endif
>>  

Makes sense, shall I resend or can you fix that up when applying?

Thanks!

-- 

Thanks,

David

Re: [PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[Radim Krčmář]

2017-04-26 10:40+0200, David Hildenbrand:
> On 25.04.2017 21:59, Radim Krčmář wrote:
>> 2017-04-25 21:03+0200, David Hildenbrand:
>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>> @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu);
>>>  #ifdef __KVM_HAVE_IOAPIC
>>>  void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
>>>  void kvm_arch_post_irq_routing_update(struct kvm *kvm);
>>> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm);
>> 
>> (A nitpick: it might be useful even without __KVM_HAVE_IOAPIC so weak
>>  linking would probably be cleaner for a slow path.)
>> 
>>>  #else
>>>  static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
>>>  {
>>> @@ -511,6 +512,10 @@ 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)
>>>  {
>>>  }
>>> +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
>>> +{
>>> +	return true;
>>> +}
>>>  #endif
>>>  
> 
> Makes sense, shall I resend or can you fix that up when applying?

Please send a v2, it's going to be automatically compile-tested on all
arches that way. :)

Re: [PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[David Hildenbrand]

On 26.04.2017 16:27, Radim Krčmář wrote:
> 2017-04-26 10:40+0200, David Hildenbrand:
>> On 25.04.2017 21:59, Radim Krčmář wrote:
>>> 2017-04-25 21:03+0200, David Hildenbrand:
>>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>>> @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu);
>>>>  #ifdef __KVM_HAVE_IOAPIC
>>>>  void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
>>>>  void kvm_arch_post_irq_routing_update(struct kvm *kvm);
>>>> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm);
>>>
>>> (A nitpick: it might be useful even without __KVM_HAVE_IOAPIC so weak
>>>  linking would probably be cleaner for a slow path.)
>>>
>>>>  #else
>>>>  static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
>>>>  {
>>>> @@ -511,6 +512,10 @@ 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)
>>>>  {
>>>>  }
>>>> +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
>>>> +{
>>>> +	return true;
>>>> +}
>>>>  #endif
>>>>  
>>
>> Makes sense, shall I resend or can you fix that up when applying?
> 
> Please send a v2, it's going to be automatically compile-tested on all
> arches that way. :)
> 

Using a weak symbol for the first time... Is the following what you had in mind?

---- snip ----
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 397b7b5..b725d4e 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1009,6 +1009,7 @@ static inline int mmu_notifier_retry(struct kvm *kvm, unsigned long mmu_seq)
 #define KVM_MAX_IRQ_ROUTES 1024
 #endif
 
+bool kvm_arch_can_set_irq_routing(struct kvm *kvm);
 int kvm_set_irq_routing(struct kvm *kvm,
                        const struct kvm_irq_routing_entry *entries,
                        unsigned nr,
diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index cc30d01..a080659 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -172,6 +172,11 @@ void __attribute__((weak)) kvm_arch_irq_routing_update(struct kvm *kvm)
 {
 }
 
+bool __attribute__((weak)) kvm_arch_can_set_irq_routing(struct kvm *kvm)
+{
+       return true;
+}
+
 int kvm_set_irq_routing(struct kvm *kvm,
                        const struct kvm_irq_routing_entry *ue,
                        unsigned nr,
---- snip ----

-- 

Thanks,

David

Re: [PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[Radim Krčmář]

2017-04-26 17:21+0200, David Hildenbrand:
> On 26.04.2017 16:27, Radim Krčmář wrote:
> > 2017-04-26 10:40+0200, David Hildenbrand:
> >> On 25.04.2017 21:59, Radim Krčmář wrote:
> >>> 2017-04-25 21:03+0200, David Hildenbrand:
> >>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> >>>> @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu);
> >>>>  #ifdef __KVM_HAVE_IOAPIC
> >>>>  void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm);
> >>>>  void kvm_arch_post_irq_routing_update(struct kvm *kvm);
> >>>> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm);
> >>>
> >>> (A nitpick: it might be useful even without __KVM_HAVE_IOAPIC so weak
> >>>  linking would probably be cleaner for a slow path.)
> >>>
> >>>>  #else
> >>>>  static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
> >>>>  {
> >>>> @@ -511,6 +512,10 @@ 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)
> >>>>  {
> >>>>  }
> >>>> +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
> >>>> +{
> >>>> +	return true;
> >>>> +}
> >>>>  #endif
> >>>>  
> >>
> >> Makes sense, shall I resend or can you fix that up when applying?
> > 
> > Please send a v2, it's going to be automatically compile-tested on all
> > arches that way. :)
> > 
> 
> Using a weak symbol for the first time... Is the following what you had in mind?
> 
> ---- snip ----
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 397b7b5..b725d4e 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1009,6 +1009,7 @@ static inline int mmu_notifier_retry(struct kvm *kvm, unsigned long mmu_seq)
>  #define KVM_MAX_IRQ_ROUTES 1024
>  #endif
>  
> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm);
>  int kvm_set_irq_routing(struct kvm *kvm,
>                         const struct kvm_irq_routing_entry *entries,
>                         unsigned nr,
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index cc30d01..a080659 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -172,6 +172,11 @@ void __attribute__((weak)) kvm_arch_irq_routing_update(struct kvm *kvm)
>  {
>  }
>  
> +bool __attribute__((weak)) kvm_arch_can_set_irq_routing(struct kvm *kvm)

Yes, exactly what I had in mind.
(I'd just write "__weak" to make it shorter. :])

> +{
> +       return true;
> +}
> +
>  int kvm_set_irq_routing(struct kvm *kvm,
>                         const struct kvm_irq_routing_entry *ue,
>                         unsigned nr,
> ---- snip ----
> 
> -- 
> 
> Thanks,
> 
> David

Re: [PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

Hi David,

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on next-20170424]
[cannot apply to v4.11-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Hildenbrand/KVM-x86-don-t-hold-kvm-lock-in-KVM_SET_GSI_ROUTING/20170426-171503
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: powerpc-currituck_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   In file included from arch/powerpc/include/asm/kvm_ppc.h:30:0,
                    from arch/powerpc/kernel/smp.c:41:
>> include/linux/kvm_host.h:515:13: error: 'kvm_arch_can_set_irq_routing' defined but not used [-Werror=unused-function]
    static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors

vim +/kvm_arch_can_set_irq_routing +515 include/linux/kvm_host.h

   509	static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
   510	{
   511	}
   512	static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm)
   513	{
   514	}
 > 515	static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
   516	{
   517		return true;
   518	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14606 bytes --]

Re: [PATCH] KVM: x86: don't hold kvm->lock in KVM_SET_GSI_ROUTING

[Radim Krčmář]

2017-04-26 20:33+0800, kbuild test robot:
> Hi David,
> 
> [auto build test ERROR on kvm/linux-next]
> [also build test ERROR on next-20170424]
> [cannot apply to v4.11-rc8]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/David-Hildenbrand/KVM-x86-don-t-hold-kvm-lock-in-KVM_SET_GSI_ROUTING/20170426-171503
> base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
> config: powerpc-currituck_defconfig (attached as .config)
> compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=powerpc 
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from arch/powerpc/include/asm/kvm_ppc.h:30:0,
>                     from arch/powerpc/kernel/smp.c:41:
> >> include/linux/kvm_host.h:515:13: error: 'kvm_arch_can_set_irq_routing' defined but not used [-Werror=unused-function]
>     static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    cc1: all warnings being treated as errors
> 
> vim +/kvm_arch_can_set_irq_routing +515 include/linux/kvm_host.h
> 
>    509	static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
>    510	{
>    511	}
>    512	static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm)
>    513	{
>    514	}
>  > 515	static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)

Oh, we need both static and inline to supress the warning ...

>    516	{
>    517		return true;
>    518	}
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH v3 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              | 18 +++++++++++++-----
 arch/x86/kvm/x86.c              | 11 ++++++++++-
 3 files changed, 24 insertions(+), 6 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..59e05fe 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -93,21 +93,29 @@ static inline int pic_in_kernel(struct kvm *kvm)
 
 static inline int irqchip_split(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
+	int mode = kvm->arch.irqchip_mode;
+
+	/* Matches smp_wmb() when setting 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;
+
+	/* Matches smp_wmb() when setting 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_NONE;
+	int mode = kvm->arch.irqchip_mode;
 
-	/* Matches with wmb after initializing kvm->irq_routing. */
+	/* Matches smp_wmb() when setting 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/x86.c b/arch/x86/kvm/x86.c
index ccbd45e..8f776f0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3934,9 +3934,14 @@ 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;
+			/* Pairs with smp_rmb() when reading irqchip_mode */
+			smp_wmb();
 			goto split_irqchip_unlock;
+		}
 		/* Pairs with irqchip_in_kernel. */
 		smp_wmb();
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
@@ -4024,8 +4029,12 @@ 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;
+			/* Pairs with smp_rmb() when reading irqchip_mode */
+			smp_wmb();
 			mutex_lock(&kvm->slots_lock);
 			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
-- 
2.9.3

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

[Radim Krčmář]

2017-04-07 10:50+0200, 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>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -3934,9 +3934,14 @@ 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;
> +			/* Pairs with smp_rmb() when reading irqchip_mode */
> +			smp_wmb();
>  			goto split_irqchip_unlock;
> +		}
>  		/* Pairs with irqchip_in_kernel. */
>  		smp_wmb();
>  		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
> @@ -4024,8 +4029,12 @@ 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;
> +			/* Pairs with smp_rmb() when reading irqchip_mode */
> +			smp_wmb();

I think these two barriers are pointless.

KVM_IRQCHIP_INIT_IN_PROGRESS and KVM_IRQCHIP_NONE have the same result
for readers of irqchip mode and we don't even have a barrier after
setting KVM_IRQCHIP_INIT_IN_PROGRESS mode, so it looks weird.

I can remove them if you agree.

Thanks.

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

[David Hildenbrand]

On 12.04.2017 20:26, Radim Krčmář wrote:
> 2017-04-07 10:50+0200, 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>
>> ---
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> @@ -3934,9 +3934,14 @@ 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;
>> +			/* Pairs with smp_rmb() when reading irqchip_mode */
>> +			smp_wmb();
>>  			goto split_irqchip_unlock;
>> +		}
>>  		/* Pairs with irqchip_in_kernel. */
>>  		smp_wmb();
>>  		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
>> @@ -4024,8 +4029,12 @@ 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;
>> +			/* Pairs with smp_rmb() when reading irqchip_mode */
>> +			smp_wmb();
> 
> I think these two barriers are pointless.
> 
> KVM_IRQCHIP_INIT_IN_PROGRESS and KVM_IRQCHIP_NONE have the same result
> for readers of irqchip mode and we don't even have a barrier after
> setting KVM_IRQCHIP_INIT_IN_PROGRESS mode, so it looks weird.
> 
> I can remove them if you agree.

Paolo requested this, and I agreed to rather have a unified handling for
memory barriers with kvm->arch.irqchip_mode, then trying to optimize for
special cases.

(implicit knowledge here is e.g. that this is protected by kvm->lock)

If you think we can drop this, feel free to drop.

Thanks!

> 
> Thanks.
> 


-- 

Thanks,

David

[PATCH v3 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 | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 6825cd3..2e5eec8 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -282,24 +282,26 @@ 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;
+
+	/* Matches smp_wmb() when setting irqchip_mode */
+	smp_rmb();
 	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

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

[Radim Krčmář]

2017-04-07 10:50+0200, 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 | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 6825cd3..2e5eec8 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -282,24 +282,26 @@ 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;
> +
> +	/* Matches smp_wmb() when setting irqchip_mode */
> +	smp_rmb();

This barrier is superfluous as well ... aren't all callers using
kvm->lock to provide ordering?

The check for KVM_IRQCHIP_NONE would prevent nothing if we could catch
the initialization from the outside and hence need a barrier.

Thanks.

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

[David Hildenbrand]

On 12.04.2017 20:36, Radim Krčmář wrote:
> 2017-04-07 10:50+0200, 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 | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>> index 6825cd3..2e5eec8 100644
>> --- a/arch/x86/kvm/irq_comm.c
>> +++ b/arch/x86/kvm/irq_comm.c
>> @@ -282,24 +282,26 @@ 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;
>> +
>> +	/* Matches smp_wmb() when setting irqchip_mode */
>> +	smp_rmb();
> 
> This barrier is superfluous as well ... aren't all callers using
> kvm->lock to provide ordering?

Yes they are. Paolo suggested this. I think we can safely drop this.

Thanks!

> 
> The check for KVM_IRQCHIP_NONE would prevent nothing if we could catch
> the initialization from the outside and hence need a barrier.
> 
> Thanks.
> 


-- 

Thanks,

David

[PATCH v3 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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 59e05fe..26d8dd4 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -85,10 +85,11 @@ static inline struct kvm_pic *pic_irqchip(struct kvm *kvm)
 
 static inline int pic_in_kernel(struct kvm *kvm)
 {
-	int ret;
+	int mode = kvm->arch.irqchip_mode;
 
-	ret = (pic_irqchip(kvm) != NULL);
-	return ret;
+	/* Matches smp_wmb() when setting irqchip_mode */
+	smp_rmb();
+	return mode == KVM_IRQCHIP_KERNEL;
 }
 
 static inline int irqchip_split(struct kvm *kvm)
-- 
2.9.3

[PATCH v3 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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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

[PATCH v3 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 047b17a..af97c5a 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 26d8dd4..8fec66d 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)
 {
 	int mode = kvm->arch.irqchip_mode;
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 2e5eec8..d82fc14 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 8f776f0..a804e63 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 v3 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 289270a..a433317 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -646,7 +646,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;
 
@@ -659,7 +659,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 d88e4b9..5a1de92 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)
 {
 	int mode = kvm->arch.irqchip_mode;
-- 
2.9.3

[PATCH v3 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 a433317..da66650 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 v3 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 da66650..f8e988a 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -645,8 +645,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));
@@ -658,8 +656,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 v3 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 f8e988a..10941a6 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -642,7 +642,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;
 
@@ -650,10 +650,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;
 
@@ -664,5 +663,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 5a1de92..29ce197 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -128,8 +128,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 a804e63..cc5c884 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 v3 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 cc5c884..8424481 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4034,10 +4034,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			/* Pairs with smp_rmb() when reading irqchip_mode */
 			smp_wmb();
 			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 v3 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 af97c5a..2da2b42 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -660,9 +660,11 @@ void kvm_pic_destroy(struct kvm *kvm)
 	if (!vpic)
 		return;
 
+	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 10941a6..6e1d8cb 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -637,7 +637,9 @@ void kvm_ioapic_destroy(struct kvm *kvm)
 		return;
 
 	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 8424481..889b7f9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4021,9 +4021,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;
 		}
 
@@ -4033,10 +4031,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
 			/* Pairs with smp_rmb() when reading irqchip_mode */
 			smp_wmb();
-			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 v3 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 d82fc14..fb1e37e 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)
@@ -292,26 +290,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 v3 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 6e1d8cb..3072cdf 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 v3 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 3072cdf..dc29a27 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;
 	}
 }
@@ -660,7 +660,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 v3 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 dc29a27..bdff437 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 d025074..12f930c 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 4d28a9d..a8d5403 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 v3 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 fb1e37e..d71f7a7 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -278,18 +278,16 @@ 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;
 
 	/* Matches smp_wmb() when setting irqchip_mode */
 	smp_rmb();
 	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:
@@ -297,16 +295,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;
@@ -317,7 +315,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;
@@ -325,12 +323,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 v3 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 v3 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 2da2b42..fce571c 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 v3 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 fce571c..0252e68 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 8fec66d..0edd22c 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -114,8 +114,6 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 	return mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
 }
 
-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 v3 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 0252e68..5b5c87f 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 v3 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 5b5c87f..d481f33 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 v3 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 d481f33..bdcd413 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 v3 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 d71f7a7..4517a4c 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

Re: [PATCH v3 00/24] pic/ioapic/irqchip cleanups + minor fixes

[Radim Krčmář]

2017-04-07 10:50+0200, 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.

Looks good, applied to kvm/queue to get testing, thanks.

I have noted cosmetical changes for barriers, but they are not big
enough to need a v4 of the series.

> v2 -> v3:
> - Added memory barriers as requested by Paolo to patch 2-4
> 
> 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           | 16 ++++-----
>  arch/x86/kvm/irq.c              |  2 +-
>  arch/x86/kvm/irq.h              | 32 +++++++++---------
>  arch/x86/kvm/irq_comm.c         | 45 ++++++++++++--------------
>  arch/x86/kvm/x86.c              | 45 +++++++++++++-------------
>  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, 117 insertions(+), 146 deletions(-)
> 
> -- 
> 2.9.3
> 

Re: [PATCH v3 00/24] pic/ioapic/irqchip cleanups + minor fixes

[David Hildenbrand]

On 12.04.2017 20:58, Radim Krčmář wrote:
> 2017-04-07 10:50+0200, 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.
> 
> Looks good, applied to kvm/queue to get testing, thanks.
> 
> I have noted cosmetical changes for barriers, but they are not big
> enough to need a v4 of the series.
> 

Thanks, feel free to fix this up if you like. I think this should work
with and without these barriers, so it doesn't matter.

-- 

Thanks,

David