Re: [PATCH RFC 19/35] ACPI / GICv2: Add GIC specific ACPI boot support

[Julien Grall <julien.grall@linaro.org>]

Hi Parth,

On 04/02/2015 14:02, parth.dixit@linaro.org wrote:
> From: Naresh Bhat <naresh.bhat@linaro.org>
>
> ACPI on Xen hypervisor uses MADT table for proper GIC initialization.
> It needs to parse GIC related subtables, collect CPU interface and distributor
> addresses and call driver initialization function (which is hardware
> abstraction agnostic). In a similar way, FDT initialize GICv1/2.

No need to spec about GICv1, we don't support it on Xen.

> NOTE: This commit allow to initialize GICv1/2 only.

Ditto.

>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
>
> Conflicts:
>
> 	xen/arch/arm/irq.c

This may have come via a conflicted merge.

> ---
>   xen/arch/arm/gic-v2.c      | 271 +++++++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/setup.c       |   3 +-
>   xen/include/asm-arm/acpi.h |   2 +
>   3 files changed, 275 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index faad1ff..cb1d205 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -777,6 +777,277 @@ DT_DEVICE_START(gicv2, "GICv2:", DEVICE_GIC)
>           .init = gicv2_init,
>   DT_DEVICE_END
>
> +#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)
> +
> +#include <xen/acpi.h>
> +#include <xen/errno.h>
> +#include <xen/vmap.h>
> +#include <asm/acpi.h>

Please no include in the middle of the file.

> +
> +/*
> + * Hard code here, we can not get memory size from MADT (but FDT does),
> + * this size can be inferred from GICv2 spec
> + */
> +
> +#define ACPI_GIC_DIST_MEM_SIZE   0x00010000 // (SZ_64K)
> +#define ACPI_GIC_CPU_IF_MEM_SIZE 0x00002000 // (SZ_8K)

Why don't you use the proper define (SZ_64K/SZ_8k) rather than 
hardcoding it?

