| 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: <529F4F39.9090001@arm.com>
Date: Wed, 04 Dec 2013 15:50:17 +0000
From: Marc Zyngier <marc.zyngier@....com>
To: Hanjun Guo <hanjun.guo@...aro.org>
CC: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Catalin Marinas <Catalin.Marinas@....com>,
Will Deacon <Will.Deacon@....com>,
Russell King - ARM Linux <linux@....linux.org.uk>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Mark Rutland <Mark.Rutland@....com>,
Matthew Garrett <mjg59@...f.ucam.org>,
"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
"patches@...aro.org" <patches@...aro.org>,
Linus Walleij <linus.walleij@...aro.org>,
Olof Johansson <olof@...om.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
Jon Masters <jonathan@...masters.org>,
"grant.likely@...aro.org" <grant.likely@...aro.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC part2 PATCH 9/9] ACPI / GIC: Initialize GIC using the information
in MADT
On 04/12/13 15:32, Hanjun Guo wrote:
> On 2013年12月04日 01:26, Marc Zyngier wrote:
>> Hi Hanjun,
>>
>> On 03/12/13 16:39, Hanjun Guo wrote:
>>> In MADT table, there are GIC cpu interface base address and
>>> GIC distributor base address, use them to convert GIC to ACPI.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
>>> ---
>>> arch/arm64/kernel/irq.c | 5 ++++
>>> drivers/acpi/plat/arm-core.c | 66 ++++++++++++++++++++++++++++++++++++------
>>> include/linux/acpi.h | 6 ++++
>>> 3 files changed, 68 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
>>> index 473e5db..a9e68bf 100644
>>> --- a/arch/arm64/kernel/irq.c
>>> +++ b/arch/arm64/kernel/irq.c
>>> @@ -25,6 +25,7 @@
>>> #include <linux/irq.h>
>>> #include <linux/smp.h>
>>> #include <linux/init.h>
>>> +#include <linux/acpi.h>
>>> #include <linux/irqchip.h>
>>> #include <linux/seq_file.h>
>>> #include <linux/ratelimit.h>
>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>>> void __init init_IRQ(void)
>>> {
>>> irqchip_init();
>>> +
>>> + if (!handle_arch_irq)
>>> + acpi_gic_init();
>>> +
>> Why is the GIC hardcoded?
>
> Very good question, thanks. I considered GIC only in my patch set.
> I have no idea how to handle the GIC hardcoded problem here for
> now, but I will figure it out later.
>
> If any suggestion, I will appreciate a lot.
>
>> How are you going to support other interrupt
>> controllers?
>
> ACPI 5.0 supports GICv2 only for now, if we want to
> support other interrupt controller, we should introduce
> some OEM table and parsing it, and it will not covered
> by this patch set.
>
>>> if (!handle_arch_irq)
>>> panic("No interrupt controller found.");
>>> }
>>> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
>>> index 17c99e1..509b847 100644
>>> --- a/drivers/acpi/plat/arm-core.c
>>> +++ b/drivers/acpi/plat/arm-core.c
>>> @@ -29,6 +29,7 @@
>>> #include <linux/module.h>
>>> #include <linux/irq.h>
>>> #include <linux/irqdomain.h>
>>> +#include <linux/irqchip/arm-gic.h>
>>> #include <linux/slab.h>
>>> #include <linux/bootmem.h>
>>> #include <linux/ioport.h>
>>> @@ -211,11 +212,21 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
>>> return 0;
>>> }
>>>
>>> +#ifdef CONFIG_ARM_GIC
>>> +/*
>>> + * Hard code here, we can not get memory size from MADT (but FDT does),
>>> + * this size is described in ARMv8 foudation model's User Guide
>>> + */
>>> +#define GIC_DISTRIBUTOR_MEMORY_SIZE (SZ_8K)
>>> +#define GIC_CPU_INTERFACE_MEMORY_SIZE (SZ_4K)
>> Aside from the incorrect sizes, how do you plan to address the other
>> regions that the GICv2 specification describes?
>
> Did these regions have the same base address? I mean the same
> as GIC distributor base address and GIC cpu interface base address.
>
> if yes, since the base address is stored in gic_init(), it can be for
> furture
> use. if I misunderstood your question, please let me know.
Look at the VGIC implementation for KVM in virt/kvm/arm. It does its own
probing of the additional regions used for virtualization.
The GIC and VGIC code are completely separate, and you'll need to find
an acceptable solution for that too.
>>> static int __init
>>> acpi_parse_gic_distributor(struct acpi_subtable_header *header,
>>> const unsigned long end)
>>> {
>>> struct acpi_madt_generic_distributor *distributor = NULL;
>>> + void __iomem *dist_base = NULL;
>>> + void __iomem *cpu_base = NULL;
>>>
>>> distributor = (struct acpi_madt_generic_distributor *)header;
>>>
>>> @@ -224,8 +235,43 @@ acpi_parse_gic_distributor(struct acpi_subtable_header *header,
>>>
>>> acpi_table_print_madt_entry(header);
>>>
>>> + /* GIC is initialised after page_init(), no need for early_ioremap */
>>> + dist_base = ioremap(distributor->base_address,
>>> + GIC_CPU_INTERFACE_MEMORY_SIZE);
>>> + if (!dist_base) {
>>> + pr_warn(PREFIX "unable to map gic dist registers\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + /*
>>> + * acpi_lapic_addr is stored in acpi_parse_madt(),
>>> + * so we can use it here for GIC init
>>> + */
>>> + if (acpi_lapic_addr) {
>>> + iounmap(dist_base);
>>> + pr_warn(PREFIX "Invalid GIC cpu interface base address\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + cpu_base = ioremap(acpi_lapic_addr, GIC_CPU_INTERFACE_MEMORY_SIZE);
>>> + if (!cpu_base) {
>>> + iounmap(dist_base);
>>> + pr_warn(PREFIX "unable to map gic cpu registers\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + gic_init(distributor->gic_id, -1, dist_base, cpu_base);
>>> +
>>> return 0;
>>> }
>>> +#else
>>> +static int __init
>>> +acpi_parse_gic_distributor(struct acpi_subtable_header *header,
>>> + const unsigned long end)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +#endif /* CONFIG_ARM_GIC */
>>>
>>> /*
>>> * Parse GIC cpu interface related entries in MADT
>>> @@ -234,7 +280,7 @@ acpi_parse_gic_distributor(struct acpi_subtable_header *header,
>>> static int __init acpi_parse_madt_gic_entries(void)
>>> {
>>> int count;
>>> -
>>> +
>>> /*
>>> * do a partial walk of MADT to determine how many CPUs
>>> * we have including disabled CPUs
>>> @@ -468,19 +514,21 @@ static void __init acpi_process_madt(void)
>>> * Parse MADT GIC cpu interface entries
>>> */
>>> error = acpi_parse_madt_gic_entries();
>>> - if (!error) {
>>> - /*
>>> - * Parse MADT GIC distributor entries
>>> - */
>>> - acpi_parse_madt_gic_distributor_entries();
>>> - }
>>> + if (!error)
>>> + pr_info("Using ACPI for processor (GIC) configuration information\n");
>>> }
>>>
>>> - pr_info("Using ACPI for processor (GIC) configuration information\n");
>>> -
>>> return;
>>> }
>>>
>>> +int __init acpi_gic_init(void)
>>> +{
>>> + /*
>>> + * Parse MADT GIC distributor entries
>>> + */
>>> + return acpi_parse_madt_gic_distributor_entries();
>>> +}
>>> +
>> Why can't you do the GIC init in the GIC code? We've tried hard to make
>> interrupt controllers discoverable and self contained.
>
> thanks for your suggestion, Rob also had the same suggestion,
> will try to update it in next version.
>
>> What are you
>> going to do when ACPI adds GICv3 to the mix? I don't really think this
>> model (shoving everything into the core ACPI code) is sustainable in the
>> long run...
>
> Since GICv3 related ACPI proposal is not public and not goes into ACPI
> spec, my suggestion is that we implement GICv2 only for now and post
> another patches for GICv3 when the new ACPI spec is available.
Certainly. But I think you should aim for a scalable solution right
away, instead of starting with something that we already know won't work
for stuff that is already around the corner (which is what I infer from
your "non public" statement).
Cheers,
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/
Powered by blists - more mailing lists