Re: [RFC part2 PATCH 9/9] ACPI / GIC: Initialize GIC using theinformation in MADT

[Olof Johansson]

On Wed, Dec 04, 2013 at 03:50:17PM +0000, Marc Zyngier wrote:
> 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@xxxxxxxxxx>
> >>> ---
> >>>   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).

Again, I wonder if we might be better off converting GIC info (since
it's likely to be there on all systems) in the EFI boot wrapper into
FDT data when ACPI is provided.

Essentially, if we can describe:
* Memory
* Console uart (if one exists) for debug
* Timers
* Interrupts
(Possibly PCI host controllers too but I'm less sure that can be done
generically)

in the FDT stub, then we can keep a lot of the lowlevel init code common
instead of diverging. We can also get away from having to update both ACPI and
DT for GICv3, etc, reducing boilerplate code in the kernel to handle both.

If we have to add some properties to bring across some ACPI information we can
of course do so, hopefully it'll be a small amount.


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/