> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@xxxxxxxxxx] > Sent: Friday, November 12, 2021 5:32 PM > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > <longpeng2@xxxxxxxxxx>; alex.williamson@xxxxxxxxxx > Cc: qemu-devel@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Gonglei (Arei) > <arei.gonglei@xxxxxxxxxx> > 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@xxxxxxxxxx> > > 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); > >
