| Message ID | 1436780855-3617-16-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Feng, On 07/13/2015 11:47 AM, Feng Wu wrote: > From: Eric Auger <eric.auger@linaro.org> > > This patch adds the registration/unregistration of an > irq_bypass_consumer on irqfd assignment/deassignment. > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > Signed-off-by: Feng Wu <feng.wu@intel.com> > --- > virt/kvm/eventfd.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c > index 647ffb8..4225eea 100644 > --- a/virt/kvm/eventfd.c > +++ b/virt/kvm/eventfd.c > @@ -35,6 +35,7 @@ > #include <linux/srcu.h> > #include <linux/slab.h> > #include <linux/seqlock.h> > +#include <linux/irqbypass.h> > #include <trace/events/kvm.h> > > #include <kvm/iodev.h> > @@ -140,6 +141,7 @@ irqfd_shutdown(struct work_struct *work) > /* > * It is now safe to release the object's resources > */ > + irq_bypass_unregister_consumer(&irqfd->consumer); > eventfd_ctx_put(irqfd->eventfd); > kfree(irqfd); > } > @@ -380,6 +382,11 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args) > */ > fdput(f); > > + irqfd->consumer.token = (void *)irqfd->eventfd; > + kvm_arch_irq_consumer_init(&irqfd->consumer); what if the architecture does not implement kvm_arch_irq_consumer_init? Also you are using here this single function kvm_arch_irq_consumer_init to do some irq bypass manager settings + attaching your irqfd->arch_update cb which does not really relate to IRQ bypass manager. I think I preferred the approach where start/top/add/del were exposed separately ([RFC v2 5/6] KVM: introduce kvm_arch functions for IRQ bypass). Why not adding another kvm_arch_irq_routing_update then, not necessarily linked to irq bypass manager. Best Regards Eric > + ret = irq_bypass_register_consumer(&irqfd->consumer); > + WARN_ON(ret); > + > return 0; > > fail: > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
13/07/2015 15:16, Eric Auger wrote: >> > >> > + irqfd->consumer.token = (void *)irqfd->eventfd; >> > + kvm_arch_irq_consumer_init(&irqfd->consumer); > what if the architecture does not implement kvm_arch_irq_consumer_init? > > Also you are using here this single function kvm_arch_irq_consumer_init > to do some irq bypass manager settings + attaching your > irqfd->arch_update cb which does not really relate to IRQ bypass > manager. I think I preferred the approach where start/top/add/del were > exposed separately ([RFC v2 5/6] KVM: introduce kvm_arch functions for > IRQ bypass). > > Why not adding another kvm_arch_irq_routing_update then, not necessarily > linked to irq bypass manager. Yes, I also preferred the dummy kvm_arch_* functions to this approach with an init function. You'd have to add dummy init functions anyway for non-ARM, non-x86 architectures. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Monday, July 13, 2015 9:47 PM > To: Eric Auger; Wu, Feng; kvm@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: alex.williamson@redhat.com; joro@8bytes.org > Subject: Re: [v5 15/19] KVM: eventfd: add irq bypass consumer management > > 13/07/2015 15:16, Eric Auger wrote: > >> > > >> > + irqfd->consumer.token = (void *)irqfd->eventfd; > >> > + kvm_arch_irq_consumer_init(&irqfd->consumer); > > what if the architecture does not implement kvm_arch_irq_consumer_init? > > > > Also you are using here this single function kvm_arch_irq_consumer_init > > to do some irq bypass manager settings + attaching your > > irqfd->arch_update cb which does not really relate to IRQ bypass > > manager. I think I preferred the approach where start/top/add/del were > > exposed separately ([RFC v2 5/6] KVM: introduce kvm_arch functions for > > IRQ bypass). > > > > Why not adding another kvm_arch_irq_routing_update then, not necessarily > > linked to irq bypass manager. > > Yes, I also preferred the dummy kvm_arch_* functions to this approach > with an init function. You'd have to add dummy init functions anyway > for non-ARM, non-x86 architectures. I think dummy kvm_arch_* is okay for me. However, my point is that currently 'add_producer ' and 'del_producer ' are mandatory, other callbacks are optional. In patch "[RFC v2 5/6] KVM: introduce kvm_arch functions for IRQ bypass " and "[RFC v2 6/6] KVM: eventfd: add irq bypass consumer management ", it provides all the callbacks, which means we need to implement dummy arch specific functions no matter it is necessary. In that case, seems it is pointless to make some of the callbacks optional. Anyway, if you guys are fine with the dummy approach, I am good! :) Thanks, Feng > > Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c index 647ffb8..4225eea 100644 --- a/virt/kvm/eventfd.c +++ b/virt/kvm/eventfd.c @@ -35,6 +35,7 @@ #include <linux/srcu.h> #include <linux/slab.h> #include <linux/seqlock.h> +#include <linux/irqbypass.h> #include <trace/events/kvm.h> #include <kvm/iodev.h> @@ -140,6 +141,7 @@ irqfd_shutdown(struct work_struct *work) /* * It is now safe to release the object's resources */ + irq_bypass_unregister_consumer(&irqfd->consumer); eventfd_ctx_put(irqfd->eventfd); kfree(irqfd); } @@ -380,6 +382,11 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args) */ fdput(f); + irqfd->consumer.token = (void *)irqfd->eventfd; + kvm_arch_irq_consumer_init(&irqfd->consumer); + ret = irq_bypass_register_consumer(&irqfd->consumer); + WARN_ON(ret); + return 0; fail: