| 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
| ||
|
Message-ID: <84efcd26-2ea1-1727-87e8-f476c943aec5@arm.com>
Date: Tue, 5 Dec 2017 16:56:26 +0000
From: Marc Zyngier <marc.zyngier@....com>
To: shankerd@...eaurora.org,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>
Subject: Re: [PATCH] irqchip/gic-v3: Fix the driver probe() fail due to
disabled GICC entry
On 05/12/17 13:21, Shanker Donthineni wrote:
> Hi Marc,
>
> On 12/05/2017 02:59 AM, Marc Zyngier wrote:
>> On 04/12/17 14:04, Shanker Donthineni wrote:
>>> Hi Thanks,
>>>
>>> On 12/04/2017 04:28 AM, Marc Zyngier wrote:
>>>> On 03/12/17 23:21, Shanker Donthineni wrote:
>>>>> As per MADT specification, it's perfectly valid firmware can pass
>>>>> MADT table to OS with disabled GICC entries. ARM64-SMP code skips
>>>>> those cpu cores to bring online. However the current GICv3 driver
>>>>> probe bails out in this case on systems where redistributor regions
>>>>> are not in the always-on power domain.
>>>>>
>>>>> This patch does the two things to fix the panic.
>>>>> - Don't return an error in gic_acpi_match_gicc() for disabled GICC.
>>>>> - No need to keep GICR region information for disabled GICC.
>>>>>
>>>>> Kernel crash traces:
>>>>> Kernel panic - not syncing: No interrupt controller found.
>>>>> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.13.5 #26
>>>>> [<ffff000008087770>] dump_backtrace+0x0/0x218
>>>>> [<ffff0000080879dc>] show_stack+0x14/0x20
>>>>> [<ffff00000883b078>] dump_stack+0x98/0xb8
>>>>> [<ffff0000080c5c14>] panic+0x118/0x26c
>>>>> [<ffff000008b62348>] init_IRQ+0x24/0x2c
>>>>> [<ffff000008b609fc>] start_kernel+0x230/0x394
>>>>> [<ffff000008b601e4>] __primary_switched+0x64/0x6c
>>>>> ---[ end Kernel panic - not syncing: No interrupt controller found.
>>>>>
>>>>> Disabled GICC subtable example:
>>>>> Subtable Type : 0B [Generic Interrupt Controller]
>>>>> Length : 50
>>>>> Reserved : 0000
>>>>> CPU Interface Number : 0000003D
>>>>> Processor UID : 0000003D
>>>>> Flags (decoded below) : 00000000
>>>>> Processor Enabled : 0
>>>>> Performance Interrupt Trig Mode : 0
>>>>> Virtual GIC Interrupt Trig Mode : 0
>>>>> Parking Protocol Version : 00000000
>>>>> Performance Interrupt : 00000017
>>>>> Parked Address : 0000000000000000
>>>>> Base Address : 0000000000000000
>>>>> Virtual GIC Base Address : 0000000000000000
>>>>> Hypervisor GIC Base Address : 0000000000000000
>>>>> Virtual GIC Interrupt : 00000019
>>>>> Redistributor Base Address : 0000FFFF88F40000
>>>>> ARM MPIDR : 000000000000000D
>>>>> Efficiency Class : 00
>>>>> Reserved : 000000
>>>>>
>>>>> Signed-off-by: Shanker Donthineni <shankerd@...eaurora.org>
>>>>> ---
>>>>> drivers/irqchip/irq-gic-v3.c | 14 +++++++++-----
>>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>>> index b56c3e2..a30fbac 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>>> @@ -1331,6 +1331,10 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>>>>> u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
>>>>> void __iomem *redist_base;
>>>>>
>>>>> + /* GICC entry which has !ACPI_MADT_ENABLED is not unusable so skip */
>>>>> + if (!(gicc->flags & ACPI_MADT_ENABLED))
>>>>> + return 0;
>>>>> +
>>>>> redist_base = ioremap(gicc->gicr_base_address, size);
>>>>> if (!redist_base)
>>>>> return -ENOMEM;
>>>>> @@ -1374,13 +1378,13 @@ static int __init gic_acpi_match_gicc(struct acpi_subtable_header *header,
>>>>> (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 is enabled and has not valid gicr base address, then it means
>>>>> + * GICR base is not presented via GICC
>>>>> */
>>>>> - if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address)
>>>>> - return 0;
>>>>> + if ((gicc->flags & ACPI_MADT_ENABLED) && (!gicc->gicr_base_address))
>>>>> + return -ENODEV;
>>>>
>>>> This doesn't feel quite right. It would mean that having the ENABLED
>>>> flag cleared and potentially no address would make it valid? It looks to
>>>> me that the original code is "less wrong".
>>>>
>>>> What am I missing?
>>>>
>>>
>>> Original definition of the function gic_acpi_match_gicc().
>>> {
>>> if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address)
>>> return 0;
>>>
>>> return -ENODEV;
>>> }
>>>
>>> Above code triggers the driver probe fail for the two reasons.
>>> 1) GICC with ACPI_MADT_ENABLED=0, it's a bug according to ACPI spec.
>>> 2) GICC with ACPI_MADT_ENABLED=1 and invalid GICR address, expected.
>>>
>>>
>>> This patch fix the first failed case and keep the second case intact.
>>> if ((gicc->flags & ACPI_MADT_ENABLED) && (!gicc->gicr_base_address))
>>> return -ENODEV;
>>>
>>> return 0;
>> If (1) is a firmware bug, then why is it handled in the SMP code? You're
>> even saying that this is the right thing to do?
>>
>
> It's a bug in Linux GICv3 driver not firmware. Firmware is populating MADT
> table according to ACPI specification.
>
>> As for (2), you seem to imply that only the address matter. So why isn't
>> it just:
>>
>> if (gicc->gicr_base_address)
>> return 0;
>>
>> ?
>
> ACPI spec says operating shouldn't attempt to use GICC configuration parameters
> if the flag ACPI_MADT_ENABLED is cleared. I believe we should check GICR address
> only for enabled GICC interfaces.
Then please rewrite the commit message to actually explain this. This is
confusing as hell.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists