| 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
| ||
|
Date: Tue, 12 Jan 2016 23:05:41 +0800
From: Hanjun Guo <hanjun.guo@...aro.org>
To: Marc Zyngier <marc.zyngier@....com>,
Tomasz Nowicki <tn@...ihalf.com>, tglx@...utronix.de,
jason@...edaemon.net, rjw@...ysocki.net, lorenzo.pieralisi@....com,
robert.richter@...iumnetworks.com, shijie.huang@....com,
guohanjun@...wei.com, Suravee.Suthikulpanit@....com
CC: mw@...ihalf.com, graeme.gregory@...aro.org,
Catalin.Marinas@....com, will.deacon@....com,
linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, ddaney.cavm@...il.com
Subject: Re: [PATCH V2 03/10] irqchip,GICv3,ACPI: Add redistributor support
via GICC structures.
On 01/12/2016 08:03 PM, Marc Zyngier wrote:
> On 17/12/15 11:52, Tomasz Nowicki wrote:
>> On systems supporting GICv3 and above, in MADT GICC structures, the
>> field of GICR Base Address holds the 64-bit physical address of the
>> associated Redistributor if the GIC Redistributors are not in the
>> always-on power domain, so instead of init GICR regions via GIC
>> redistributor structure(s), init it with GICR base address in GICC
>> structures in that case.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
>> Signed-off-by: Tomasz Nowicki <tn@...ihalf.com>
>> ---
>> drivers/irqchip/irq-gic-v3.c | 98 ++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 89 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index c4b929c..0528e82 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -39,6 +39,7 @@
>> struct redist_region {
>> void __iomem *redist_base;
>> phys_addr_t phys_base;
>> + bool single_redist;
>> };
>>
>> struct gic_chip_data {
>> @@ -435,6 +436,9 @@ static int gic_populate_rdist(void)
>> return 0;
>> }
>>
>> + if (gic_data.redist_regions[i].single_redist)
>> + break;
>> +
>> if (gic_data.redist_stride) {
>> ptr += gic_data.redist_stride;
>> } else {
>> @@ -965,6 +969,7 @@ IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init);
>> #ifdef CONFIG_ACPI
>> static struct redist_region *redist_regs __initdata;
>> static u32 nr_redist_regions __initdata;
>> +static bool single_redist;
>>
>> static int __init
>> gic_acpi_register_redist(phys_addr_t phys_base, u64 size)
>> @@ -979,7 +984,8 @@ gic_acpi_register_redist(phys_addr_t phys_base, u64 size)
>> }
>>
>> redist_regs[count].phys_base = phys_base;
>> - redist_regs[count++].redist_base = redist_base;
>> + redist_regs[count].redist_base = redist_base;
>
> nit: move the count++ out of the access in the previous patch, this will
> make this series a bit easier to follow.
OK.
>
>> + redist_regs[count++].single_redist = single_redist;
>
> What is that single_redist for? Is that because you can't rely on
> GICR_TYPER.Last?
Yes, there is no GICR_TYPER.Last bit for some redistributors,
as it's valid for redistributor regions.
>
>> return 0;
>> }
>>
>> @@ -993,6 +999,48 @@ gic_acpi_parse_madt_redist(struct acpi_subtable_header *header,
>> return gic_acpi_register_redist(redist->base_address, redist->length);
>> }
>>
>> +static int __init
>> +gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + struct acpi_madt_generic_interrupt *gicc;
>> + void __iomem *redist_base;
>> + u64 typer;
>> + u32 size;
>> +
>> + gicc = (struct acpi_madt_generic_interrupt *)header;
>> + redist_base = ioremap(gicc->gicr_base_address, SZ_64K * 2);
>> + if (!redist_base)
>> + return -ENOMEM;
>> +
>> + typer = readq_relaxed(redist_base + GICR_TYPER);
>> + /* don't map reserved page as it's buggy to access it */
>> + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2;
>> + iounmap(redist_base);
>
> What a mess. If you discover a !VLPIS redistributor, why do you have to
> unmap it to remap it again? Also, please map the whole region for the
I think I missed something here, I didn't know it's GICv3 or v4, I need
to check the GICR_TYPER first, then decide map 2 or 4 SZ_64K pages.
> redistributor as we have on the DT side (4 64kB pages for VLPIS capable
> redistributors).
>
> Also, the spec says:
>
> "On systems supporting GICv3 and above, this field holds the 64-bit
> physical address of the associated Redistributor. If all of the GIC
> Redistributors are in the always-on power domain, GICR structures should
> be used to describe the Redistributors instead, and this field must be
> set to 0."
>
> which triggers two questions:
> - Can you access always the GICR_TYPER register without waking the
> redistributor up?
I missed this part, can you suggest how can we do that? accessing some
register before access to redistributor?
> - How do you cope with situations where some redistributors are in the
> always-on domain, and some are not?
I'm not sure if there is such hardware, if yes, do we need to fix
the spec first?
>
>> + return gic_acpi_register_redist(gicc->gicr_base_address, size);
>> +}
>> +
>> +static int __init gic_acpi_collect_gicr_base(void)
>> +{
>> + acpi_tbl_entry_handler redist_parser;
>> + enum acpi_madt_type type;
>> +
>> + if (single_redist) {
>> + type = ACPI_MADT_TYPE_GENERIC_INTERRUPT;
>> + redist_parser = gic_acpi_parse_madt_gicc;
>> + } else {
>> + type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
>> + redist_parser = gic_acpi_parse_madt_redist;
>> + }
>> +
>> + /* Collect redistributor base addresses in GICR entries */
>> + if (acpi_table_parse_madt(type, redist_parser, 0) > 0)
>> + return 0;
>> +
>> + pr_info("No valid GICR entries exist\n");
>> + return -ENODEV;
>> +}
>> +
>> static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header,
>> const unsigned long end)
>> {
>> @@ -1000,6 +1048,42 @@ static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header,
>> return 0;
>> }
>>
>> +static int __init gic_acpi_match_gicc(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + struct acpi_madt_generic_interrupt *gicc =
>> + (struct acpi_madt_generic_interrupt *)header;
>> +
>> + /*
>> + * If GICC is enabled and has valid gicr base address, then it means
>> + * GICR base is presented via GICC
>> + */
>> + if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address)
>> + return 0;
>> +
>> + return -ENODEV;
>> +}
>> +
>> +static int __init gic_acpi_count_gicr_regions(void)
>> +{
>> + int count;
>> +
>> + /* Count how many redistributor regions we have */
>> + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>> + gic_acpi_match_gicr, 0);
>> + if (count > 0) {
>> + single_redist = false;
>> + return count;
>> + }
>> +
>> + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>> + gic_acpi_match_gicc, 0);
>> + if (count > 0)
>> + single_redist = true;
>> +
>> + return count;
>> +}
>> +
>
> Here, you seem to consider GICR and GICC tables to be mutually
> exclusive. I don't think the spec says so.
Good question, I will ask Charles first about it.
Thanks
Hanjun
Powered by blists - more mailing lists