[RFC part2 PATCH 9/9] ACPI / GIC: Initialize GIC using the information in MADT

[hanjun.guo@linaro.org (Hanjun Guo)]

On 2013?12?04? 23:50, 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@linaro.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.

Ok, will review the VGIC code for KVM, thanks for the guidance.

>>>>    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).

yes, sure I will, thanks for your comments.

Thanks
Hanjun