linux-kernel - Re: [PATCH 06/11] irqchip/gic-v3: Configure SGIs as standard 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: <20200612113900.09d53bd0@why>
Date:   Fri, 12 Jun 2020 11:39:00 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Russell King <linux@....linux.org.uk>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Sumit Garg <sumit.garg@...aro.org>, kernel-team@...roid.com
Subject: Re: [PATCH 06/11] irqchip/gic-v3: Configure SGIs as standard
 interrupts

Hi Valentin,

On Thu, 21 May 2020 15:04:54 +0100
Valentin Schneider <valentin.schneider@....com> wrote:

> On 19/05/20 17:17, Marc Zyngier wrote:
> > Change the way we deal with GICv3 SGIs by turning them into proper
> > IRQs, and calling into the arch code to register the interrupt range
> > instead of a callback.
> >
> > Signed-off-by: Marc Zyngier <maz@...nel.org>
> > ---
> >  drivers/irqchip/irq-gic-v3.c | 91 +++++++++++++++++++++---------------
> >  1 file changed, 53 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 23d7c87da407..d57289057b75 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1163,10 +1142,36 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> >
> >  static void gic_smp_init(void)
> >  {
> > -	set_smp_cross_call(gic_raise_softirq);
> > +	struct irq_fwspec sgi_fwspec = {
> > +		.fwnode		= gic_data.fwnode,
> > +	};
> > +	int base_sgi;
> > +
> >       cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
> >                                 "irqchip/arm/gicv3:starting",
> >                                 gic_starting_cpu, NULL);
> > +
> > +	if (is_of_node(gic_data.fwnode)) {
> > +		/* DT */
> > +		sgi_fwspec.param_count = 3;
> > +		sgi_fwspec.param[0] = GIC_IRQ_TYPE_SGI;
> > +		sgi_fwspec.param[1] = 0;
> > +		sgi_fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
> > +	} else {
> > +		/* ACPI */
> > +		sgi_fwspec.param_count = 2;
> > +		sgi_fwspec.param[0] = 0;
> > +		sgi_fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> > +	}
> > +
> > +	/* Register all 8 non-secure SGIs */
> > +	base_sgi = __irq_domain_alloc_irqs(gic_data.domain, -1, 8,
> > +					   NUMA_NO_NODE, &sgi_fwspec,
> > +					   false, NULL);  
> 
> So IIUC using irq_reserve_ipi() would require us to have a separate IPI
> domain, so instead here we can use a fwspec + the 'regular' GIC domain.

Indeed. Using an IPI domain wouldn't bring much. But the major point
against the current state of the IPI domain is that it sucks a bit for
our use case. We want interrupts to be contiguous in the Linux IRQ
space, and the IPI allocator prevents this.

But maybe I should just bite the bullet and hack that as well.

> One thing I see is that by not going through irq_reserve_ipi(), we don't set
> data->common->ipi_offset. I think this is all kzalloc'd, and we want an
> offset of 0 so it all works out, but this feels somewhat fragile.

So far, nothing is using this field on the limited piece of code we
use. But I agree, not the nicest behaviour.

> > +	if (WARN_ON(base_sgi <= 0))
> > +		return;
> > +
> > +	set_smp_ipi_range(base_sgi, 8);
> >  }
> >
> >  static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> > @@ -1289,6 +1296,13 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
> >
> >       switch (__get_intid_range(hw)) {
> >       case SGI_RANGE:
> > +		irq_set_percpu_devid(irq);
> > +		irq_domain_set_info(d, irq, hw, chip, d->host_data,
> > +				    handle_percpu_devid_fasteoi_ipi,
> > +				    NULL, NULL);
> > +		irq_set_status_flags(irq, IRQ_NOAUTOEN);  
> 
> FWIW IRQ_NOAUTOEN is already set by irq_set_percpu_devid_flags(), so that's
> not required. I know we do that for (E)PPIs, I think I already have a small
> patch stashed somewhere regarding that.

Already merged! ;-)

Thanks,

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