[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Wed, 4 Feb 2015, parth.dixit@xxxxxxxxxx wrote:
> From: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
> 
> 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.
> 
> NOTE: This commit allow to initialize GICv1/2 only.
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
> Signed-off-by: Graeme Gregory <graeme.gregory@xxxxxxxxxx>
> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> Signed-off-by: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> 
> Conflicts:
> 
>       xen/arch/arm/irq.c

Remove this from the commit message


>  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>
> +
> +/*
> + * 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)
> +
> +static DEFINE_PER_CPU(u64, gic_percpu_cpu_base);
> +static cpumask_t gic_acpi_cpu_mask;
> +static u64 dist_phy_base;

Could you reuse struct gicv2 and gicv2_info?


> +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))
> +                return -EINVAL;
> +        for_each_possible_cpu(cpu) {
> +        /*
> +         * 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;
> +        }

I don't understand, could you please elaborate?


> +        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);

please don't use DT_IRQ_TYPEs for acpi interrupt initialization


> +        }
> +
> +        /*
> +         * 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;
> +                        }
> +                }
> +        }
> +
> +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 */
> +
> +    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);

Almost everything in this function is common with the dt initialization
of the gic.  Please refactor the code to avoid code duplication.


> +    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;
> +        }
> +
> +        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);

This doesn't make sense: it can only fail if the table argument is NULL
in the first place.  Also your message prints something about the Local
APIC, that cannot be right.


> +        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;
> +}
> +#endif

At the very least acpi_gic_init should be in gic.c as it doesn't depend
on the gic version.


>  /*
>   * 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
>  
> -    gic_init();

As per the other init functions, please move the acpi_gic_init() call to
gic_init.


>      p2m_vmid_allocator_init();
>  
> 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*/

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.