[v5 0/3] KVM: irqchip: synchronize srcu only if needed

[v5 0/3] KVM: irqchip: synchronize srcu only if needed

[Yi Wang]

From: Yi Wang <foxywang@tencent.com>

We found that it may cost more than 20 milliseconds very accidentally
to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
already.

The reason is that when vmm(qemu/CloudHypervisor) invokes
KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
might_sleep and kworker of srcu may cost some delay during this period.
One way makes sence is setup empty irq routing when creating vm and
so that x86/s390 don't need to setup empty/dummy irq routing.

Note: I have no s390 machine so this patch has not been tested
thoroughly on s390 platform. Thanks to Christian for a quick test on
s390 and it still seems to work[1].

Changelog:
----------
v5:
  - rename new function to kvm_init_irq_routing as Sean suggested.
  - move invok of function of setting up empty before kvm_arch_init_vm.
  - Add necessary comment.

v4:
  - replace loop with memset when setup empty irq routing table.

v3:
  - squash setup empty routing function and use of that into one commit
  - drop the comment in s390 part

v2:
  - setup empty irq routing in kvm_create_vm
  - don't setup irq routing in x86 KVM_CAP_SPLIT_IRQCHIP
  - don't setup irq routing in s390 KVM_CREATE_IRQCHIP

v1:
  https://lore.kernel.org/kvm/20240112091128.3868059-1-foxywang@tencent.com/

1. https://lore.kernel.org/lkml/f898e36f-ba02-4c52-a3be-06caac13323e@linux.ibm.com/



Yi Wang (3):
  KVM: setup empty irq routing when create vm
  KVM: x86: don't setup empty irq routing when KVM_CAP_SPLIT_IRQCHIP
  KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

 arch/s390/kvm/kvm-s390.c |  9 +--------
 arch/x86/kvm/irq.h       |  1 -
 arch/x86/kvm/irq_comm.c  |  5 -----
 arch/x86/kvm/x86.c       |  3 ---
 include/linux/kvm_host.h |  2 ++
 virt/kvm/irqchip.c       | 23 +++++++++++++++++++++++
 virt/kvm/kvm_main.c      |  9 ++++++++-
 7 files changed, 34 insertions(+), 18 deletions(-)

-- 
2.39.3

[v5 1/3] KVM: setup empty irq routing when create vm

[Yi Wang]

From: Yi Wang <foxywang@tencent.com>

Add a new function to setup empty irq routing in kvm path, which
can be invoded in non-architecture-specific functions. The difference
compared to the kvm_setup_empty_irq_routing() is this function just
alloc the empty irq routing and does not need synchronize srcu, as
we will call it in kvm_create_vm().

Using the new adding function, we can setup empty irq routing when
kvm_create_vm(), so that x86 and s390 no longer need to set
empty/dummy irq routing when creating an IRQCHIP 'cause it avoid
an synchronize_srcu.

Signed-off-by: Yi Wang <foxywang@tencent.com>
---
 include/linux/kvm_host.h |  2 ++
 virt/kvm/irqchip.c       | 23 +++++++++++++++++++++++
 virt/kvm/kvm_main.c      |  9 ++++++++-
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 48f31dcd318a..a5f12b667ca5 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -2100,6 +2100,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
 			const struct kvm_irq_routing_entry *entries,
 			unsigned nr,
 			unsigned flags);
+int kvm_init_irq_routing(struct kvm *kvm);
 int kvm_set_routing_entry(struct kvm *kvm,
 			  struct kvm_kernel_irq_routing_entry *e,
 			  const struct kvm_irq_routing_entry *ue);
@@ -2108,6 +2109,7 @@ void kvm_free_irq_routing(struct kvm *kvm);
 #else
 
 static inline void kvm_free_irq_routing(struct kvm *kvm) {}
+int kvm_init_irq_routing(struct kvm *kvm) {}
 
 #endif
 
diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 1e567d1f6d3d..ec1fda7fffea 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -237,3 +237,26 @@ int kvm_set_irq_routing(struct kvm *kvm,
 
 	return r;
 }
