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

[Marc Zyngier <marc.zyngier@arm.com>]

[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).

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>;
};

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

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

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.

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).

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

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...