| Message ID | 20211103081657.1945-5-longpeng2@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | optimize the downtime for vfio migration | expand |
Hi Paolo, Ping... Do you have any suggestions about this change ? It seems Alex has no objection on this series now, but we need your ACK, thanks. > -----Original Message----- > From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > Sent: Wednesday, November 3, 2021 4:17 PM > To: alex.williamson@redhat.com; pbonzini@redhat.com > Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; Gonglei (Arei) > <arei.gonglei@huawei.com>; Longpeng (Mike, Cloud Infrastructure Service > Product Dept.) <longpeng2@huawei.com> > Subject: [PATCH v5 4/6] kvm: irqchip: extract > kvm_irqchip_add_deferred_msi_route > > Extract a common helper that add MSI route for specific vector > but does not commit immediately. > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > --- > accel/kvm/kvm-all.c | 15 +++++++++++++-- > include/sysemu/kvm.h | 6 ++++++ > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index db8d83b..8627f7c 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -1953,7 +1953,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg) > return kvm_set_irq(s, route->kroute.gsi, 1); > } > > -int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) > +int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, PCIDevice > *dev) > { > struct kvm_irq_routing_entry kroute = {}; > int virq; > @@ -1996,7 +1996,18 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, > PCIDevice *dev) > > kvm_add_routing_entry(s, &kroute); > kvm_arch_add_msi_route_post(&kroute, vector, dev); > - kvm_irqchip_commit_routes(s); > + > + return virq; > +} > + > +int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) > +{ > + int virq; > + > + virq = kvm_irqchip_add_deferred_msi_route(s, vector, dev); > + if (virq >= 0) { > + kvm_irqchip_commit_routes(s); > + } > > return virq; > } > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index a1ab1ee..8de0d9a 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -476,6 +476,12 @@ void kvm_init_cpu_signals(CPUState *cpu); > * @return: virq (>=0) when success, errno (<0) when failed. > */ > int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev); > +/** > + * Add MSI route for specific vector but does not commit to KVM > + * immediately > + */ > +int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, > + PCIDevice *dev); > int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg, > PCIDevice *dev); > void kvm_irqchip_commit_routes(KVMState *s); > -- > 1.8.3.1
On 11/3/21 09:16, Longpeng(Mike) wrote: > Extract a common helper that add MSI route for specific vector > but does not commit immediately. > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> I think adding the new function is not necessary; I have no problem moving the call to kvm_irqchip_commit_routes to the callers. Perhaps you can have an API like this: typedef struct KVMRouteChange { KVMState *s; int changes; } KVMRouteChange; KVMRouteChange kvm_irqchip_begin_route_changes(KVMState *s) { return (KVMRouteChange) { .s = s, .changes = 0 }; } void kvm_irqchip_commit_route_changes(KVMRouteChange *c) { if (c->changes) { kvm_irqchip_commit_routes(c->s); c->changes = 0; } } int kvm_irqchip_add_msi_route(KVMRouteChange *c, int vector, PCIDevice *dev) { KVMState *s = c->s; ... kvm_add_routing_entry(s, &kroute); kvm_arch_add_msi_route_post(&kroute, vector, dev); c->changes++; return virq; } so it's harder for the callers to "forget" kvm_irqchip_commit_route_changes. Paolo > --- > accel/kvm/kvm-all.c | 15 +++++++++++++-- > include/sysemu/kvm.h | 6 ++++++ > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index db8d83b..8627f7c 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -1953,7 +1953,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg) > return kvm_set_irq(s, route->kroute.gsi, 1); > } > > -int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) > +int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, PCIDevice *dev) > { > struct kvm_irq_routing_entry kroute = {}; > int virq; > @@ -1996,7 +1996,18 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) > > kvm_add_routing_entry(s, &kroute); > kvm_arch_add_msi_route_post(&kroute, vector, dev); > - kvm_irqchip_commit_routes(s); > + > + return virq; > +} > + > +int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) > +{ > + int virq; > + > + virq = kvm_irqchip_add_deferred_msi_route(s, vector, dev); > + if (virq >= 0) { > + kvm_irqchip_commit_routes(s); > + } > > return virq; > } > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index a1ab1ee..8de0d9a 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -476,6 +476,12 @@ void kvm_init_cpu_signals(CPUState *cpu); > * @return: virq (>=0) when success, errno (<0) when failed. > */ > int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev); > +/** > + * Add MSI route for specific vector but does not commit to KVM > + * immediately > + */ > +int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, > + PCIDevice *dev); > int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg, > PCIDevice *dev); > void kvm_irqchip_commit_routes(KVMState *s); >
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Friday, November 12, 2021 5:32 PM > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > <longpeng2@huawei.com>; alex.williamson@redhat.com > Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; Gonglei (Arei) > <arei.gonglei@huawei.com> > Subject: Re: [PATCH v5 4/6] kvm: irqchip: extract > kvm_irqchip_add_deferred_msi_route > > On 11/3/21 09:16, Longpeng(Mike) wrote: > > Extract a common helper that add MSI route for specific vector > > but does not commit immediately. > > > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > > I think adding the new function is not necessary; I have no problem > moving the call to kvm_irqchip_commit_routes to the callers. Perhaps > you can have an API like this: > > typedef struct KVMRouteChange { > KVMState *s; > int changes; > } KVMRouteChange; > > KVMRouteChange kvm_irqchip_begin_route_changes(KVMState *s) > { > return (KVMRouteChange) { .s = s, .changes = 0 }; > } > > void kvm_irqchip_commit_route_changes(KVMRouteChange *c) > { > if (c->changes) { > kvm_irqchip_commit_routes(c->s); > c->changes = 0; > } > } > > int kvm_irqchip_add_msi_route(KVMRouteChange *c, int vector, PCIDevice *dev) > { > KVMState *s = c->s; > ... > kvm_add_routing_entry(s, &kroute); > kvm_arch_add_msi_route_post(&kroute, vector, dev); > c->changes++; > > return virq; > } > > so it's harder for the callers to "forget" kvm_irqchip_commit_route_changes. > Make sense. We have 4 adding functions currently, the first two trigger committing inside and the others do not. 1. kvm_irqchip_add_adapter_route (commit inside) 2. kvm_irqchip_add_msi_route (commit inside) 3. kvm_irqchip_add_irq_route (commit outside) 4. kvm_irqchip_add_hv_sint_route (commit outside) How about just moving the kvm_irqchip_commit_routes() out of kvm_irqchip_add_msi_route() in this series and implement the solution you suggested in another series ? I think we should apply the solution to s390_adapter routing type and updating paths. > Paolo > > > --- > > accel/kvm/kvm-all.c | 15 +++++++++++++-- > > include/sysemu/kvm.h | 6 ++++++ > > 2 files changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index db8d83b..8627f7c 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -1953,7 +1953,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg) > > return kvm_set_irq(s, route->kroute.gsi, 1); > > } > > > > -int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) > > +int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, PCIDevice > *dev) > > { > > struct kvm_irq_routing_entry kroute = {}; > > int virq; > > @@ -1996,7 +1996,18 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, > PCIDevice *dev) > > > > kvm_add_routing_entry(s, &kroute); > > kvm_arch_add_msi_route_post(&kroute, vector, dev); > > - kvm_irqchip_commit_routes(s); > > + > > + return virq; > > +} > > + > > +int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) > > +{ > > + int virq; > > + > > + virq = kvm_irqchip_add_deferred_msi_route(s, vector, dev); > > + if (virq >= 0) { > > + kvm_irqchip_commit_routes(s); > > + } > > > > return virq; > > } > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > > index a1ab1ee..8de0d9a 100644 > > --- a/include/sysemu/kvm.h > > +++ b/include/sysemu/kvm.h > > @@ -476,6 +476,12 @@ void kvm_init_cpu_signals(CPUState *cpu); > > * @return: virq (>=0) when success, errno (<0) when failed. > > */ > > int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev); > > +/** > > + * Add MSI route for specific vector but does not commit to KVM > > + * immediately > > + */ > > +int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, > > + PCIDevice *dev); > > int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg, > > PCIDevice *dev); > > void kvm_irqchip_commit_routes(KVMState *s); > >
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index db8d83b..8627f7c 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -1953,7 +1953,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg) return kvm_set_irq(s, route->kroute.gsi, 1); } -int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) +int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, PCIDevice *dev) { struct kvm_irq_routing_entry kroute = {}; int virq; @@ -1996,7 +1996,18 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) kvm_add_routing_entry(s, &kroute); kvm_arch_add_msi_route_post(&kroute, vector, dev); - kvm_irqchip_commit_routes(s); + + return virq; +} + +int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev) +{ + int virq; + + virq = kvm_irqchip_add_deferred_msi_route(s, vector, dev); + if (virq >= 0) { + kvm_irqchip_commit_routes(s); + } return virq; } diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index a1ab1ee..8de0d9a 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -476,6 +476,12 @@ void kvm_init_cpu_signals(CPUState *cpu); * @return: virq (>=0) when success, errno (<0) when failed. */ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev); +/** + * Add MSI route for specific vector but does not commit to KVM + * immediately + */ +int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, + PCIDevice *dev); int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg, PCIDevice *dev); void kvm_irqchip_commit_routes(KVMState *s);
Extract a common helper that add MSI route for specific vector but does not commit immediately. Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> --- accel/kvm/kvm-all.c | 15 +++++++++++++-- include/sysemu/kvm.h | 6 ++++++ 2 files changed, 19 insertions(+), 2 deletions(-)