+
+/*
+ * Alloc empty irq routing.
+ * Called only during vm creation, because we don't synchronize_srcu here.
+ */
+int kvm_init_irq_routing(struct kvm *kvm)
+{
+	struct kvm_irq_routing_table *new;
+	int chip_size;
+
+	new = kzalloc(struct_size(new, map, 1), GFP_KERNEL_ACCOUNT);
+	if (!new)
+		return -ENOMEM;
+
+	new->nr_rt_entries = 1;
+
+	chip_size = sizeof(int) * KVM_NR_IRQCHIPS * KVM_IRQCHIP_NUM_PINS;
+	memset(new->chip, -1, chip_size);
+
+	RCU_INIT_POINTER(kvm->irq_routing, new);
+
+	return 0;
+}
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ff0a20565f90..4100ebdd14fe 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1233,6 +1233,11 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
 		goto out_err_no_irq_srcu;
 
 	refcount_set(&kvm->users_count, 1);
+
+	r = kvm_init_irq_routing(kvm);
+	if (r)
+		goto out_err_no_irq_routing;
+
 	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
 		for (j = 0; j < 2; j++) {
 			slots = &kvm->__memslots[i][j];
@@ -1308,9 +1313,11 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
 out_err_no_disable:
 	kvm_arch_destroy_vm(kvm);
 out_err_no_arch_destroy_vm:
-	WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
 	for (i = 0; i < KVM_NR_BUSES; i++)
 		kfree(kvm_get_bus(kvm, i));
+	kvm_free_irq_routing(kvm);
+out_err_no_irq_routing:
+	WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
 	cleanup_srcu_struct(&kvm->irq_srcu);
 out_err_no_irq_srcu:
 	cleanup_srcu_struct(&kvm->srcu);
-- 
2.39.3

Re: [v5 1/3] KVM: setup empty irq routing when create vm

[Christian Borntraeger]

Am 06.05.24 um 12:17 schrieb Yi Wang:
> From: Yi Wang <foxywang@tencent.com>
> 
> Add a new function to setup empty irq routing in kvm path, which
> can be invoded in non-architecture-specific functions. The difference
> compared to the kvm_setup_empty_irq_routing() is this function just
> alloc the empty irq routing and does not need synchronize srcu, as
> we will call it in kvm_create_vm().
> 
> Using the new adding function, we can setup empty irq routing when
> kvm_create_vm(), so that x86 and s390 no longer need to set
> empty/dummy irq routing when creating an IRQCHIP 'cause it avoid
> an synchronize_srcu.
> 
> Signed-off-by: Yi Wang <foxywang@tencent.com>
Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> ---
>   include/linux/kvm_host.h |  2 ++
>   virt/kvm/irqchip.c       | 23 +++++++++++++++++++++++
>   virt/kvm/kvm_main.c      |  9 ++++++++-
>   3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 48f31dcd318a..a5f12b667ca5 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -2100,6 +2100,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
>   			const struct kvm_irq_routing_entry *entries,
>   			unsigned nr,
>   			unsigned flags);
> +int kvm_init_irq_routing(struct kvm *kvm);
>   int kvm_set_routing_entry(struct kvm *kvm,
>   			  struct kvm_kernel_irq_routing_entry *e,
>   			  const struct kvm_irq_routing_entry *ue);
> @@ -2108,6 +2109,7 @@ void kvm_free_irq_routing(struct kvm *kvm);
>   #else
>   
>   static inline void kvm_free_irq_routing(struct kvm *kvm) {}
> +int kvm_init_irq_routing(struct kvm *kvm) {}
>   
>   #endif
>   
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index 1e567d1f6d3d..ec1fda7fffea 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -237,3 +237,26 @@ int kvm_set_irq_routing(struct kvm *kvm,
>   
>   	return r;
>   }
> +
> +/*
> + * Alloc empty irq routing.
> + * Called only during vm creation, because we don't synchronize_srcu here.
> + */
> +int kvm_init_irq_routing(struct kvm *kvm)
> +{
> +	struct kvm_irq_routing_table *new;
> +	int chip_size;
> +
> +	new = kzalloc(struct_size(new, map, 1), GFP_KERNEL_ACCOUNT);
> +	if (!new)
> +		return -ENOMEM;
> +
> +	new->nr_rt_entries = 1;
> +
> +	chip_size = sizeof(int) * KVM_NR_IRQCHIPS * KVM_IRQCHIP_NUM_PINS;
> +	memset(new->chip, -1, chip_size);
> +
> +	RCU_INIT_POINTER(kvm->irq_routing, new);
> +
> +	return 0;
> +}
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ff0a20565f90..4100ebdd14fe 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1233,6 +1233,11 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
>   		goto out_err_no_irq_srcu;
>   
>   	refcount_set(&kvm->users_count, 1);
> +
> +	r = kvm_init_irq_routing(kvm);
> +	if (r)
> +		goto out_err_no_irq_routing;
> +
>   	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
>   		for (j = 0; j < 2; j++) {
>   			slots = &kvm->__memslots[i][j];
> @@ -1308,9 +1313,11 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
>   out_err_no_disable:
>   	kvm_arch_destroy_vm(kvm);
>   out_err_no_arch_destroy_vm:
> -	WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
>   	for (i = 0; i < KVM_NR_BUSES; i++)
>   		kfree(kvm_get_bus(kvm, i));
> +	kvm_free_irq_routing(kvm);
> +out_err_no_irq_routing:
> +	WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
>   	cleanup_srcu_struct(&kvm->irq_srcu);
>   out_err_no_irq_srcu:
>   	cleanup_srcu_struct(&kvm->srcu);

[v5 2/3] KVM: x86: don't setup empty irq routing when KVM_CAP_SPLIT_IRQCHIP

[Yi Wang]

From: Yi Wang <foxywang@tencent.com>

We found that it may cost more than 20 milliseconds very accidentally
to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
already.

The reason is that when vmm(qemu/CloudHypervisor) invokes
KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
might_sleep and kworker of srcu may cost some delay during this period.

As we have set up empty irq routing when creating vm, so this is no
need now.

Signed-off-by: Yi Wang <foxywang@tencent.com>
---
 arch/x86/kvm/irq.h      | 1 -
 arch/x86/kvm/irq_comm.c | 5 -----
 arch/x86/kvm/x86.c      | 3 ---
 3 files changed, 9 deletions(-)

diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index c2d7cfe82d00..76d46b2f41dd 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -106,7 +106,6 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu);
 int apic_has_pending_timer(struct kvm_vcpu *vcpu);
 
 int kvm_setup_default_irq_routing(struct kvm *kvm);