> +
> +static DEFINE_PER_CPU(u64, gic_percpu_cpu_base);
> +static cpumask_t gic_acpi_cpu_mask;
> +static u64 dist_phy_base;
> +
> +static int __init
> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> +                        const unsigned long end)
> +{
> +        struct acpi_madt_generic_interrupt *processor;
> +        unsigned int cpu;
> +
> +        processor = (struct acpi_madt_generic_interrupt *)header;
> +
> +        if (BAD_MADT_ENTRY(processor, end))
if ( ... )

> +                return -EINVAL;
> +        for_each_possible_cpu(cpu) {

for_each_possible( ... )
{

Please checking the coding style within this patch. I won't tell more 
foo ( ... )
{

in this patch.

> +        /*
> +         * FIXME: This condition is failing.
> +         * In Xen we first want to bring/initialize the GIC in hypervisor with single CPU
> +         * if (processor->mpidr == cpu_logical_map(cpu))
> +         */
> +                        goto find;
> +        }

Looking at the code in this function, this whole loop doesn't seem 
useful. It make the code more complicate to read. Why don't you validate 
directly the GIC addresses in this function?

This is what Linux does and it's divide by 5 the produced code.

> +
> +        printk("\nUnable to find CPU corresponding to GIC CPU entry [mpdir %lx]\n",
> +                (long)processor->mpidr);
> +
> +        return 0;
> +
> +find:
> +        /* Read from APIC table and fill up the GIC variables */
> +        gicv2.dbase = processor->redist_base_address;
> +        gicv2.cbase = processor->base_address;
> +        gicv2.hbase = processor->gich_base_address;
> +        gicv2.vbase = processor->gicv_base_address;
> +        gicv2_info.maintenance_irq = processor->vgic_maintenance_interrupt;
> +        if( processor->flags & ACPI_MADT_ENABLED )
> +        {
> +            if( processor->flags & ACPI_MADT_VGIC )
> +                acpi_set_irq(gicv2_info.maintenance_irq, DT_IRQ_TYPE_EDGE_BOTH);
> +            else
> +                acpi_set_irq(gicv2_info.maintenance_irq, DT_IRQ_TYPE_LEVEL_MASK);
> +        }
> +
> +        /*
> +         * Do not validate CPU i/f base, we can still use "Local Interrupt
> +         * Controller Address" from MADT header instead.
> +         */
> +        per_cpu(gic_percpu_cpu_base, cpu) = processor->base_address;
> +        cpumask_set_cpu(cpu, &gic_acpi_cpu_mask);
> +
> +        return 0;
> +}
> +
> +static int __init
> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
> +                                const unsigned long end)
> +{
> +        struct acpi_madt_generic_distributor *dist;
> +
> +        dist = (struct acpi_madt_generic_distributor *)header;
> +
> +        if (BAD_MADT_ENTRY(dist, end))
> +                return -EINVAL;
> +
> +        dist_phy_base = dist->base_address;
> +
> +        return 0;
> +}
> +
> +static int gic_acpi_validate_init(u64 madt_cpu_addr)
> +{
> +        void __iomem *cpu_base, *dist_base;
> +        u64 gic_cpu_base = 0;
> +        unsigned int cpu;
> +
> +        /* Process all GICC entries delivered by MADT */
> +        if (!cpumask_empty(&gic_acpi_cpu_mask)) {
> +                /*
> +                 * If MADT contains at least one GICC entry, it must be BSP
> +                 * dedicated.
> +                 */
> +                if (!cpumask_test_cpu(0, &gic_acpi_cpu_mask)) {
> +                        printk("GICC entries exist but unable to find BSP GICC "
> +                                "address\n");
> +                        goto madt_cpu_base;
> +                }
> +
> +                /*
> +                 * There is no support for non-banked GICv1/2 register in ACPI
> +                 * spec. All CPU interface addresses have to be the same.
> +                 */
> +                gic_cpu_base = per_cpu(gic_percpu_cpu_base, 0);
> +                for_each_cpu (cpu, &gic_acpi_cpu_mask) {
> +                        if (gic_cpu_base != per_cpu(gic_percpu_cpu_base, cpu)) {
> +                                printk("GICC addresses are different, no support"
> +                                        "for non-banked GICC registers !!!\n");
> +                                gic_cpu_base = 0;
> +                                goto madt_cpu_base;
> +                        }
> +                }
> +        }

Based on what I say above, this could be validate when fetch the GICC 
information.

> +
> +madt_cpu_base:
> +        /* If no GICC address provided, use address from MADT header */
> +        if (!gic_cpu_base) {
> +                if (!madt_cpu_addr) {
> +                        printk("Unable to find GICC address\n");
> +                        return -EINVAL;
> +                }
> +
> +                printk("Attempt to use Local Interrupt Controller Address"
> +                        "as GICC base address\n");
> +                gic_cpu_base = madt_cpu_addr;
> +        }
> +
> +        cpu_base = ioremap(gic_cpu_base, ACPI_GIC_CPU_IF_MEM_SIZE);
> +        if (!cpu_base) {
> +                printk("Unable to map GICC registers\n");
> +                return -ENOMEM;
> +        }
> +
> +        dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE);
> +        if (!dist_base) {
> +                printk("Unable to map GICD registers\n");
> +                iounmap(cpu_base);
> +                return -ENOMEM;
> +        }
> +
> +        /*
> +         * FIXME: Initialize zero GIC instance (no multi-GIC support) based on
> +         * addresses obtained from MADT. Also, set GIC as default IRQ domain
> +         * to allow for GSI registration and GSI to IRQ number translation
> +         * (see acpi_register_gsi() and acpi_gsi_to_irq()).
> +         *
> +         * gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL);
> +         * irq_set_default_host(gic_data[0].domain);
> +         */
> +
> +    /* TODO: Add check on distributor, cpu size */

Indentation ... All the code below could be shared with the DT version. 
Please create an helper to avoid duplication.

> +
> +    printk("GICv2 initialization from ACPI MADT table :\n"
> +              "        gic_dist_addr=%"PRIpaddr"\n"
> +              "        gic_cpu_addr=%"PRIpaddr"\n"
> +              "        gic_hyp_addr=%"PRIpaddr"\n"
> +              "        gic_vcpu_addr=%"PRIpaddr"\n"
> +              "        gic_maintenance_irq=%u\n",
> +              gicv2.dbase, gicv2.cbase, gicv2.hbase, gicv2.vbase,
> +              gicv2_info.maintenance_irq);
> +
> +    if ( (gicv2.dbase & ~PAGE_MASK) || (gicv2.cbase & ~PAGE_MASK) ||
> +         (gicv2.hbase & ~PAGE_MASK) || (gicv2.vbase & ~PAGE_MASK) )
> +        panic("GICv2 interfaces not page aligned");
> +
> +    gicv2.map_dbase = ioremap_nocache(gicv2.dbase, PAGE_SIZE);
> +    if ( !gicv2.map_dbase )
> +        panic("GICv2: Failed to ioremap for GIC distributor\n");
> +
> +    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, PAGE_SIZE);
> +
> +    if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> +        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE * 0x10,
> +                                           PAGE_SIZE);
> +    else
> +        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE, PAGE_SIZE);
> +
> +    if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
> +        panic("GICv2: Failed to ioremap for GIC CPU interface\n");
> +
> +    gicv2.map_hbase = ioremap_nocache(gicv2.hbase, PAGE_SIZE);
> +    if ( !gicv2.map_hbase )
> +        panic("GICv2: Failed to ioremap for GIC Virtual interface\n");
> +
> +    /* Global settings: interrupt distributor */
> +    spin_lock_init(&gicv2.lock);
> +    spin_lock(&gicv2.lock);
> +
> +    gicv2_dist_init();
> +    gicv2_cpu_init();
> +    gicv2_hyp_init();
> +
> +    spin_unlock(&gicv2.lock);
> +
> +    gicv2_info.hw_version = GIC_V2;
> +    register_gic_ops(&gicv2_ops);
> +
> +    return 0;
> +}
> +
> +int __init
> +gic_v2_acpi_init(struct acpi_table_header *table)
> +{
> +        struct acpi_table_madt *madt;
> +        int count;
> +
> +        /* Collect CPU base addresses */
> +        count = acpi_parse_entries(sizeof(struct acpi_table_madt),
> +                                   gic_acpi_parse_madt_cpu, table,
> +                                   ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> +                                   MAX_GIC_CPU_INTERFACE);
> +        if (count <= 0) {
> +                printk("Error during GICC entries parsing\n");
> +                return -EINVAL;
> +        }
> +
> +        /*
> +         * Find distributor base address. We expect one distributor entry since
> +         * ACPI 5.0 spec neither support multi-GIC instances nor GIC cascade.
> +         */
> +        count = acpi_parse_entries(sizeof(struct acpi_table_madt),
> +                                   gic_acpi_parse_madt_distributor, table,
> +                                   ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> +                                   MAX_GIC_DISTRIBUTOR);
> +        if (count <= 0) {
> +                printk("Error during GICD entries parsing\n");
> +                return -EINVAL;
> +        }

There should be only one Distributor. Please check count is always equal 
to 1.

> +
> +        madt = (struct acpi_table_madt *)table;
> +        return gic_acpi_validate_init((u64)madt->address);
> +}
> +
> +static int __init acpi_parse_madt(struct acpi_table_header *table)
> +{
> +        struct acpi_table_madt *madt = NULL;
> +        madt = (struct acpi_table_madt *)table;
> +
> +        if (!madt)
> +                return 1;
> +        else
> +                printk("Local APIC address 0x%08x\n", madt->address);
> +
> +        return 0;
> +}
> +
> +int __init acpi_gic_init()
> +{
> +       acpi_status status;
> +       int err;
> +
> +       status = acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt);
> +
> +       if (ACPI_FAILURE(status)) {
> +               const char *msg = acpi_format_exception(status);
> +               printk("\nFailed to get MADT table, %s\n", msg);
> +               return 1;
> +       }
> +
> +       err = acpi_table_parse(ACPI_SIG_MADT, gic_v2_acpi_init);
> +       if (err)
> +             printk("\nFailed to initialize GIC IRQ controller\n");
> +
> +       return 0;
> +}

I will make the same comment as on the UART driver. We should use a 
generic framework (maybe base on device). This is more cleaner and will 
make easy the support of GICv3/GICv2m.

> +#endif
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 317b985..93c8a8a 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -784,11 +784,12 @@ void __init start_xen(unsigned long boot_phys_offset,
>   /* Comment for now take it after GIC initialization */
>   #if defined(CONFIG_ACPI) && defined(CONFIG_ARM_64)
>      init_xen_acpi_time();
> +   acpi_gic_init();
>   #else
>       init_xen_time();
> +    gic_init();
>   #endif

Same remark as the timer, a same binary should work for ACPI and DT.

gic_init should be able to know if we use ACPI or DT and, based on it, 
correctly initialized the the GIC.

> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index c2d25db..01ce28d 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -106,5 +106,7 @@ static inline void acpi_disable_pci(void)
>   #endif
>
>   #define MAX_GIC_CPU_INTERFACE 65535
> +#define MAX_GIC_DISTRIBUTOR   1                /* should be the same as MAX_GIC_NR */
> +extern int __init acpi_gic_init(void);
>
>   #endif /*_ASM_ARM_ACPI_H*/
>

Regards,

-- 
Julien Grall