linux-kernel - Re: [PATCH RFC 2/2] irqchip/gic: Allow the use of SGI interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <112a725164b7fe321f27357fd4cd772f@www.loen.fr>
Date:   Wed, 23 Oct 2019 14:22:34 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        <bcm-kernel-feedback-list@...adcom.com>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Souvik Chakravarty <souvik.chakravarty@....com>,
        Jim Quinlan <james.quinlan@...adcom.com>,
        Sudeep Holla <sudeep.holla@....com>,
        Thanu Rangarajan <thanu.rangarajan@....com>
Subject: Re: [PATCH RFC 2/2] irqchip/gic: Allow the use of SGI interrupts

Hi Florian,

Needless to say, I mostly have questions...

On 2019-10-23 01:05, Florian Fainelli wrote:
> SGI interrupts are a convenient way for trusted firmware to target a
> specific set of CPUs. Update the ARM GIC code to allow the 
> translation
> and mapping of SGI interrupts.
>
> Since the kernel already uses SGIs for various inter-processor 
> interrupt
> activities, we specifically make sure that we do not let users of the
> IRQ API to even try to map those.
>
> Internal IPIs remain dispatched through handle_IPI() while public 
> SGIs
> get promoted to a normal interrupt flow management.
>
> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> ---
>  drivers/irqchip/irq-gic.c | 41 
> +++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 30ab623343d3..dcfdbaacdd64 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -385,7 +385,10 @@ static void __exception_irq_entry
> gic_handle_irq(struct pt_regs *regs)
>  			 * Pairs with the write barrier in gic_raise_softirq
>  			 */
>  			smp_rmb();
> -			handle_IPI(irqnr, regs);
> +			if (irqnr < NR_IPI)
> +				handle_IPI(irqnr, regs);
> +			else
> +				handle_domain_irq(gic->domain, irqnr, regs);

Double EOI, UNPREDICTABLE territory, your state machine is now dead.

>  #endif
>  			continue;
>  		}
> @@ -1005,20 +1008,34 @@ static int gic_irq_domain_translate(struct
> irq_domain *d,
>  		if (fwspec->param_count < 3)
>  			return -EINVAL;
>
> -		/* Get the interrupt number and add 16 to skip over SGIs */
> -		*hwirq = fwspec->param[1] + 16;
> -
> -		/*
> -		 * For SPIs, we need to add 16 more to get the GIC irq
> -		 * ID number
> -		 */
> -		if (!fwspec->param[0])
> +		*hwirq = fwspec->param[1];
> +		switch (fwspec->param[0]) {
> +		case 0:
> +			/*
> +			 * For SPIs, we need to add 16 more to get the GIC irq
> +			 * ID number
> +			 */
> +			*hwirq += 16;
> +			/* fall through */
> +		case 1:
> +			/* Add 16 to skip over SGIs */
>  			*hwirq += 16;
> +			*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
>
> -		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +			/* Make it clear that broken DTs are... broken */
> +			WARN_ON(*type == IRQ_TYPE_NONE);
> +			break;
> +		case 2:
> +			/* Refuse to map internal IPIs */
> +			if (*hwirq < NR_IPI)

So depending on how the kernel uses SGIs, you can or cannot use these 
SGIs.
That looks like a good way to corner ourselves into not being to change 
much.

Also, do you expect this to work for both Group-0 and Group-1 
interrupts
(since you imply that this works as a communication medium with the 
secure
side)? Given that the kernel running in NS has no way to enable/disable
Group-0 interrupts, this looks terminally flawed. Or is that Group-1 
only?

How do we describe which SGIs are guaranteed to be available to Linux?

> +				return -EPERM;
> +
> +			*type = IRQ_TYPE_NONE;

Or not. SGI are edge triggered, by definition.

> +			break;
> +		default:
> +			break;
> +		}
>
> -		/* Make it clear that broken DTs are... broken */
> -		WARN_ON(*type == IRQ_TYPE_NONE);

Really?

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