-int kvm_setup_empty_irq_routing(struct kvm *kvm);
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 			     struct kvm_lapic_irq *irq,
 			     struct dest_map *dest_map);
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 68f3f6c26046..6ee7ca39466e 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -397,11 +397,6 @@ int kvm_setup_default_irq_routing(struct kvm *kvm)
 
 static const struct kvm_irq_routing_entry empty_routing[] = {};
 
-int kvm_setup_empty_irq_routing(struct kvm *kvm)
-{
-	return kvm_set_irq_routing(kvm, empty_routing, 0, 0);
-}
-
 void kvm_arch_post_irq_routing_update(struct kvm *kvm)
 {
 	if (!irqchip_split(kvm))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 91478b769af0..01270182757b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6527,9 +6527,6 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 			goto split_irqchip_unlock;
 		if (kvm->created_vcpus)
 			goto split_irqchip_unlock;
-		r = kvm_setup_empty_irq_routing(kvm);
-		if (r)
-			goto split_irqchip_unlock;
 		/* Pairs with irqchip_in_kernel. */
 		smp_wmb();
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
-- 
2.39.3

Re: [v5 2/3] KVM: x86: don't setup empty irq routing when KVM_CAP_SPLIT_IRQCHIP

[Sean Christopherson]

On Mon, May 06, 2024, Yi Wang wrote:
> From: Yi Wang <foxywang@tencent.com>
> 
> We found that it may cost more than 20 milliseconds very accidentally
> to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
> already.
> 
> The reason is that when vmm(qemu/CloudHypervisor) invokes
> KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
> might_sleep and kworker of srcu may cost some delay during this period.
> 
> As we have set up empty irq routing when creating vm, so this is no
> need now.
> 
> Signed-off-by: Yi Wang <foxywang@tencent.com>
> ---
>  arch/x86/kvm/irq.h      | 1 -
>  arch/x86/kvm/irq_comm.c | 5 -----
>  arch/x86/kvm/x86.c      | 3 ---
>  3 files changed, 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
> index c2d7cfe82d00..76d46b2f41dd 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -106,7 +106,6 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu);
>  int apic_has_pending_timer(struct kvm_vcpu *vcpu);
>  
>  int kvm_setup_default_irq_routing(struct kvm *kvm);
> -int kvm_setup_empty_irq_routing(struct kvm *kvm);
>  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  			     struct kvm_lapic_irq *irq,
>  			     struct dest_map *dest_map);
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 68f3f6c26046..6ee7ca39466e 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -397,11 +397,6 @@ int kvm_setup_default_irq_routing(struct kvm *kvm)
>  
>  static const struct kvm_irq_routing_entry empty_routing[] = {};

