Re: [PATCH 01/10] irqchip: irq-mips-gic: export gic_send_ipi

[Qais Yousef <qais.yousef@imgtec.com>]

On 08/24/2015 06:17 PM, Marc Zyngier wrote:
> [adding Mark Rutland, as this is heading straight into uncharted DT
> territory]
>
> On 24/08/15 17:39, Qais Yousef wrote:
>> On 08/24/2015 04:07 PM, Thomas Gleixner wrote:
>>> On Mon, 24 Aug 2015, Qais Yousef wrote:
>>>> On 08/24/2015 02:32 PM, Marc Zyngier wrote:
>>>>> I'd rather see something more "architected" than this blind export, or
>>>>> at least some level of filtering (the idea random drivers can access
>>>>> such a low-level function doesn't make me feel very good).
>>>> I don't know how to architect this better or how to perform  the filtering,
>>>> but I'm happy to hear suggestions and try them out.
>>>> Keep in mind that detecting GIC and writing your own gic_send_ipi() is very
>>>> simple. I have done this when the driver was out of tree. So restricting it by
>>>> not exporting it will not prevent someone from really accessing the
>>>> functionality, it's just they have to do it their own way.
>>> Keep in mind that we are not talking about out of tree hackery. We
>>> talk about a kernel code submission and I doubt, that you will get
>>> away with a GIC detection/fiddling burried in your driver code.
>>>
>>> Keep in mind that just slapping an export to some random function is
>>> not much better than doing a GIC hack in the driver.
>>>
>>> Marcs concerns about blindly exposing IPI functionality to drivers is
>>> well justified and that kind of coprocessor stuff is not unique to
>>> your particular SoC. We're going to see such things more frequently in
>>> the not so distant future, so we better think now about proper
>>> solutions to that problem.
>> Sure I'm not trying to argue against that.
>>
>>> There are a couple of issues to solve:
>>>
>>> 1) How is the IPI which is received by the coprocessor reserved in the
>>>      system?
>>>
>>> 2) How is it associated to a particular driver?
>> Shouldn't 'interrupts' property in DT take care of these 2 questions?
>> Maybe we can give it an alias name to make it more readable that this
>> interrupt is requested for external IPI.
> The "interrupts" property has a rather different meaning, and isn't
> designed to hardcode IPIs. Also, this property describes an interrupt
> from a device to the CPU, not the other way around (I imagine you also
> have an interrupt coming from the AXD to the CPU, possibly using an IPI
> too).

Yes we have an interrupt from AXD to the CPU. But the way I take
care of the routing at the moment is that the CPU routes the interrupt it
receives from AXD. And AXD routes the interrupt it receives from the
CPU. This is useful because in MIPS GIC the routing is done per hw thread
on the core so this gives the flexibility for each one to choose what it
suits it best.

>
> We can deal with these issues, but that's not something we can improvise.
>
> What I had in mind was something fairly generic:
> - interrupt-source: something generating an interrupt
> - interrupt-sink: something being targeted by an interrupt
>
> You could then express things like:
>
> intc: interrupt-controller@1000 {
> 	interrupt-controller;
> };
>
> mydevice@f0000000 {
> 	interrupt-source = <&intc INT_SPEC 2 &inttarg1 &inttarg1>;
> };

To make sure we're on the same page. INT_SPEC here refers to the arguments
we pass to a standard interrupts property, right?

>
> inttarg1: mydevice@f1000000 {
> 	interrupt-sink = <&intc HWAFFINITY1>;
> };
>
> inttarg2: cpu@1 {
> 	interrupt-sink = <&intc HWAFFINITY2>;
> };

And HWAFFINITY here is the core (or hardware thread) this interrupt will 
be routed to?

So for my case where CPU is on core 0 and AXD is on core 1 my 
description will look like

cpu: cpu@0 {
         interrupt-source = <&gic GIC_SHARED 36 IRQ_TYPE_EDGE_RISING 1 
&axd>;
         interrupt-sink = <&gic 0>;
}

axd: axd {
         interrupt-source = <&gic GIC_SHARED 37 IRQ_TYPE_EDGE_RISING 1 
&cpu>;
         interrupt-sink = <&gic 1>;
}

If I didn't misunderstand you, the issue I see with this is that the 
information about which IRQ to use to send an IPI to AXD is not present 
in the AXD node. We will need to search the cpu node for something that 
is meant to be routed to axd or have some logic to implicitly infer it 
from interrupt-sink in axd node. Not convenient IMO.

Can we replace 'something' in interrupt-source and interrupt-sink 
definitions to 'host' or 'CPU' or do we really care about creating IPI 
between any 2 'things'?

Changing the definition will also make interrupt-sink a synonym/alias to 
interrupts property. So the description will become

axd: axd {
         interrupt-source = <&gic GIC_SHARED 36 IRQ_TYPE_EDGE_RISING>; 
/* interrupt from CPU to AXD */
         interrupt-sink = <&gic GIC_SHARED 37 IRQ_TYPE_EDGE_RISING>; /* 
interrupt from AXD to CPU */
}

But this assume Linux won't take care of the routing. If we want Linux 
to take care of the routing, maybe something like this then?

axd: axd {
         interrupt-source = <&gic GIC_SHARED 36 IRQ_TYPE_EDGE_RISING 
HWAFFINITY1>; /* interrupt from CPU to
AXD@HWAFFINITY1*/
         interrupt-sink = <&gic GIC_SHARED 37 IRQ_TYPE_EDGE_RISING 
HWAFFINITY2>; /* interrupt from AXD to CPU@HWAFFINITY2 */
}

I don't think it's necessary to specify the HWAFFINITY2 for 
interrupt-sink as linux can use SMP affinity to move it around but we 
can make it optional in case there's a need to hardcode it to a specific 
Linux core. Or maybe the driver can use affinity hint..

>
> You could also imagine having CPUs being both source and sink.
>
>>> 3) How do we ensure that a driver cannot issue random IPIs and can
>>>      only send the associated ones?
>> If we get the irq number from DT then I'm not sure how feasible it is to
>> implement a generic_send_ipi() function that takes this number to
>> generate an IPI.
>>
>> Do you think this approach would work?
> If you follow the above approach, it should be pretty easy to derive a
> source identifier and a sink identifier from the DT, and have the core
> code to route one to the other and do the right thing.

Do you think it's better for linux to take care of all the routing 
instead of each core doing its own routing?
If Linux to do the routing for the other core (even if optionally), 
what's the mechanism to do that? We can't use irq_set_affinity() because 
we want to map something that is not part of Linux. A new mapping 
function in struct irq_domain_ops maybe?

>
> The source identifier could also be used to describe an IPI in a fairly
> safe way (the target being fixed by DT, but the actual number used
> dynamically allocated by the kernel).

To be dynamic, then the interrupt controller must specify which 
interrupts are actually free to use. What if the DT doesn't describe all 
the hardawre that is connected to GIC and Linux thinks its free to use 
but actually it's connected to a real hardware but no one told us about? 
I think since this information will always have to be looked up maybe 
it's better to give the responsibility to the user to specify something 
they know will work explicitly.

>
> This is just a 10 minutes braindump, so feel free to throw rocks at it
> and to come up with a better solution! :-)

Thanks for that. My brain is tied down to my use case to come up with 
something generic easily :-)

Any pointers on the best way to tie gic_send_ipi() with the driver/core 
code? The way it's currently tied to the core code is through SMP IPI 
functions which I don't think we can use. I'm thinking adding a pointer 
function in struct irq_chip would be the easiest approach maybe?

Thanks,
Qais

>
> Thanks,
>
> 	M.