| Message ID | 1435843047-6327-13-git-send-email-eric.auger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/07/2015 15:17, Eric Auger wrote: > - new fields are added on producer side: linux irq, vfio_device handle, > active which reflects whether the source is active (at interrupt > controller level or at VFIO level - automasked -) and finally an > opaque pointer which will be used to point to the vfio_platform_device > in this series. Linux IRQ and active should be okay. As to the vfio_device handle, you should link it from the vfio_platform_device instead. And for the vfio_platform_device, you can link it from the vfio_platform_irq instead. Once you've done this, embed the irq_bypass_producer struct in the vfio_platform_irq struct; in the new kvm_arch_* functions, go back to the vfio_platform_irq struct via container_of. From there you can retrieve pointers to the vfio_platform_device and the vfio_device. > - new fields on consumer side: the kvm handle, the gsi You do not need to add these. Instead, add the kvm handle to irqfd only. Like above, embed the irq_bypass_consumer struct in the irqfd struct; in the new kvm_arch_* functions, go back to the vfio_platform_irq struct via container_of. 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: Thursday, July 02, 2015 9:41 PM > To: Eric Auger; eric.auger@st.com; linux-arm-kernel@lists.infradead.org; > kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; > christoffer.dall@linaro.org; marc.zyngier@arm.com; > alex.williamson@redhat.com; avi.kivity@gmail.com; mtosatti@redhat.com; > Wu, Feng; joro@8bytes.org; b.reynal@virtualopensystems.com > Cc: linux-kernel@vger.kernel.org; patches@linaro.org > Subject: Re: [RFC 12/17] irq: bypass: Extend skeleton for ARM forwarding > control > > > > On 02/07/2015 15:17, Eric Auger wrote: > > - new fields are added on producer side: linux irq, vfio_device handle, > > active which reflects whether the source is active (at interrupt > > controller level or at VFIO level - automasked -) and finally an > > opaque pointer which will be used to point to the vfio_platform_device > > in this series. > > Linux IRQ and active should be okay. As to the vfio_device handle, you > should link it from the vfio_platform_device instead. And for the > vfio_platform_device, you can link it from the vfio_platform_irq instead. > > Once you've done this, embed the irq_bypass_producer struct in the > vfio_platform_irq struct; in the new kvm_arch_* functions, go back to > the vfio_platform_irq struct via container_of. From there you can > retrieve pointers to the vfio_platform_device and the vfio_device. > > > - new fields on consumer side: the kvm handle, the gsi > > You do not need to add these. Instead, add the kvm handle to irqfd > only. Like above, embed the irq_bypass_consumer struct in the irqfd > struct; in the new kvm_arch_* functions, go back to the > vfio_platform_irq struct via container_of. > I also need the gsi field here, for posted-interrupts, I need 'gsi', 'irq' to update the IRTE. 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
> -----Original Message----- > From: Wu, Feng > Sent: Friday, July 03, 2015 10:20 AM > To: Paolo Bonzini; Eric Auger; eric.auger@st.com; > linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; > alex.williamson@redhat.com; avi.kivity@gmail.com; mtosatti@redhat.com; > joro@8bytes.org; b.reynal@virtualopensystems.com > Cc: linux-kernel@vger.kernel.org; patches@linaro.org; Wu, Feng > Subject: RE: [RFC 12/17] irq: bypass: Extend skeleton for ARM forwarding > control > > > > > -----Original Message----- > > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > > Sent: Thursday, July 02, 2015 9:41 PM > > To: Eric Auger; eric.auger@st.com; linux-arm-kernel@lists.infradead.org; > > kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; > > christoffer.dall@linaro.org; marc.zyngier@arm.com; > > alex.williamson@redhat.com; avi.kivity@gmail.com; mtosatti@redhat.com; > > Wu, Feng; joro@8bytes.org; b.reynal@virtualopensystems.com > > Cc: linux-kernel@vger.kernel.org; patches@linaro.org > > Subject: Re: [RFC 12/17] irq: bypass: Extend skeleton for ARM forwarding > > control > > > > > > > > On 02/07/2015 15:17, Eric Auger wrote: > > > - new fields are added on producer side: linux irq, vfio_device handle, > > > active which reflects whether the source is active (at interrupt > > > controller level or at VFIO level - automasked -) and finally an > > > opaque pointer which will be used to point to the vfio_platform_device > > > in this series. > > > > Linux IRQ and active should be okay. As to the vfio_device handle, you > > should link it from the vfio_platform_device instead. And for the > > vfio_platform_device, you can link it from the vfio_platform_irq instead. > > > > Once you've done this, embed the irq_bypass_producer struct in the > > vfio_platform_irq struct; in the new kvm_arch_* functions, go back to > > the vfio_platform_irq struct via container_of. From there you can > > retrieve pointers to the vfio_platform_device and the vfio_device. > > > > > - new fields on consumer side: the kvm handle, the gsi > > > > You do not need to add these. Instead, add the kvm handle to irqfd > > only. Like above, embed the irq_bypass_consumer struct in the irqfd > > struct; in the new kvm_arch_* functions, go back to the > > vfio_platform_irq struct via container_of. > > > > I also need the gsi field here, for posted-interrupts, I need 'gsi', 'irq' to > update the IRTE. Oh... we can get gsi from irq_bypass_consumer -> _irqfd -> gsi, so it is not needed in irq_bypass_consumer. Got it! :) Thanks, Feng > > 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
> -----Original Message----- > From: Eric Auger [mailto:eric.auger@linaro.org] > Sent: Thursday, July 02, 2015 9:17 PM > To: eric.auger@st.com; eric.auger@linaro.org; > linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; > alex.williamson@redhat.com; pbonzini@redhat.com; avi.kivity@gmail.com; > mtosatti@redhat.com; Wu, Feng; joro@8bytes.org; > b.reynal@virtualopensystems.com > Cc: linux-kernel@vger.kernel.org; patches@linaro.org > Subject: [RFC 12/17] irq: bypass: Extend skeleton for ARM forwarding control > > - [add,del]_[consumer,producer] updated to takes both the consumer and > producer handles. This is requested to combine info from both, > typically to link the source irq owned by the producer with the gsi > owned by the consumer (forwarded IRQ setup). > - new functions are added: [stop,resume]_[consumer, producer]. Those are > needed for forwarding since the state change requires to entermingle > actions at consumer, producer. > - On handshake, we now call connect, disconnect which features the more > complex sequence. > - new fields are added on producer side: linux irq, vfio_device handle, > active which reflects whether the source is active (at interrupt > controller level or at VFIO level - automasked -) and finally an > opaque pointer which will be used to point to the vfio_platform_device > in this series. > - new fields on consumer side: the kvm handle, the gsi > > Integration of posted interrupt series will help to refine those choices On PI side, I need another filed as below, struct irq_bypass_consumer { struct list_head node; void *token; + unsigned irq; /*got from producer when registered*/ void (*add_producer)(struct irq_bypass_producer *, struct irq_bypass_consumer *); void (*del_producer)(struct irq_bypass_producer *, struct irq_bypass_consumer *); + void (*update)(struct irq_bypass_consumer *); }; 'update' is used to update the IRTE, while irq is initialized when registered, which is used to find the right IRTE. Thanks, Feng > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > > --- > > - connect/disconnect could become a cb too. For forwarding it may make > sense to have failure at connection: this would happen when the physical > IRQ is either active at irqchip level or VFIO masked. This means some > of the cb should return an error and this error management could be > prod/cons specific. Where to attach the connect/disconnect cb: to the > cons or prod, to both? > - Hence may be sensible to do the list_add only if connect returns 0 > - disconnect would not be allowed to fail. > --- > include/linux/irqbypass.h | 26 ++++++++++++++++++++++--- > kernel/irq/bypass.c | 48 > +++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 67 insertions(+), 7 deletions(-) > > diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h > index 718508e..591ae3f 100644 > --- a/include/linux/irqbypass.h > +++ b/include/linux/irqbypass.h > @@ -3,17 +3,37 @@ > > #include <linux/list.h> > > +struct vfio_device; > +struct irq_bypass_consumer; > +struct kvm; > + > struct irq_bypass_producer { > struct list_head node; > void *token; > - /* TBD */ > + unsigned int irq; /* host physical irq */ > + struct vfio_device *vdev; /* vfio device that requested irq */ > + /* is irq active at irqchip or VFIO masked? */ > + bool active; > + void *opaque; > + void (*stop_producer)(struct irq_bypass_producer *); > + void (*resume_producer)(struct irq_bypass_producer *); > + void (*add_consumer)(struct irq_bypass_producer *, > + struct irq_bypass_consumer *); > + void (*del_consumer)(struct irq_bypass_producer *, > + struct irq_bypass_consumer *); > }; > > struct irq_bypass_consumer { > struct list_head node; > void *token; > - void (*add_producer)(struct irq_bypass_producer *); > - void (*del_producer)(struct irq_bypass_producer *); > + unsigned int gsi; /* the guest gsi */ > + struct kvm *kvm; > + void (*stop_consumer)(struct irq_bypass_consumer *); > + void (*resume_consumer)(struct irq_bypass_consumer *); > + void (*add_producer)(struct irq_bypass_consumer *, > + struct irq_bypass_producer *); > + void (*del_producer)(struct irq_bypass_consumer *, > + struct irq_bypass_producer *); > }; > > int irq_bypass_register_producer(struct irq_bypass_producer *); > diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c > index 5d0f92b..fb31fef 100644 > --- a/kernel/irq/bypass.c > +++ b/kernel/irq/bypass.c > @@ -19,6 +19,46 @@ static LIST_HEAD(producers); > static LIST_HEAD(consumers); > static DEFINE_MUTEX(lock); > > +/* lock must be hold when calling connect */ > +static void connect(struct irq_bypass_producer *prod, > + struct irq_bypass_consumer *cons) > +{ > + pr_info("++++ %s prod(%d) -> cons(%d)\n", > + __func__, prod->irq, cons->gsi); > + if (prod->stop_producer) > + prod->stop_producer(prod); > + if (cons->stop_consumer) > + cons->stop_consumer(cons); > + if (prod->add_consumer) > + prod->add_consumer(prod, cons); > + if (cons->add_producer) > + cons->add_producer(cons, prod); > + if (cons->resume_consumer) > + cons->resume_consumer(cons); > + if (prod->resume_producer) > + prod->resume_producer(prod); > +} > + > +/* lock must be hold when calling disconnect */ > +static void disconnect(struct irq_bypass_producer *prod, > + struct irq_bypass_consumer *cons) > +{ > + pr_info("---- %s prod(%d) -> cons(%d)\n", > + __func__, prod->irq, cons->gsi); > + if (prod->stop_producer) > + prod->stop_producer(prod); > + if (cons->stop_consumer) > + cons->stop_consumer(cons); > + if (cons->del_producer) > + cons->del_producer(cons, prod); > + if (prod->del_consumer) > + prod->del_consumer(prod, cons); > + if (cons->resume_consumer) > + cons->resume_consumer(cons); > + if (prod->resume_producer) > + prod->resume_producer(prod); > +} > + > int irq_bypass_register_producer(struct irq_bypass_producer *producer) > { > struct irq_bypass_producer *tmp; > @@ -38,7 +78,7 @@ int irq_bypass_register_producer(struct > irq_bypass_producer *producer) > > list_for_each_entry(consumer, &consumers, node) { > if (consumer->token == producer->token) { > - consumer->add_producer(producer); > + connect(producer, consumer); > break; > } > } > @@ -56,7 +96,7 @@ void irq_bypass_unregister_producer(struct > irq_bypass_producer *producer) > > list_for_each_entry(consumer, &consumers, node) { > if (consumer->token == producer->token) { > - consumer->del_producer(producer); > + disconnect(producer, consumer); > break; > } > } > @@ -86,7 +126,7 @@ int irq_bypass_register_consumer(struct > irq_bypass_consumer *consumer) > > list_for_each_entry(producer, &producers, node) { > if (producer->token == consumer->token) { > - consumer->add_producer(producer); > + connect(producer, consumer); > break; > } > } > @@ -104,7 +144,7 @@ void irq_bypass_unregister_consumer(struct > irq_bypass_consumer *consumer) > > list_for_each_entry(producer, &producers, node) { > if (producer->token == consumer->token) { > - consumer->del_producer(producer); > + disconnect(producer, consumer); > break; > } > } > -- > 1.9.1 -- 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
On 03/07/2015 04:43, Wu, Feng wrote: > > struct irq_bypass_consumer { > struct list_head node; > void *token; > + unsigned irq; /*got from producer when registered*/ > void (*add_producer)(struct irq_bypass_producer *, > struct irq_bypass_consumer *); > void (*del_producer)(struct irq_bypass_producer *, > struct irq_bypass_consumer *); > + void (*update)(struct irq_bypass_consumer *); > }; > > 'update' is used to update the IRTE, while irq is initialized when > registered, which is used to find the right IRTE. Feel free to add "update" in your PI patches. I am not sure if "irq" belongs here or in the containing struct. You can play with both and submit the version that looks better to you. 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
Paolo, On 07/03/2015 04:24 AM, Wu, Feng wrote: > > >> -----Original Message----- >> From: Wu, Feng >> Sent: Friday, July 03, 2015 10:20 AM >> To: Paolo Bonzini; Eric Auger; eric.auger@st.com; >> linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; >> kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; >> alex.williamson@redhat.com; avi.kivity@gmail.com; mtosatti@redhat.com; >> joro@8bytes.org; b.reynal@virtualopensystems.com >> Cc: linux-kernel@vger.kernel.org; patches@linaro.org; Wu, Feng >> Subject: RE: [RFC 12/17] irq: bypass: Extend skeleton for ARM forwarding >> control >> >> >> >>> -----Original Message----- >>> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >>> Sent: Thursday, July 02, 2015 9:41 PM >>> To: Eric Auger; eric.auger@st.com; linux-arm-kernel@lists.infradead.org; >>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; >>> christoffer.dall@linaro.org; marc.zyngier@arm.com; >>> alex.williamson@redhat.com; avi.kivity@gmail.com; mtosatti@redhat.com; >>> Wu, Feng; joro@8bytes.org; b.reynal@virtualopensystems.com >>> Cc: linux-kernel@vger.kernel.org; patches@linaro.org >>> Subject: Re: [RFC 12/17] irq: bypass: Extend skeleton for ARM forwarding >>> control >>> >>> >>> >>> On 02/07/2015 15:17, Eric Auger wrote: >>>> - new fields are added on producer side: linux irq, vfio_device handle, >>>> active which reflects whether the source is active (at interrupt >>>> controller level or at VFIO level - automasked -) and finally an >>>> opaque pointer which will be used to point to the vfio_platform_device >>>> in this series. >>> >>> Linux IRQ and active should be okay. As to the vfio_device handle, you >>> should link it from the vfio_platform_device instead. And for the >>> vfio_platform_device, you can link it from the vfio_platform_irq instead. >>> >>> Once you've done this, embed the irq_bypass_producer struct in the >>> vfio_platform_irq struct; in the new kvm_arch_* functions, go back to >>> the vfio_platform_irq struct via container_of. From there you can >>> retrieve pointers to the vfio_platform_device and the vfio_device. >>> >>>> - new fields on consumer side: the kvm handle, the gsi >>> >>> You do not need to add these. Instead, add the kvm handle to irqfd >>> only. Like above, embed the irq_bypass_consumer struct in the irqfd >>> struct; in the new kvm_arch_* functions, go back to the >>> vfio_platform_irq struct via container_of. >>> >> >> I also need the gsi field here, for posted-interrupts, I need 'gsi', 'irq' to >> update the IRTE. > > Oh... we can get gsi from irq_bypass_consumer -> _irqfd -> gsi, so it > is not needed in irq_bypass_consumer. Got it! :) The issue I have is that struct _irqfd is local to eventfd.c so it cannot be used in archi specific code. Is it acceptable to move it to kvm_host.h, naming it something like kvm_kernel_irqfd (as done for kvm_kernel_irq_routing_entry)? Would also need to move _irqfd_resampler there (kvm_kernel_irqfd_resampler). irqfd user struct cannot be used in a standalone manner since we miss the kvm handle. Thanks Eric > > Thanks, > Feng > >> >> 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
> -----Original Message----- > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo > Bonzini > Sent: Friday, July 03, 2015 2:52 PM > To: Wu, Feng; Eric Auger; eric.auger@st.com; > linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; > alex.williamson@redhat.com; avi.kivity@gmail.com; mtosatti@redhat.com; > joro@8bytes.org; b.reynal@virtualopensystems.com > Cc: linux-kernel@vger.kernel.org; patches@linaro.org > Subject: Re: [RFC 12/17] irq: bypass: Extend skeleton for ARM forwarding > control > > > > On 03/07/2015 04:43, Wu, Feng wrote: > > > > struct irq_bypass_consumer { > > struct list_head node; > > void *token; > > + unsigned irq; /*got from producer when registered*/ > > void (*add_producer)(struct irq_bypass_producer *, > > struct irq_bypass_consumer *); > > void (*del_producer)(struct irq_bypass_producer *, > > struct irq_bypass_consumer *); > > + void (*update)(struct irq_bypass_consumer *); > > }; > > > > 'update' is used to update the IRTE, while irq is initialized when > > registered, which is used to find the right IRTE. > > Feel free to add "update" in your PI patches. I am not sure if "irq" > belongs here or in the containing struct. You can play with both and > submit the version that looks better to you. Thanks for your review, Paolo. In my understanding, irq comes from the producer side, while gsi belongs to the consumer, so we need to get the irq from the producer somewhere. I am not sure adding irq here is the good way, but what I need is in the 'update' function, I have irq, gsi in hand. :) 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
On 03/07/2015 08:54, Eric Auger wrote: >> > Oh... we can get gsi from irq_bypass_consumer -> _irqfd -> gsi, so it >> > is not needed in irq_bypass_consumer. Got it! :) > The issue I have is that struct _irqfd is local to eventfd.c so it > cannot be used in archi specific code. Is it acceptable to move it to > kvm_host.h, naming it something like kvm_kernel_irqfd (as done for > kvm_kernel_irq_routing_entry)? Would also need to move _irqfd_resampler > there (kvm_kernel_irqfd_resampler). Yes, that's okay. Can you put it in a new header file kvm_irqfd.h though? Paolo > irqfd user struct cannot be used in a standalone manner since we miss > the kvm handle. -- 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
On 03/07/2015 09:00, Wu, Feng wrote: >>> > > struct irq_bypass_consumer { >>> > > struct list_head node; >>> > > void *token; >>> > > + unsigned irq; /*got from producer when registered*/ >>> > > void (*add_producer)(struct irq_bypass_producer *, >>> > > struct irq_bypass_consumer *); >>> > > void (*del_producer)(struct irq_bypass_producer *, >>> > > struct irq_bypass_consumer *); >>> > > + void (*update)(struct irq_bypass_consumer *); >>> > > }; >>> > > >>> > > 'update' is used to update the IRTE, while irq is initialized when >>> > > registered, which is used to find the right IRTE. >> > >> > Feel free to add "update" in your PI patches. I am not sure if "irq" >> > belongs here or in the containing struct. You can play with both and >> > submit the version that looks better to you. > Thanks for your review, Paolo. In my understanding, irq comes from > the producer side, while gsi belongs to the consumer, so we need > to get the irq from the producer somewhere. I am not sure adding > irq here is the good way, but what I need is in the 'update' function, > I have irq, gsi in hand. :) It's difficult to say without seeing the patches... The IRQ is stored in the producer already with Eric's changes. If you need to store the old IRQ value, because "update" needs to do something with it, then I think "irq" belongs in the container struct. Perhaps "update" needs to have a producer argument as well? 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
On 02/07/2015 15:17, Eric Auger wrote: > + void (*stop_producer)(struct irq_bypass_producer *); > + void (*resume_producer)(struct irq_bypass_producer *); Also, can you call these just "stop"/"resume" ... > + void (*add_consumer)(struct irq_bypass_producer *, > + struct irq_bypass_consumer *); > + void (*del_consumer)(struct irq_bypass_producer *, > + struct irq_bypass_consumer *); > }; > > struct irq_bypass_consumer { > struct list_head node; > void *token; > - void (*add_producer)(struct irq_bypass_producer *); > - void (*del_producer)(struct irq_bypass_producer *); > + unsigned int gsi; /* the guest gsi */ > + struct kvm *kvm; > + void (*stop_consumer)(struct irq_bypass_consumer *); > + void (*resume_consumer)(struct irq_bypass_consumer *); ... and same here? The KVM functions could be named - kvm_arch_irq_bypass_add_producer - kvm_arch_irq_bypass_del_producer - kvm_arch_irq_bypass_stop - kvm_arch_irq_bypass_resume 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: Friday, July 03, 2015 3:06 PM > To: Wu, Feng; Eric Auger; eric.auger@st.com; > linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > kvm@vger.kernel.org; christoffer.dall@linaro.org; marc.zyngier@arm.com; > alex.williamson@redhat.com; avi.kivity@gmail.com; mtosatti@redhat.com; > joro@8bytes.org; b.reynal@virtualopensystems.com > Cc: linux-kernel@vger.kernel.org; patches@linaro.org > Subject: Re: [RFC 12/17] irq: bypass: Extend skeleton for ARM forwarding > control > > > > On 03/07/2015 09:00, Wu, Feng wrote: > >>> > > struct irq_bypass_consumer { > >>> > > struct list_head node; > >>> > > void *token; > >>> > > + unsigned irq; /*got from producer when registered*/ > >>> > > void (*add_producer)(struct irq_bypass_producer *, > >>> > > struct irq_bypass_consumer *); > >>> > > void (*del_producer)(struct irq_bypass_producer *, > >>> > > struct irq_bypass_consumer *); > >>> > > + void (*update)(struct irq_bypass_consumer *); > >>> > > }; > >>> > > > >>> > > 'update' is used to update the IRTE, while irq is initialized when > >>> > > registered, which is used to find the right IRTE. > >> > > >> > Feel free to add "update" in your PI patches. I am not sure if "irq" > >> > belongs here or in the containing struct. You can play with both and > >> > submit the version that looks better to you. > > Thanks for your review, Paolo. In my understanding, irq comes from > > the producer side, while gsi belongs to the consumer, so we need > > to get the irq from the producer somewhere. I am not sure adding > > irq here is the good way, but what I need is in the 'update' function, > > I have irq, gsi in hand. :) > > It's difficult to say without seeing the patches... The IRQ is stored > in the producer already with Eric's changes. If you need to store the > old IRQ value, because "update" needs to do something with it, then I > think "irq" belongs in the container struct. > > Perhaps "update" needs to have a producer argument as well? I also consider this method, basically, I will call 'update' in irqfd_update(), but seems I need do extra things to get the producer structure (such as, iterate the producer list to find the one with the same 'token') before calling 'update' from consumer side. I am not sure it is worth doing that way. 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
Hi Paolo, On 07/02/2015 03:40 PM, Paolo Bonzini wrote: > > > On 02/07/2015 15:17, Eric Auger wrote: >> - new fields are added on producer side: linux irq, vfio_device handle, >> active which reflects whether the source is active (at interrupt >> controller level or at VFIO level - automasked -) and finally an >> opaque pointer which will be used to point to the vfio_platform_device >> in this series. > > Linux IRQ and active should be okay. As to the vfio_device handle, you > should link it from the vfio_platform_device instead. And for the > vfio_platform_device, you can link it from the vfio_platform_irq instead. For this last one, I don't think this is achievable since if I store the vfio_platform_irq in the opaque, it matches irqs[i] of vfio_platform_device and I don't have any mean to retrieve "i" when calling container_of. struct vfio_platform_irq *irq = container_of(prod, struct vfio_platform_irq, producer); struct vfio_platform_device *vpdev = container_of(irq, struct vfio_platform_device, irqs[i?]); struct vfio_platform_device { ../.. struct vfio_platform_irq *irqs; ../.. } So I think I still need to pass vfio_platform_device in the opaque and look on irqs array to identify the right vfio_platform_irq *. Do I miss sthg? - Eric > > Once you've done this, embed the irq_bypass_producer struct in the > vfio_platform_irq struct; in the new kvm_arch_* functions, go back to > the vfio_platform_irq struct via container_of. From there you can > retrieve pointers to the vfio_platform_device and the vfio_device. > >> - new fields on consumer side: the kvm handle, the gsi > > You do not need to add these. Instead, add the kvm handle to irqfd > only. Like above, embed the irq_bypass_consumer struct in the irqfd > struct; in the new kvm_arch_* functions, go back to the > vfio_platform_irq struct via container_of. > > 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
On 03/07/2015 15:12, Eric Auger wrote: >> > Linux IRQ and active should be okay. As to the vfio_device handle, you >> > should link it from the vfio_platform_device instead. And for the >> > vfio_platform_device, you can link it from the vfio_platform_irq instead. > For this last one, I don't think this is achievable since if I store the > vfio_platform_irq in the opaque, it matches irqs[i] of > vfio_platform_device and I don't have any mean to retrieve "i" when > calling container_of. Right, notice I said "link it": struct vfio_platform_irq *irq = container_of(prod, struct vfio_platform_irq, producer); struct vfio_platform_device *vpdev = irq->vpdev; struct vfio_device *vdev = vpdev->vdev; Would this be okay? 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
On 07/03/2015 07:20 PM, Paolo Bonzini wrote: > > > On 03/07/2015 15:12, Eric Auger wrote: >>>> Linux IRQ and active should be okay. As to the vfio_device handle, you >>>> should link it from the vfio_platform_device instead. And for the >>>> vfio_platform_device, you can link it from the vfio_platform_irq instead. >> For this last one, I don't think this is achievable since if I store the >> vfio_platform_irq in the opaque, it matches irqs[i] of >> vfio_platform_device and I don't have any mean to retrieve "i" when >> calling container_of. > > Right, notice I said "link it": > > struct vfio_platform_irq *irq = > container_of(prod, struct vfio_platform_irq, producer); > struct vfio_platform_device *vpdev = irq->vpdev; > struct vfio_device *vdev = vpdev->vdev; > > Would this be okay? Yes that's what I did. I added the vfio_device handle in struct vfio_platform_irq Thanks ;-) Have a nice WE Eric > > 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/include/linux/irqbypass.h b/include/linux/irqbypass.h index 718508e..591ae3f 100644 --- a/include/linux/irqbypass.h +++ b/include/linux/irqbypass.h @@ -3,17 +3,37 @@ #include <linux/list.h> +struct vfio_device; +struct irq_bypass_consumer; +struct kvm; + struct irq_bypass_producer { struct list_head node; void *token; - /* TBD */ + unsigned int irq; /* host physical irq */ + struct vfio_device *vdev; /* vfio device that requested irq */ + /* is irq active at irqchip or VFIO masked? */ + bool active; + void *opaque; + void (*stop_producer)(struct irq_bypass_producer *); + void (*resume_producer)(struct irq_bypass_producer *); + void (*add_consumer)(struct irq_bypass_producer *, + struct irq_bypass_consumer *); + void (*del_consumer)(struct irq_bypass_producer *, + struct irq_bypass_consumer *); }; struct irq_bypass_consumer { struct list_head node; void *token; - void (*add_producer)(struct irq_bypass_producer *); - void (*del_producer)(struct irq_bypass_producer *); + unsigned int gsi; /* the guest gsi */ + struct kvm *kvm; + void (*stop_consumer)(struct irq_bypass_consumer *); + void (*resume_consumer)(struct irq_bypass_consumer *); + void (*add_producer)(struct irq_bypass_consumer *, + struct irq_bypass_producer *); + void (*del_producer)(struct irq_bypass_consumer *, + struct irq_bypass_producer *); }; int irq_bypass_register_producer(struct irq_bypass_producer *); diff --git a/kernel/irq/bypass.c b/kernel/irq/bypass.c index 5d0f92b..fb31fef 100644 --- a/kernel/irq/bypass.c +++ b/kernel/irq/bypass.c @@ -19,6 +19,46 @@ static LIST_HEAD(producers); static LIST_HEAD(consumers); static DEFINE_MUTEX(lock); +/* lock must be hold when calling connect */ +static void connect(struct irq_bypass_producer *prod, + struct irq_bypass_consumer *cons) +{ + pr_info("++++ %s prod(%d) -> cons(%d)\n", + __func__, prod->irq, cons->gsi); + if (prod->stop_producer) + prod->stop_producer(prod); + if (cons->stop_consumer) + cons->stop_consumer(cons); + if (prod->add_consumer) + prod->add_consumer(prod, cons); + if (cons->add_producer) + cons->add_producer(cons, prod); + if (cons->resume_consumer) + cons->resume_consumer(cons); + if (prod->resume_producer) + prod->resume_producer(prod); +} + +/* lock must be hold when calling disconnect */ +static void disconnect(struct irq_bypass_producer *prod, + struct irq_bypass_consumer *cons) +{ + pr_info("---- %s prod(%d) -> cons(%d)\n", + __func__, prod->irq, cons->gsi); + if (prod->stop_producer) + prod->stop_producer(prod); + if (cons->stop_consumer) + cons->stop_consumer(cons); + if (cons->del_producer) + cons->del_producer(cons, prod); + if (prod->del_consumer) + prod->del_consumer(prod, cons); + if (cons->resume_consumer) + cons->resume_consumer(cons); + if (prod->resume_producer) + prod->resume_producer(prod); +} + int irq_bypass_register_producer(struct irq_bypass_producer *producer) { struct irq_bypass_producer *tmp; @@ -38,7 +78,7 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer) list_for_each_entry(consumer, &consumers, node) { if (consumer->token == producer->token) { - consumer->add_producer(producer); + connect(producer, consumer); break; } } @@ -56,7 +96,7 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer) list_for_each_entry(consumer, &consumers, node) { if (consumer->token == producer->token) { - consumer->del_producer(producer); + disconnect(producer, consumer); break; } } @@ -86,7 +126,7 @@ int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer) list_for_each_entry(producer, &producers, node) { if (producer->token == consumer->token) { - consumer->add_producer(producer); + connect(producer, consumer); break; } } @@ -104,7 +144,7 @@ void irq_bypass_unregister_consumer(struct irq_bypass_consumer *consumer) list_for_each_entry(producer, &producers, node) { if (producer->token == consumer->token) { - consumer->del_producer(producer); + disconnect(producer, consumer); break; } }
- [add,del]_[consumer,producer] updated to takes both the consumer and producer handles. This is requested to combine info from both, typically to link the source irq owned by the producer with the gsi owned by the consumer (forwarded IRQ setup). - new functions are added: [stop,resume]_[consumer, producer]. Those are needed for forwarding since the state change requires to entermingle actions at consumer, producer. - On handshake, we now call connect, disconnect which features the more complex sequence. - new fields are added on producer side: linux irq, vfio_device handle, active which reflects whether the source is active (at interrupt controller level or at VFIO level - automasked -) and finally an opaque pointer which will be used to point to the vfio_platform_device in this series. - new fields on consumer side: the kvm handle, the gsi Integration of posted interrupt series will help to refine those choices Signed-off-by: Eric Auger <eric.auger@linaro.org> --- - connect/disconnect could become a cb too. For forwarding it may make sense to have failure at connection: this would happen when the physical IRQ is either active at irqchip level or VFIO masked. This means some of the cb should return an error and this error management could be prod/cons specific. Where to attach the connect/disconnect cb: to the cons or prod, to both? - Hence may be sensible to do the list_add only if connect returns 0 - disconnect would not be allowed to fail. --- include/linux/irqbypass.h | 26 ++++++++++++++++++++++--- kernel/irq/bypass.c | 48 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 67 insertions(+), 7 deletions(-)