linux-kernel - Re: [PATCH v4 06/10] irqchip / GICv3: Add ACPI support for GICv3+ initialization

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]

Date:	Tue, 11 Aug 2015 15:19:37 +0800
From:	Hanjun Guo <hanjun.guo@...aro.org>
To:	Marc Zyngier <marc.zyngier@....com>,
	Jason Cooper <jason@...edaemon.net>,
	Will Deacon <Will.Deacon@....com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Jiang Liu <jiang.liu@...ux.intel.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	"suravee.suthikulpanit@....com" <suravee.suthikulpanit@....com>,
	Timur Tabi <timur@...eaurora.org>,
	Tomasz Nowicki <tomasz.nowicki@...aro.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Mark Brown <broonie@...nel.org>, Wei Huang <wei@...hat.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>
Subject: Re: [PATCH v4 06/10] irqchip / GICv3: Add ACPI support for GICv3+
 initialization

Hi Marc,

On 08/07/2015 12:42 AM, Marc Zyngier wrote:
> On 05/08/15 15:00, Hanjun Guo wrote:
>> On 08/04/2015 09:17 PM, Marc Zyngier wrote:
>>> On 29/07/15 11:08, Hanjun Guo wrote:
>>>> From: Tomasz Nowicki <tomasz.nowicki@...aro.org>
>>>>
>>>> With the refator of gic_of_init(), GICv3/4 can be initialized
>>>> by gic_init_bases() with gic distributor base address and gic
>>>> redistributor region(s).
>>>>
>>>> So get the redistributor region base addresses from MADT GIC
>>>> redistributor subtable, and the distributor base address from
>>>> GICD subtable to init GICv3 irqchip in ACPI way.
>>>>
>>>> Note: GIC redistributor base address may also be provided in
>>>> GICC structures on systems supporting GICv3 and above if the GIC
>>>> Redistributors are not in the always-on power domain, this
>>>> patch didn't implement such feature yet.
>>>>
>>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@...aro.org>
>>>> [hj: Rework this patch and fix multi issues]
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
>>>> ---
>>>>    drivers/irqchip/irq-gic-v3.c | 174 +++++++++++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 169 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>> index c0b96c6..ebc5604 100644
>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>> @@ -15,6 +15,7 @@
>>>>     * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>     */
>>>>
>>>> +#include <linux/acpi.h>
>>>>    #include <linux/cpu.h>
>>>>    #include <linux/cpu_pm.h>
>>>>    #include <linux/delay.h>
>>>> @@ -25,6 +26,7 @@
>>>>    #include <linux/percpu.h>
>>>>    #include <linux/slab.h>
>>>>
>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>    #include <linux/irqchip/arm-gic-v3.h>
>>>>
>>>>    #include <asm/cputype.h>
>>>> @@ -802,7 +804,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
>>>>       set_handle_irq(gic_handle_irq);
>>>>
>>>>       if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>>>> -            its_init(domain_token, &gic_data.rdists, gic_data.domain);
>>>> +            its_init(irq_domain_token_to_of_node(domain_token),
>>>> +                     &gic_data.rdists, gic_data.domain);
>>>
>>> This doesn't make much sense. The first parameter to its_init is indeed
>>> supposed to be an of_node, but what is the point of calling its_init if
>>> you *know* you don't have the necessary topological information to parse
>>> the firmware tables?
>>>
>>> I don't see *any* code that is going to parse the ITS table in this
>>> series, so please don't call its_init passing a NULL pointer to it. This
>>> is just gross.
>>
>> OK, the ITS ACPI code is in later patch which combined with IORT. How
>> about moving it to the GIC of init code temporary?
>
> Just don't call its_init if irq_domain_token_to_of_node(domain_token) is
> NULL. But don't call it with a NULL parameter.

OK.

[...]

>>>> +}
>>>> +IRQCHIP_ACPI_DECLARE(gic_v3, ACPI_MADT_GIC_VERSION_V3, gic_acpi_init);
>>>> +IRQCHIP_ACPI_DECLARE(gic_v4, ACPI_MADT_GIC_VERSION_V4, gic_acpi_init);
>>>
>>> As mentioned before, this doesn't work.
>>
>> hmm, I think we need more discussion for this one, but we need to match
>> V4 for GICv3 drivers, and everything will be the same in the dirver
>> as I said before.
>
> And as I said before, you don't need to distinguish v3 from v4 in the
> ACPI code. Matching GICv3 is enough.

OK, how about the following code when parsing the GIC version?

/*
  * GICv3 driver can find out it's V3 or V4 itself, just set the
  * gic_version to V3 to match the driver if firmware presented V4.
  */
if (gic_version == ACPI_MADT_GIC_VERSION_V4)
         gic_version = ACPI_MADT_GIC_VERSION_V3;

Thanks
Hanjun


--
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/