empty_routing is unused after this patch and can+should be removed, e.g.

arch/x86/kvm/irq_comm.c:398:43: error: ‘empty_routing’ defined but not used [-Werror=unused-const-variable=]
  398 | static const struct kvm_irq_routing_entry empty_routing[] = {};

I'll fix this up when applying, too.

Re: [v5 2/3] KVM: x86: don't setup empty irq routing when KVM_CAP_SPLIT_IRQCHIP

[Yi Wang]

On Tue, Jun 11, 2024 at 8:25 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, May 06, 2024, Yi Wang wrote:
> > From: Yi Wang <foxywang@tencent.com>
> >
> > We found that it may cost more than 20 milliseconds very accidentally
> > to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
> > already.
> >
> > The reason is that when vmm(qemu/CloudHypervisor) invokes
> > KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
> > might_sleep and kworker of srcu may cost some delay during this period.
> >
> > As we have set up empty irq routing when creating vm, so this is no
> > need now.
> >
> > Signed-off-by: Yi Wang <foxywang@tencent.com>
> > ---
> >  arch/x86/kvm/irq.h      | 1 -
> >  arch/x86/kvm/irq_comm.c | 5 -----
> >  arch/x86/kvm/x86.c      | 3 ---
> >  3 files changed, 9 deletions(-)
> >
> > diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
> > index c2d7cfe82d00..76d46b2f41dd 100644
> > --- a/arch/x86/kvm/irq.h
> > +++ b/arch/x86/kvm/irq.h
> > @@ -106,7 +106,6 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu);
> >  int apic_has_pending_timer(struct kvm_vcpu *vcpu);
> >
> >  int kvm_setup_default_irq_routing(struct kvm *kvm);
> > -int kvm_setup_empty_irq_routing(struct kvm *kvm);
> >  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> >                            struct kvm_lapic_irq *irq,
> >                            struct dest_map *dest_map);
> > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> > index 68f3f6c26046..6ee7ca39466e 100644
> > --- a/arch/x86/kvm/irq_comm.c
> > +++ b/arch/x86/kvm/irq_comm.c
> > @@ -397,11 +397,6 @@ int kvm_setup_default_irq_routing(struct kvm *kvm)
> >
> >  static const struct kvm_irq_routing_entry empty_routing[] = {};
>
> empty_routing is unused after this patch and can+should be removed, e.g.
>
> arch/x86/kvm/irq_comm.c:398:43: error: ‘empty_routing’ defined but not used [-Werror=unused-const-variable=]
>   398 | static const struct kvm_irq_routing_entry empty_routing[] = {};
>
> I'll fix this up when applying, too.


Many thanks for your work, Sean.


---
Best wishes
Yi Wang

[v5 3/3] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

[Yi Wang]

From: Yi Wang <foxywang@tencent.com>

As we have setup empty irq routing in kvm_create_vm(), there's
no need to setup dummy routing when KVM_CREATE_IRQCHIP.

