| Message ID | 1390581822-32624-8-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: > On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same > interrupt. Mention here that you are therefore creating a linked list of actions for each interrupt. If you use xen/list.h for this then you get a load of helpers and iterators which would save you open coding them. Some discussion of the behaviour wrt acking the physical interrupt might also be interesting, especially in the case where the shared IRQ is routed to the guest and to the hypervisor or to multiple guests. > Signed-off-by: Julien Grall <julien.grall@linaro.org> > CC: Keir Fraser <keir@xen.org> > --- > xen/arch/arm/gic.c | 48 ++++++++++++++++++++++++++++++++++++++++-------- > xen/arch/arm/irq.c | 6 +++++- > xen/include/xen/irq.h | 1 + > 3 files changed, 46 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index ebd2b5f..8ba1de3 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -534,32 +534,64 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id) > { > struct irq_desc *desc; > unsigned long flags; > - struct irqaction *action; > + struct irqaction *action, **action_ptr; > > desc = irq_to_desc(irq->irq); > > spin_lock_irqsave(&desc->lock,flags); > desc->handler->shutdown(desc); > action = desc->action; > - desc->action = NULL; > - desc->status &= ~IRQ_GUEST; > + > + action_ptr = &desc->action; > + for ( ;; ) > + { > + action = *action_ptr; > + > + if ( !action ) > + { > + printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", > + irq->irq); > + return; > + } > + > + if ( action->dev_id == dev_id ) > + break; > + > + action_ptr = &action->next; > + } > + > + /* Found it - remove it from the action list */ > + *action_ptr = action->next; > + > + /* If this was the list action, shut down the IRQ */ > + if ( !desc->action ) > + { > + desc->handler->shutdown(desc); > + desc->status &= ~IRQ_GUEST; > + } > > spin_unlock_irqrestore(&desc->lock,flags); > > /* Wait to make sure it's not being used on another CPU */ > do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS ); > > - if (action && action->free_on_release) > + if ( action && action->free_on_release ) > xfree(action); > } > > static int __setup_irq(struct irq_desc *desc, unsigned int irq, > struct irqaction *new) > { > - if ( desc->action != NULL ) > - return -EBUSY; > + struct irqaction *action = desc->action; > + > + ASSERT(new != NULL); > + > + /* Check that dev_id is correctly filled if we have multiple action */ > + if ( action != NULL && ( action->dev_id == NULL || new->dev_id == NULL ) ) > + return -EINVAL; > > - desc->action = new; > + new->next = desc->action; > + desc->action = new; > dsb(); > > return 0; > @@ -610,7 +642,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > > rc = __setup_irq(desc, irq->irq, new); > > - if ( !rc ) > + if ( !rc && disabled ) > desc->handler->startup(desc); > > spin_unlock_irqrestore(&desc->lock, flags); > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 3e326b0..edf0404 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -179,7 +179,11 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) > { > desc->status &= ~IRQ_PENDING; > spin_unlock_irq(&desc->lock); > - action->handler(irq, action->dev_id, regs); > + do > + { > + action->handler(irq, action->dev_id, regs); > + action = action->next; > + } while ( action ); > spin_lock_irq(&desc->lock); > } > > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h > index f2e6215..54314b8 100644 > --- a/xen/include/xen/irq.h > +++ b/xen/include/xen/irq.h > @@ -11,6 +11,7 @@ > > struct irqaction { > void (*handler)(int, void *, struct cpu_user_regs *); > + struct irqaction *next; > const char *name; > void *dev_id; > bool_t free_on_release;
Hi Ian, On 02/19/2014 11:55 AM, Ian Campbell wrote: > On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same >> interrupt. > > Mention here that you are therefore creating a linked list of actions > for each interrupt. > > If you use xen/list.h for this then you get a load of helpers and > iterators which would save you open coding them. I will use it on the next version. > Some discussion of the behaviour wrt acking the physical interrupt might > also be interesting, especially in the case where the shared IRQ is > routed to the guest and to the hypervisor or to multiple guests. For now, it's forbidden by the patch #3 of this series. Sharing IRQ between Xen and guest will need a bit of rework, mainly when the Stefano's patch series to remove the maintenance interrupt will be applied. Cheers,
Hi Ian, On 02/19/2014 11:55 AM, Ian Campbell wrote: > On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same >> interrupt. > > Mention here that you are therefore creating a linked list of actions > for each interrupt. > > If you use xen/list.h for this then you get a load of helpers and > iterators which would save you open coding them. After thinking, using xen/list.h won't really remove open code, except removing "action_ptr" in release_dt_irq. Calling release_dt_irq to an IRQ with multiple action shouldn't be called often. Therefore, having both prev and next is a waste of space. Cheers,
On Mon, 2014-02-24 at 14:08 +0000, Julien Grall wrote: > (Adding Jan for x86 part). > > On 02/20/2014 09:29 PM, Julien Grall wrote: > > Hi Ian, > > > > On 02/19/2014 11:55 AM, Ian Campbell wrote: > >> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: > >>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same > >>> interrupt. > >> > >> Mention here that you are therefore creating a linked list of actions > >> for each interrupt. > >> > >> If you use xen/list.h for this then you get a load of helpers and > >> iterators which would save you open coding them. > > > > After thinking, using xen/list.h won't really remove open code, except > > removing "action_ptr" in release_dt_irq. You can list_for_each*() in do_IRQ too and in release_dt_irq you get to use a well debugged list delete implementation instead of reimplementing your own. > > Calling release_dt_irq to an IRQ with multiple action shouldn't be > > called often. Therefore, having both prev and next is a waste of space. If this is a concern we could take in the Linux single linked list macros alongside the existing doubly linked ines we took from them. Ian. > Jan, as it's common code, do you have any thoughts? >
On 02/24/2014 02:32 PM, Jan Beulich wrote: >>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote: >> (Adding Jan for x86 part). >> >> On 02/20/2014 09:29 PM, Julien Grall wrote: >>> Hi Ian, >>> >>> On 02/19/2014 11:55 AM, Ian Campbell wrote: >>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same >>>>> interrupt. >>>> >>>> Mention here that you are therefore creating a linked list of actions >>>> for each interrupt. >>>> >>>> If you use xen/list.h for this then you get a load of helpers and >>>> iterators which would save you open coding them. >>> >>> After thinking, using xen/list.h won't really remove open code, except >>> removing "action_ptr" in release_dt_irq. >>> >>> Calling release_dt_irq to an IRQ with multiple action shouldn't be >>> called often. Therefore, having both prev and next is a waste of space. >> >> Jan, as it's common code, do you have any thoughts? > > In fact I'm not convinced this action chaining is correct in the first > place, as mentioned by Ian too (considering the potential sharing > between hypervisor and guest). Furthermore, if this is really just > about IOMMU handlers, why can't the SMMU code register a single > action and disambiguate by the dev_id argument passed to the > handler? The patch #3 of this serie protects the IRQ to be shared with the domain. I should have remove "eg ARM SMMU" in the description. ARM SMMU is not the only the case, we don't know in advance if the IRQ will be shared (except browsing the DT and checking if this IRQ was used by another devices...). We may have the same thing with other devices. The logic is painful to handle internally in ARM SMMU driver while we can handle it generically. No need to duplicate the code when a new driver will have the same problem.
Hello Jan, On 02/24/2014 02:48 PM, Julien Grall wrote: > On 02/24/2014 02:32 PM, Jan Beulich wrote: >>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote: >>> (Adding Jan for x86 part). >>> >>> On 02/20/2014 09:29 PM, Julien Grall wrote: >>>> Hi Ian, >>>> >>>> On 02/19/2014 11:55 AM, Ian Campbell wrote: >>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same >>>>>> interrupt. >>>>> >>>>> Mention here that you are therefore creating a linked list of actions >>>>> for each interrupt. >>>>> >>>>> If you use xen/list.h for this then you get a load of helpers and >>>>> iterators which would save you open coding them. >>>> >>>> After thinking, using xen/list.h won't really remove open code, except >>>> removing "action_ptr" in release_dt_irq. >>>> >>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be >>>> called often. Therefore, having both prev and next is a waste of space. >>> >>> Jan, as it's common code, do you have any thoughts? >> >> In fact I'm not convinced this action chaining is correct in the first >> place, as mentioned by Ian too (considering the potential sharing >> between hypervisor and guest). Furthermore, if this is really just >> about IOMMU handlers, why can't the SMMU code register a single >> action and disambiguate by the dev_id argument passed to the >> handler? > > The patch #3 of this serie protects the IRQ to be shared with the domain. > > I should have remove "eg ARM SMMU" in the description. ARM SMMU is not > the only the case, we don't know in advance if the IRQ will be shared > (except browsing the DT and checking if this IRQ was used by another > devices...). We may have the same thing with other devices. > > The logic is painful to handle internally in ARM SMMU driver while we > can handle it generically. No need to duplicate the code when a new > driver will have the same problem. > I haven't heard any answer from you. Shall I take as a "go"? Regards,
>>> On 11.03.14 at 16:16, Julien Grall <julien.grall@linaro.org> wrote: > Hello Jan, > > On 02/24/2014 02:48 PM, Julien Grall wrote: >> On 02/24/2014 02:32 PM, Jan Beulich wrote: >>>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote: >>>> (Adding Jan for x86 part). >>>> >>>> On 02/20/2014 09:29 PM, Julien Grall wrote: >>>>> Hi Ian, >>>>> >>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote: >>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same >>>>>>> interrupt. >>>>>> >>>>>> Mention here that you are therefore creating a linked list of actions >>>>>> for each interrupt. >>>>>> >>>>>> If you use xen/list.h for this then you get a load of helpers and >>>>>> iterators which would save you open coding them. >>>>> >>>>> After thinking, using xen/list.h won't really remove open code, except >>>>> removing "action_ptr" in release_dt_irq. >>>>> >>>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be >>>>> called often. Therefore, having both prev and next is a waste of space. >>>> >>>> Jan, as it's common code, do you have any thoughts? >>> >>> In fact I'm not convinced this action chaining is correct in the first >>> place, as mentioned by Ian too (considering the potential sharing >>> between hypervisor and guest). Furthermore, if this is really just >>> about IOMMU handlers, why can't the SMMU code register a single >>> action and disambiguate by the dev_id argument passed to the >>> handler? >> >> The patch #3 of this serie protects the IRQ to be shared with the domain. >> >> I should have remove "eg ARM SMMU" in the description. ARM SMMU is not >> the only the case, we don't know in advance if the IRQ will be shared >> (except browsing the DT and checking if this IRQ was used by another >> devices...). We may have the same thing with other devices. >> >> The logic is painful to handle internally in ARM SMMU driver while we >> can handle it generically. No need to duplicate the code when a new >> driver will have the same problem. > > I haven't heard any answer from you. Shall I take as a "go"? I'm sorry, this got lost between other stuff. Honestly I'm still not convinced generic multi-action IRQ support is indeed useful. But I wouldn't veto it either if others are convinced of this approach. An option possibly to explore might be to have a per-arch trigger enabling this, and keep it off for x86. Jan
On Tue, 11 Mar 2014, Jan Beulich wrote: > >>> On 11.03.14 at 16:16, Julien Grall <julien.grall@linaro.org> wrote: > > Hello Jan, > > > > On 02/24/2014 02:48 PM, Julien Grall wrote: > >> On 02/24/2014 02:32 PM, Jan Beulich wrote: > >>>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote: > >>>> (Adding Jan for x86 part). > >>>> > >>>> On 02/20/2014 09:29 PM, Julien Grall wrote: > >>>>> Hi Ian, > >>>>> > >>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote: > >>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: > >>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same > >>>>>>> interrupt. > >>>>>> > >>>>>> Mention here that you are therefore creating a linked list of actions > >>>>>> for each interrupt. > >>>>>> > >>>>>> If you use xen/list.h for this then you get a load of helpers and > >>>>>> iterators which would save you open coding them. > >>>>> > >>>>> After thinking, using xen/list.h won't really remove open code, except > >>>>> removing "action_ptr" in release_dt_irq. > >>>>> > >>>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be > >>>>> called often. Therefore, having both prev and next is a waste of space. > >>>> > >>>> Jan, as it's common code, do you have any thoughts? > >>> > >>> In fact I'm not convinced this action chaining is correct in the first > >>> place, as mentioned by Ian too (considering the potential sharing > >>> between hypervisor and guest). Furthermore, if this is really just > >>> about IOMMU handlers, why can't the SMMU code register a single > >>> action and disambiguate by the dev_id argument passed to the > >>> handler? > >> > >> The patch #3 of this serie protects the IRQ to be shared with the domain. > >> > >> I should have remove "eg ARM SMMU" in the description. ARM SMMU is not > >> the only the case, we don't know in advance if the IRQ will be shared > >> (except browsing the DT and checking if this IRQ was used by another > >> devices...). We may have the same thing with other devices. > > >> The logic is painful to handle internally in ARM SMMU driver while we > >> can handle it generically. No need to duplicate the code when a new > >> driver will have the same problem. > > > > I haven't heard any answer from you. Shall I take as a "go"? > > I'm sorry, this got lost between other stuff. Honestly I'm still not > convinced generic multi-action IRQ support is indeed useful. I agree. In general if an IRQ is shared among multiple devices, it is likely to go to Dom0 and have a single action from Xen point of view. An IRQ shared between Xen and a guest is a very bad idea.
Hi Stefano, On 17/03/14 19:06, Stefano Stabellini wrote: > On Tue, 11 Mar 2014, Jan Beulich wrote: >>>>> On 11.03.14 at 16:16, Julien Grall <julien.grall@linaro.org> wrote: >>> Hello Jan, >>> >>> On 02/24/2014 02:48 PM, Julien Grall wrote: >>>> On 02/24/2014 02:32 PM, Jan Beulich wrote: >>>>>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote: >>>>>> (Adding Jan for x86 part). >>>>>> >>>>>> On 02/20/2014 09:29 PM, Julien Grall wrote: >>>>>>> Hi Ian, >>>>>>> >>>>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote: >>>>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >>>>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same >>>>>>>>> interrupt. >>>>>>>> >>>>>>>> Mention here that you are therefore creating a linked list of actions >>>>>>>> for each interrupt. >>>>>>>> >>>>>>>> If you use xen/list.h for this then you get a load of helpers and >>>>>>>> iterators which would save you open coding them. >>>>>>> >>>>>>> After thinking, using xen/list.h won't really remove open code, except >>>>>>> removing "action_ptr" in release_dt_irq. >>>>>>> >>>>>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be >>>>>>> called often. Therefore, having both prev and next is a waste of space. >>>>>> >>>>>> Jan, as it's common code, do you have any thoughts? >>>>> >>>>> In fact I'm not convinced this action chaining is correct in the first >>>>> place, as mentioned by Ian too (considering the potential sharing >>>>> between hypervisor and guest). Furthermore, if this is really just >>>>> about IOMMU handlers, why can't the SMMU code register a single >>>>> action and disambiguate by the dev_id argument passed to the >>>>> handler? >>>> >>>> The patch #3 of this serie protects the IRQ to be shared with the domain. >>>> >>>> I should have remove "eg ARM SMMU" in the description. ARM SMMU is not >>>> the only the case, we don't know in advance if the IRQ will be shared >>>> (except browsing the DT and checking if this IRQ was used by another >>>> devices...). We may have the same thing with other devices. >> >>>> The logic is painful to handle internally in ARM SMMU driver while we >>>> can handle it generically. No need to duplicate the code when a new >>>> driver will have the same problem. >>> >>> I haven't heard any answer from you. Shall I take as a "go"? >> >> I'm sorry, this got lost between other stuff. Honestly I'm still not >> convinced generic multi-action IRQ support is indeed useful. > > I agree. > In general if an IRQ is shared among multiple devices, it is likely to > go to Dom0 and have a single action from Xen point of view. > An IRQ shared between Xen and a guest is a very bad idea. I guess you agree with Jan, rigth? If so, I think you misunderstood the goal of this patch. This patch does *NOT* add support for IRQ sharing between domains and Xen (patch #3 is preventing that). Some devices are describing a same interrupt twice in the device tree and the action is not the same to accomplish. For instance for the SMMU on midway, the device tree bindings is: smmu_sata: smmu@9,20180000 { compatible = "arm,mmu-400"; reg = <0x9 0x20180000 0x10000>; mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>; #global-interrupts = <1>; interrupts = <0 114 4 0 114 4>; calxeda,smmu-secure-config-access; arm,smmu-isolate-devices; }; As you can see the same interrupts is used twice: the first one is used for the global interrupt and the second one as context interrupt. For the latter, each time a new context bank is created (e.g a device is passthrough to IOMMU), we need to register another handler. Now we have 2 solutions to implement it: 1) Implement it directly in the SMMU drivers => We are assuming we wouldn't this situation on another IOMMU drivers 2) Implement it generically The former one is complicated to implement, because it's not fixed if the IRQ will be described twice or not. We can take advantage of the generic code for this purpose. Regards,
On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote: > For instance for the SMMU on midway, the device tree bindings is: > > smmu_sata: smmu@9,20180000 { > compatible = "arm,mmu-400"; > reg = <0x9 0x20180000 0x10000>; > mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>; > #global-interrupts = <1>; > interrupts = <0 114 4 0 114 4>; > calxeda,smmu-secure-config-access; > arm,smmu-isolate-devices; > }; > > As you can see the same interrupts is used twice: Is that actually valid in device tree? Or is this a quirk of the midway DT? Ian.
Hi Ian, On 03/18/2014 09:33 AM, Ian Campbell wrote: > On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote: >> For instance for the SMMU on midway, the device tree bindings is: >> >> smmu_sata: smmu@9,20180000 { >> compatible = "arm,mmu-400"; >> reg = <0x9 0x20180000 0x10000>; >> mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>; >> #global-interrupts = <1>; >> interrupts = <0 114 4 0 114 4>; >> calxeda,smmu-secure-config-access; >> arm,smmu-isolate-devices; >> }; >> >> As you can see the same interrupts is used twice: > > Is that actually valid in device tree? Or is this a quirk of the midway > DT? Yes it's valid. The interrupts property for the SMMU is described as: "Interrupt list, with the first #global-irqs entries corresponding to the global interrupts and any following entries corresponding to context interrupts, specified in order of their indexing by the SMMU. For SMMUv2 implementations, there must be exactly one interrupt per context bank. In the case of a single, combined interrupt, it must be listed multiple times." On midway there is only one IRQ with is used for both context interrupt and global interrupt. As it's the only platform on Linux with SMMU support in the device tree, we don't know if every platform will have the same behavior. Regards,
On Tue, 18 Mar 2014, Julien Grall wrote: > Hi Ian, > > On 03/18/2014 09:33 AM, Ian Campbell wrote: > > On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote: > >> For instance for the SMMU on midway, the device tree bindings is: > >> > >> smmu_sata: smmu@9,20180000 { > >> compatible = "arm,mmu-400"; > >> reg = <0x9 0x20180000 0x10000>; > >> mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>; > >> #global-interrupts = <1>; > >> interrupts = <0 114 4 0 114 4>; > >> calxeda,smmu-secure-config-access; > >> arm,smmu-isolate-devices; > >> }; > >> > >> As you can see the same interrupts is used twice: > > > > Is that actually valid in device tree? Or is this a quirk of the midway > > DT? > > Yes it's valid. The interrupts property for the SMMU is described as: > > "Interrupt list, with the first #global-irqs entries corresponding to > the global interrupts and any following entries corresponding to context > interrupts, specified in order of their indexing by the SMMU. > > For SMMUv2 implementations, there must be exactly one interrupt per > context bank. In the case of a single, combined interrupt, it must be > listed multiple times." > > On midway there is only one IRQ with is used for both context interrupt > and global interrupt. As it's the only platform on Linux with SMMU > support in the device tree, we don't know if every platform will have > the same behavior. I understand that the SMMU might reuse the same IRQ for multiple purposes. I would still handle the scenario entirely within the SMMU driver. Can't we register a single handler for each of the IRQ listed under the SMMU node and then figure out what was the notification for in the handler?
Hi Stefano, On 03/18/2014 02:06 PM, Stefano Stabellini wrote: >> On 03/18/2014 09:33 AM, Ian Campbell wrote: >>> On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote: >>>> For instance for the SMMU on midway, the device tree bindings is: >>>> >>>> smmu_sata: smmu@9,20180000 { >>>> compatible = "arm,mmu-400"; >>>> reg = <0x9 0x20180000 0x10000>; >>>> mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>; >>>> #global-interrupts = <1>; >>>> interrupts = <0 114 4 0 114 4>; >>>> calxeda,smmu-secure-config-access; >>>> arm,smmu-isolate-devices; >>>> }; >>>> >>>> As you can see the same interrupts is used twice: >>> >>> Is that actually valid in device tree? Or is this a quirk of the midway >>> DT? >> >> Yes it's valid. The interrupts property for the SMMU is described as: >> >> "Interrupt list, with the first #global-irqs entries corresponding to >> the global interrupts and any following entries corresponding to context >> interrupts, specified in order of their indexing by the SMMU. >> >> For SMMUv2 implementations, there must be exactly one interrupt per >> context bank. In the case of a single, combined interrupt, it must be >> listed multiple times." >> >> On midway there is only one IRQ with is used for both context interrupt >> and global interrupt. As it's the only platform on Linux with SMMU >> support in the device tree, we don't know if every platform will have >> the same behavior. > > I understand that the SMMU might reuse the same IRQ for multiple > purposes. I would still handle the scenario entirely within the SMMU > driver. Can't we register a single handler for each of the IRQ listed > under the SMMU node and then figure out what was the notification for in > the handler? > We will have to check in the SMMU drivers, if the IRQ was already registered or not (because we don't know in advance if the IRQ is re-used). If not, Xen will register it with a new handler. The code to register the IRQ handler will looks like: int num_irqs = dt_number_of_irq(smmu->node); dt_irqs irqs[*]; dt_irq *irq; for ( i = 0; i < num_irqs; i++ ) { irq = dt_device_get_irq(smmu->node, i); foreach ( a in irqs ) if ( irq == a ) continue; request_dt_irq(); irqs <= irq; } I understand the point that people can badly use multiple action in the future, but is it a good reason to make the code more difficult to understand? Regards,
On Tue, 18 Mar 2014, Julien Grall wrote: > Hi Stefano, > > On 03/18/2014 02:06 PM, Stefano Stabellini wrote: > >> On 03/18/2014 09:33 AM, Ian Campbell wrote: > >>> On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote: > >>>> For instance for the SMMU on midway, the device tree bindings is: > >>>> > >>>> smmu_sata: smmu@9,20180000 { > >>>> compatible = "arm,mmu-400"; > >>>> reg = <0x9 0x20180000 0x10000>; > >>>> mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>; > >>>> #global-interrupts = <1>; > >>>> interrupts = <0 114 4 0 114 4>; > >>>> calxeda,smmu-secure-config-access; > >>>> arm,smmu-isolate-devices; > >>>> }; > >>>> > >>>> As you can see the same interrupts is used twice: > >>> > >>> Is that actually valid in device tree? Or is this a quirk of the midway > >>> DT? > >> > >> Yes it's valid. The interrupts property for the SMMU is described as: > >> > >> "Interrupt list, with the first #global-irqs entries corresponding to > >> the global interrupts and any following entries corresponding to context > >> interrupts, specified in order of their indexing by the SMMU. > >> > >> For SMMUv2 implementations, there must be exactly one interrupt per > >> context bank. In the case of a single, combined interrupt, it must be > >> listed multiple times." > >> > >> On midway there is only one IRQ with is used for both context interrupt > >> and global interrupt. As it's the only platform on Linux with SMMU > >> support in the device tree, we don't know if every platform will have > >> the same behavior. > > > > I understand that the SMMU might reuse the same IRQ for multiple > > purposes. I would still handle the scenario entirely within the SMMU > > driver. Can't we register a single handler for each of the IRQ listed > > under the SMMU node and then figure out what was the notification for in > > the handler? > > > > We will have to check in the SMMU drivers, if the IRQ was already > registered or not (because we don't know in advance if the IRQ is > re-used). If not, Xen will register it with a new handler. > > The code to register the IRQ handler will looks like: > > int num_irqs = dt_number_of_irq(smmu->node); > dt_irqs irqs[*]; > dt_irq *irq; > > for ( i = 0; i < num_irqs; i++ ) > { > irq = dt_device_get_irq(smmu->node, i); > foreach ( a in irqs ) > if ( irq == a ) > continue; > request_dt_irq(); > irqs <= irq; > } > > I understand the point that people can badly use multiple action in the > future, but is it a good reason to make the code more difficult to > understand? Can't we simply register all the irq handlers at boot time, regardless of whether they are currently used?
On Tue, 2014-03-18 at 14:54 +0000, Julien Grall wrote: > Hi Stefano, > > On 03/18/2014 02:06 PM, Stefano Stabellini wrote: > >> On 03/18/2014 09:33 AM, Ian Campbell wrote: > >>> On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote: > >>>> For instance for the SMMU on midway, the device tree bindings is: > >>>> > >>>> smmu_sata: smmu@9,20180000 { > >>>> compatible = "arm,mmu-400"; > >>>> reg = <0x9 0x20180000 0x10000>; > >>>> mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>; > >>>> #global-interrupts = <1>; > >>>> interrupts = <0 114 4 0 114 4>; > >>>> calxeda,smmu-secure-config-access; > >>>> arm,smmu-isolate-devices; > >>>> }; > >>>> > >>>> As you can see the same interrupts is used twice: > >>> > >>> Is that actually valid in device tree? Or is this a quirk of the midway > >>> DT? > >> > >> Yes it's valid. The interrupts property for the SMMU is described as: > >> > >> "Interrupt list, with the first #global-irqs entries corresponding to > >> the global interrupts and any following entries corresponding to context > >> interrupts, specified in order of their indexing by the SMMU. > >> > >> For SMMUv2 implementations, there must be exactly one interrupt per > >> context bank. In the case of a single, combined interrupt, it must be > >> listed multiple times." > >> > >> On midway there is only one IRQ with is used for both context interrupt > >> and global interrupt. As it's the only platform on Linux with SMMU > >> support in the device tree, we don't know if every platform will have > >> the same behavior. > > > > I understand that the SMMU might reuse the same IRQ for multiple > > purposes. I would still handle the scenario entirely within the SMMU > > driver. Can't we register a single handler for each of the IRQ listed > > under the SMMU node and then figure out what was the notification for in > > the handler? > > > > We will have to check in the SMMU drivers, if the IRQ was already > registered or not (because we don't know in advance if the IRQ is > re-used). If not, Xen will register it with a new handler. > > The code to register the IRQ handler will looks like: > > int num_irqs = dt_number_of_irq(smmu->node); assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum is a property of the hardware I think, so the driver is allowed to make such assumptions. Ian.
On 03/18/2014 03:02 PM, Ian Campbell wrote: >> int num_irqs = dt_number_of_irq(smmu->node); > > assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum > is a property of the hardware I think, so the driver is allowed to make > such assumptions. I know it would be easier ... but you can't assume that num_irqs == 2 :). The number is not determined.
On Tue, 2014-03-18 at 15:08 +0000, Julien Grall wrote: > On 03/18/2014 03:02 PM, Ian Campbell wrote: > >> int num_irqs = dt_number_of_irq(smmu->node); > > > > assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum > > is a property of the hardware I think, so the driver is allowed to make > > such assumptions. > > I know it would be easier ... but you can't assume that num_irqs == 2 :). > > The number is not determined. Are you saying that the SMMU-400 has an arbitrary number of interrupts? Ian.
On 03/18/2014 03:10 PM, Ian Campbell wrote: > On Tue, 2014-03-18 at 15:08 +0000, Julien Grall wrote: >> On 03/18/2014 03:02 PM, Ian Campbell wrote: >>>> int num_irqs = dt_number_of_irq(smmu->node); >>> >>> assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum >>> is a property of the hardware I think, so the driver is allowed to make >>> such assumptions. >> >> I know it would be easier ... but you can't assume that num_irqs == 2 :). >> >> The number is not determined. > > Are you saying that the SMMU-400 has an arbitrary number of interrupts? Yes.
Hi Stefano, On 03/18/2014 03:01 PM, Stefano Stabellini wrote: >> I understand the point that people can badly use multiple action in the >> future, but is it a good reason to make the code more difficult to >> understand? > > Can't we simply register all the irq handlers at boot time, regardless > of whether they are currently used? > This code is doing this actually. You need to know if the IRQ is represented multiple times in the "interrupts" list or not. Regards,
On Tue, 2014-03-18 at 15:12 +0000, Julien Grall wrote: > On 03/18/2014 03:10 PM, Ian Campbell wrote: > > On Tue, 2014-03-18 at 15:08 +0000, Julien Grall wrote: > >> On 03/18/2014 03:02 PM, Ian Campbell wrote: > >>>> int num_irqs = dt_number_of_irq(smmu->node); > >>> > >>> assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum > >>> is a property of the hardware I think, so the driver is allowed to make > >>> such assumptions. > >> > >> I know it would be easier ... but you can't assume that num_irqs == 2 :). > >> > >> The number is not determined. > > > > Are you saying that the SMMU-400 has an arbitrary number of interrupts? > > Yes. And the relevant doc is in the bindings: - #global-interrupts : The number of global interrupts exposed by the device. - interrupts : Interrupt list, with the first #global-irqs entries corresponding to the global interrupts and any following entries corresponding to context interrupts, specified in order of their indexing by the SMMU. For SMMUv2 implementations, there must be exactly one interrupt per context bank. In the case of a single, combined interrupt, it must be listed multiple times.
On Tue, 18 Mar 2014, Julien Grall wrote: > Hi Stefano, > > On 03/18/2014 03:01 PM, Stefano Stabellini wrote: > > >> I understand the point that people can badly use multiple action in the > >> future, but is it a good reason to make the code more difficult to > >> understand? > > > > Can't we simply register all the irq handlers at boot time, regardless > > of whether they are currently used? > > > > This code is doing this actually. You need to know if the IRQ is > represented multiple times in the "interrupts" list or not. Oh, that's right. You could rely on request_dt_irq() being able to handle failures gracefully to make the code easier to read. Overall I don't think the code is too bad.
On 03/18/2014 03:39 PM, Stefano Stabellini wrote: > On Tue, 18 Mar 2014, Julien Grall wrote: >> Hi Stefano, >> >> On 03/18/2014 03:01 PM, Stefano Stabellini wrote: >> >>>> I understand the point that people can badly use multiple action in the >>>> future, but is it a good reason to make the code more difficult to >>>> understand? >>> >>> Can't we simply register all the irq handlers at boot time, regardless >>> of whether they are currently used? >>> >> >> This code is doing this actually. You need to know if the IRQ is >> represented multiple times in the "interrupts" list or not. > > Oh, that's right. > > You could rely on request_dt_irq() being able to handle failures > gracefully to make the code easier to read. > Overall I don't think the code is too bad. request_dt_irq doesn't sound the right place to handle it. How will you differentiate an IRQ already registered by another driver and an IRQ registered by the driver itself? IHMO, this hack is worst and could lead issue with the developper because of misconception of the error code. Regards,
Hi all, On 03/18/2014 03:26 PM, Ian Campbell wrote: > On Tue, 2014-03-18 at 15:12 +0000, Julien Grall wrote: >> On 03/18/2014 03:10 PM, Ian Campbell wrote: >>> On Tue, 2014-03-18 at 15:08 +0000, Julien Grall wrote: >>>> On 03/18/2014 03:02 PM, Ian Campbell wrote: >>>>>> int num_irqs = dt_number_of_irq(smmu->node); >>>>> >>>>> assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum >>>>> is a property of the hardware I think, so the driver is allowed to make >>>>> such assumptions. >>>> >>>> I know it would be easier ... but you can't assume that num_irqs == 2 :). >>>> >>>> The number is not determined. >>> >>> Are you saying that the SMMU-400 has an arbitrary number of interrupts? >> >> Yes. > > And the relevant doc is in the bindings: > > - #global-interrupts : The number of global interrupts exposed by the > device. > > - interrupts : Interrupt list, with the first #global-irqs entries > corresponding to the global interrupts and any > following entries corresponding to context interrupts, > specified in order of their indexing by the SMMU. > > For SMMUv2 implementations, there must be exactly one > interrupt per context bank. In the case of a single, > combined interrupt, it must be listed multiple times. I'm about to resend a new version of the interrupts and IOMMU support. As I understand the main concern is to let the developer a "powerful" way to handle multiple action on a same IRQ in the future. Adding a such feature directly in the SMMU driver will be more complex (see a preview in http://www.gossamer-threads.com/lists/xen/devel/321318#321318) and IHMO complicated the code just for protecting against developer. If in the future next IOMMU drivers (or other kind of drivers in Xen) will come up with IRQ shared, the code will be duplicated. What is the final decision for the interrupt handling? Stefano would prefer to let the SMMU drivers handle a such case. Ian, do you have any opinion? Regards,
On Wed, 2014-03-19 at 17:18 +0000, Julien Grall wrote: > What is the final decision for the interrupt handling? Stefano would > prefer to let the SMMU drivers handle a such case. Ian, do you have any > opinion? It does sound like the end result is that shared interrupts need to be handled somehow. One potential issue is that a driver needs to be somewhat aware that it can be using a shared interrupt (and e.g. not log spurious looking interrupts). Linux solves this by having an explicit IRQF_SHARED which the driver must pass iff it is prepared to cope with this situation and the core should detect mismatches, which isn't a bad solution. I think it's pretty clear from the discussion that routing an IRQ to the guest must be equivalent to a driver which does not pass IRQF_SHARED. There's also the question of error handling if/when a spurious interrupt does occur (i.e. none of the multiple handlers dealt with it). Interrupt handlers should perhaps return a status to say whether or not they did anything with the interrupt, which the core code can then deal with (logging, quenching etc). Ian.
On 02/19/2014 11:55 AM, Ian Campbell wrote: > On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same >> interrupt. > > Mention here that you are therefore creating a linked list of actions > for each interrupt. > > If you use xen/list.h for this then you get a load of helpers and > iterators which would save you open coding them. I've tried to use xen/list.h. The amount of code it's basically the same and we I have to write open code to get the first element of the list and complexify release_dt_irq which is not nice. I will stick to the version with "next" for the next version of this series. Regards,
On Mon, 2014-03-31 at 16:45 +0100, Julien Grall wrote: > On 02/19/2014 11:55 AM, Ian Campbell wrote: > > On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: > >> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same > >> interrupt. > > > > Mention here that you are therefore creating a linked list of actions > > for each interrupt. > > > > If you use xen/list.h for this then you get a load of helpers and > > iterators which would save you open coding them. > > I've tried to use xen/list.h. The amount of code it's basically the same > and we I have to write open code to get the first element of the list Why? Can you post your WIP patch please for comparison. > and complexify release_dt_irq which is not nice. > > I will stick to the version with "next" for the next version of this series. > > Regards, >
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index ebd2b5f..8ba1de3 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -534,32 +534,64 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id) { struct irq_desc *desc; unsigned long flags; - struct irqaction *action; + struct irqaction *action, **action_ptr; desc = irq_to_desc(irq->irq); spin_lock_irqsave(&desc->lock,flags); desc->handler->shutdown(desc); action = desc->action; - desc->action = NULL; - desc->status &= ~IRQ_GUEST; + + action_ptr = &desc->action; + for ( ;; ) + { + action = *action_ptr; + + if ( !action ) + { + printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", + irq->irq); + return; + } + + if ( action->dev_id == dev_id ) + break; + + action_ptr = &action->next; + } + + /* Found it - remove it from the action list */ + *action_ptr = action->next; + + /* If this was the list action, shut down the IRQ */ + if ( !desc->action ) + { + desc->handler->shutdown(desc); + desc->status &= ~IRQ_GUEST; + } spin_unlock_irqrestore(&desc->lock,flags); /* Wait to make sure it's not being used on another CPU */ do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS ); - if (action && action->free_on_release) + if ( action && action->free_on_release ) xfree(action); } static int __setup_irq(struct irq_desc *desc, unsigned int irq, struct irqaction *new) { - if ( desc->action != NULL ) - return -EBUSY; + struct irqaction *action = desc->action; + + ASSERT(new != NULL); + + /* Check that dev_id is correctly filled if we have multiple action */ + if ( action != NULL && ( action->dev_id == NULL || new->dev_id == NULL ) ) + return -EINVAL; - desc->action = new; + new->next = desc->action; + desc->action = new; dsb(); return 0; @@ -610,7 +642,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) rc = __setup_irq(desc, irq->irq, new); - if ( !rc ) + if ( !rc && disabled ) desc->handler->startup(desc); spin_unlock_irqrestore(&desc->lock, flags); diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 3e326b0..edf0404 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -179,7 +179,11 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) { desc->status &= ~IRQ_PENDING; spin_unlock_irq(&desc->lock); - action->handler(irq, action->dev_id, regs); + do + { + action->handler(irq, action->dev_id, regs); + action = action->next; + } while ( action ); spin_lock_irq(&desc->lock); } diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h index f2e6215..54314b8 100644 --- a/xen/include/xen/irq.h +++ b/xen/include/xen/irq.h @@ -11,6 +11,7 @@ struct irqaction { void (*handler)(int, void *, struct cpu_user_regs *); + struct irqaction *next; const char *name; void *dev_id; bool_t free_on_release;
On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same interrupt. Signed-off-by: Julien Grall <julien.grall@linaro.org> CC: Keir Fraser <keir@xen.org> --- xen/arch/arm/gic.c | 48 ++++++++++++++++++++++++++++++++++++++++-------- xen/arch/arm/irq.c | 6 +++++- xen/include/xen/irq.h | 1 + 3 files changed, 46 insertions(+), 9 deletions(-)