Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall <julien.grall@linaro.org>]

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,

-- 
Julien Grall