linux-kernel - Re: [PATCH 1/2 V4] irqchip: gic: Add supports for ARM GICv2m MSI(-X)

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: <5409E1B0.5060703@arm.com>
Date:	Fri, 05 Sep 2014 17:15:44 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Suravee Suthikulpanit <suravee.suthikulpanit@....com>
CC:	Mark Rutland <Mark.Rutland@....com>,
	"jason@...edaemon.net" <jason@...edaemon.net>,
	Pawel Moll <Pawel.Moll@....com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Will Deacon <Will.Deacon@....com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"Harish.Kasiviswanathan@....com" <Harish.Kasiviswanathan@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/2 V4] irqchip: gic: Add supports for ARM GICv2m MSI(-X)

On 28/08/14 09:59, Suravee Suthikulpanit wrote:
> 
> 
> On 08/15/2014 09:03 AM, Marc Zyngier wrote:
>>> +
>>> +static struct irq_chip gicv2m_chip;
>>> +
>>> +#ifdef CONFIG_OF
>>
>> Is there any reason why this should be guarded by CONFIG_OF? Surely the
>> v2m capability should only be enabled if OF is.
> 
> [Suravee]
> We are also planning to support ACPI in the future also, which will be 
> using a different init function.  Also, there is the same #ifdef in the 
> irq-gic.c for the gic_of_init().  So, I am just trying to be consistent 
> here.
> 
> 
>>> +
>>> +       memcpy(&gicv2m_chip, gic->irq_chip, sizeof(struct irq_chip));
>>> +       gicv2m_chip.name = "GICv2m",
>>> +       gicv2m_chip.irq_mask = gicv2m_mask_irq;
>>> +       gicv2m_chip.irq_unmask = gicv2m_unmask_irq;
>>> +       gic->irq_chip = &gicv2m_chip;
>>
>> I liked it until this last line. You're overriding the irq_chip for the
>> whole GIC. I was expecting you'd only use it for the MSI range
>> (basically return a range to the caller, together with your brand new
>> irq_chip).
> 
> [Suravee]
> I'm not sure if I understand you point here. Actually, I don't see the 
> whole point of the need to have a whole different irq_chip for v2m 
> stuff.  All I need is just a way to overwrite the irq_chip.irq_mask() 
> and irq_chip.irq_unmask() with the v2m version which should check for 
> MSI before calling mask/unmask_msi_irq(). I should be able to just do:
> 
> 	gic->irq_chip.irq_mask = gicv2m_mask_irq;
> 	gic->irq_chip.irq_unmask = gicv2m_unmask_irq;

You should only do it for the few interrupts that are actually routed
via the v2m widget, not inflict the overhead on all interrupts.

This is why I insist on having a separate irqchip, and for this irqchip
to be only used for the MSI-generated interrupts.

Have a look at what I do for GICv3:
http://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3.c?h=gicv3/its-split&id=b717e532c4312a410a8ee0cb2349baa2769c6b94#n712

and how this gets used when routing the interrupts:
http://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3.c?h=gicv3/its-split&id=b717e532c4312a410a8ee0cb2349baa2769c6b94#n589

The ITS gets its own irqchip, entirely separate from the rest of the
GIC. This gives you the flexibility you require, and let the other
interrupts free of any overhead.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/