Signed-off-by: Yi Wang <foxywang@tencent.com>
---
 arch/s390/kvm/kvm-s390.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 5147b943a864..ba7fd39bcbf4 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2998,14 +2998,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		break;
 	}
 	case KVM_CREATE_IRQCHIP: {
-		struct kvm_irq_routing_entry routing;
-
-		r = -EINVAL;
-		if (kvm->arch.use_irqchip) {
-			/* Set up dummy routing. */
-			memset(&routing, 0, sizeof(routing));
-			r = kvm_set_irq_routing(kvm, &routing, 0, 0);
-		}
+		r = 0;
 		break;
 	}
 	case KVM_SET_DEVICE_ATTR: {
-- 
2.39.3

Re: [v5 3/3] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

[Christian Borntraeger]

Am 06.05.24 um 12:17 schrieb Yi Wang:
> From: Yi Wang <foxywang@tencent.com>
> 
> As we have setup empty irq routing in kvm_create_vm(), there's
> no need to setup dummy routing when KVM_CREATE_IRQCHIP.
> 
> Signed-off-by: Yi Wang <foxywang@tencent.com>

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>


> ---
>   arch/s390/kvm/kvm-s390.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 5147b943a864..ba7fd39bcbf4 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2998,14 +2998,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>   		break;
>   	}
>   	case KVM_CREATE_IRQCHIP: {
> -		struct kvm_irq_routing_entry routing;
> -
> -		r = -EINVAL;
> -		if (kvm->arch.use_irqchip) {
> -			/* Set up dummy routing. */
> -			memset(&routing, 0, sizeof(routing));
> -			r = kvm_set_irq_routing(kvm, &routing, 0, 0);
> -		}
> +		r = 0;
>   		break;
>   	}
>   	case KVM_SET_DEVICE_ATTR: {

Re: [v5 3/3] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

[Sean Christopherson]

On Mon, May 06, 2024, Yi Wang wrote:
> From: Yi Wang <foxywang@tencent.com>
> 
> As we have setup empty irq routing in kvm_create_vm(), there's
> no need to setup dummy routing when KVM_CREATE_IRQCHIP.
> 
> Signed-off-by: Yi Wang <foxywang@tencent.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 5147b943a864..ba7fd39bcbf4 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2998,14 +2998,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  		break;
>  	}
>  	case KVM_CREATE_IRQCHIP: {
> -		struct kvm_irq_routing_entry routing;
> -
> -		r = -EINVAL;
> -		if (kvm->arch.use_irqchip) {
> -			/* Set up dummy routing. */
> -			memset(&routing, 0, sizeof(routing));
> -			r = kvm_set_irq_routing(kvm, &routing, 0, 0);
> -		}
> +		r = 0;

This is wrong, KVM_CREATE_IRQCHIP should fail with -EINVAL if kvm->arch.use_irqchip
is false.

There's also a functional change here, though I highly doubt it negatively affects
userspace.  Nothing in s390 prevents invoking KVM_CREATE_IRQCHIP after
KVM_SET_GSI_ROUTING, so userspace could very theoretically use KVM_CREATE_IRQCHIP
to reset to empty IRQ routing.

Christian, if it works for you, I'll massage it to this when applying.

--
From: Yi Wang <foxywang@tencent.com>
Date: Mon, 6 May 2024 18:17:51 +0800
Subject: [PATCH] KVM: s390: Don't re-setup dummy routing when
 KVM_CREATE_IRQCHIP

Now that KVM sets up empty irq routing in kvm_create_vm(), there's
no need to setup dummy routing when KVM_CREATE_IRQCHIP.

Note, userspace could very theoretically use KVM_CREATE_IRQCHIP after
KVM_SET_GSI_ROUTING to reset to empty IRQ routing, but it's extremely
unlikely any VMM does that, e.g. the main reason s390 does anything for
KVM_CREATE_IRQCHIP is to that s390 doesn't need to be special cased by the
VMM.

Signed-off-by: Yi Wang <foxywang@tencent.com>
Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Link: https://lore.kernel.org/r/20240506101751.3145407-4-foxywang@tencent.com
[sean: keep use_irqchip check, call out KVM_SET_GSI_ROUTING impact]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/s390/kvm/kvm-s390.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 82e9631cd9ef..4641083ee100 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2996,14 +2996,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		break;
 	}
 	case KVM_CREATE_IRQCHIP: {
-		struct kvm_irq_routing_entry routing;
-
 		r = -EINVAL;
-		if (kvm->arch.use_irqchip) {
-			/* Set up dummy routing. */
-			memset(&routing, 0, sizeof(routing));
-			r = kvm_set_irq_routing(kvm, &routing, 0, 0);
-		}
+		if (kvm->arch.use_irqchip)
+			r = 0;
 		break;
 	}
 	case KVM_SET_DEVICE_ATTR: {

base-commit: 9a859becf1b7f6879466e8c0ebee492b236f2080
-- 

Re: [v5 3/3] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP

[Christian Borntraeger]

Am 10.06.24 um 18:19 schrieb Sean Christopherson:
> On Mon, May 06, 2024, Yi Wang wrote:
>> From: Yi Wang <foxywang@tencent.com>
>>
>> As we have setup empty irq routing in kvm_create_vm(), there's
>> no need to setup dummy routing when KVM_CREATE_IRQCHIP.
>>
>> Signed-off-by: Yi Wang <foxywang@tencent.com>
>> ---
>>   arch/s390/kvm/kvm-s390.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 5147b943a864..ba7fd39bcbf4 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2998,14 +2998,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>>   		break;
>>   	}
>>   	case KVM_CREATE_IRQCHIP: {
>> -		struct kvm_irq_routing_entry routing;
>> -
>> -		r = -EINVAL;
>> -		if (kvm->arch.use_irqchip) {
>> -			/* Set up dummy routing. */
>> -			memset(&routing, 0, sizeof(routing));
>> -			r = kvm_set_irq_routing(kvm, &routing, 0, 0);
>> -		}
>> +		r = 0;
> 
> This is wrong, KVM_CREATE_IRQCHIP should fail with -EINVAL if kvm->arch.use_irqchip
> is false.
> 
> There's also a functional change here, though I highly doubt it negatively affects
> userspace.  Nothing in s390 prevents invoking KVM_CREATE_IRQCHIP after
> KVM_SET_GSI_ROUTING, so userspace could very theoretically use KVM_CREATE_IRQCHIP
> to reset to empty IRQ routing.
> 
> Christian, if it works for you, I'll massage it to this when applying.

Oops. Yes please do so.

Re: [v5 0/3] KVM: irqchip: synchronize srcu only if needed

[Sean Christopherson]

On Mon, 06 May 2024 18:17:48 +0800, Yi Wang wrote:
> From: Yi Wang <foxywang@tencent.com>
> 
> We found that it may cost more than 20 milliseconds very accidentally
> to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
> already.
> 
> The reason is that when vmm(qemu/CloudHypervisor) invokes
> KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
> might_sleep and kworker of srcu may cost some delay during this period.
> One way makes sence is setup empty irq routing when creating vm and
> so that x86/s390 don't need to setup empty/dummy irq routing.
> 
> [...]

Applied to kvm-x86 generic, with a lot of fixup.  A sanity check on the end
result would be much appreciated.  Thanks!

[1/3] KVM: Setup empty IRQ routing when creating a VM
      https://github.com/kvm-x86/linux/commit/fbe4a7e881d4
[2/3] KVM: x86: Don't re-setup empty IRQ routing when KVM_CAP_SPLIT_IRQCHIP
      https://github.com/kvm-x86/linux/commit/e3c89f5dd11d
[3/3] KVM: s390: Don't re-setup dummy routing when KVM_CREATE_IRQCHIP
      https://github.com/kvm-x86/linux/commit/c4201bd24f4a

--
https://github.com/kvm-x86/linux/tree/next

Re: [v5 0/3] KVM: irqchip: synchronize srcu only if needed

[Yi Wang]

On Wed, Jun 12, 2024 at 9:19 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, 06 May 2024 18:17:48 +0800, Yi Wang wrote:
> > From: Yi Wang <foxywang@tencent.com>
> >
> > We found that it may cost more than 20 milliseconds very accidentally
> > to enable cap of KVM_CAP_SPLIT_IRQCHIP on a host which has many vms
> > already.
> >
> > The reason is that when vmm(qemu/CloudHypervisor) invokes
> > KVM_CAP_SPLIT_IRQCHIP kvm will call synchronize_srcu_expedited() and
> > might_sleep and kworker of srcu may cost some delay during this period.
> > One way makes sence is setup empty irq routing when creating vm and
> > so that x86/s390 don't need to setup empty/dummy irq routing.
> >
> > [...]
>
> Applied to kvm-x86 generic, with a lot of fixup.  A sanity check on the end
> result would be much appreciated.  Thanks!

The code LGTM. I did some tests in my Intel machine and found no problems.

Thanks for your work :)

>
> [1/3] KVM: Setup empty IRQ routing when creating a VM
>       https://github.com/kvm-x86/linux/commit/fbe4a7e881d4
> [2/3] KVM: x86: Don't re-setup empty IRQ routing when KVM_CAP_SPLIT_IRQCHIP
>       https://github.com/kvm-x86/linux/commit/e3c89f5dd11d
> [3/3] KVM: s390: Don't re-setup dummy routing when KVM_CREATE_IRQCHIP
>       https://github.com/kvm-x86/linux/commit/c4201bd24f4a
>
> --
> https://github.com/kvm-x86/linux/tree/next




---
Best wishes
Yi Wang