Re: [PATCH v2 19/41] arm : acpi Add GIC specific ACPI boot support

[Julien Grall <julien.grall@citrix.com>]

Hi Parth,

On 17/05/15 21:03, Parth Dixit wrote:
> 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 GICv2.
> 
> Modify MADT table to clear GICH and GICV which are not needed
> in Dom0.
> 
> NOTE: This commit allow to initialize GICv2 only.
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@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>
> ---
>  xen/arch/arm/gic-v2.c       | 132 +++++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/gic.c          |  18 +++++-
>  xen/drivers/char/arm-uart.c | 136 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 284 insertions(+), 2 deletions(-)
>  create mode 100644 xen/drivers/char/arm-uart.c
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 7276951..fcdcd19 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -28,6 +28,8 @@
>  #include <xen/list.h>
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
> +#include <xen/acpi.h>
> +#include <acpi/actables.h>
>  #include <asm/p2m.h>
>  #include <asm/domain.h>
>  #include <asm/platform.h>
> @@ -35,6 +37,7 @@
>  
>  #include <asm/io.h>
>  #include <asm/gic.h>
> +#include <asm/acpi.h>
>  
>  /*
>   * LR register definitions are GIC v2 specific.
> @@ -692,9 +695,122 @@ static int __init dt_gicv2_init(void)
>      return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
> +static int __init
> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> +                        const unsigned long end)
> +{
> +        struct acpi_madt_generic_interrupt *processor;
> +
> +        processor = (struct acpi_madt_generic_interrupt *)header;
> +
> +        if (BAD_MADT_ENTRY(processor, end))

if ( ... )

> +                return -EINVAL;

The indentation looks wrong.

> +
> +        /* Read from APIC table and fill up the GIC variables */
> +
> +        gicv2.cbase = processor->base_address;
> +        gicv2.hbase = processor->gich_base_address;
> +        gicv2.vbase = processor->gicv_base_address;

This is per processor right? The value should be the same on each CPU as
we don't support non-banked GIC.

You would have to check that each value is the same on every CPU.

> +        gicv2_info.maintenance_irq = processor->vgic_maintenance_interrupt;
> +        if( processor->flags & ACPI_MADT_ENABLED )

The check doesn't seem necessary,

> +        {
> +            if( processor->flags & ACPI_MADT_VGIC )
> +                set_irq_type(gicv2_info.maintenance_irq, ACPI_IRQ_TYPE_EDGE_BOTH);
> +            else
> +                set_irq_type(gicv2_info.maintenance_irq, ACPI_IRQ_TYPE_LEVEL_MASK);

set_irq_type for local IRQ is very expensive. This should be done only one.

> +        }
> +
> +        processor->gich_base_address = 0;
> +        processor->gicv_base_address = 0;
> +        processor->vgic_maintenance_interrupt = 0;
> +        clean_dcache_va_range(processor, sizeof(struct acpi_madt_generic_interrupt));

Same remark as for the GTDT and MADT table. This doesn't belong to the
GIC initialization but DOM0 building.

Furthermore, the number of CPU for domain may be different as long as
the processor->flags.

Overall, I think this table should be recreated for DOM0.

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

if ( ... )

> +                return -EINVAL;

The indentation looks wrong

> +
> +        gicv2.dbase = dist->base_address;
> +
> +        return 0;
> +}
> +
> +static int __init acpi_gicv2_init(void)
> +{
> +    acpi_status status;
> +    struct acpi_table_header *table;
> +    u8 checksum;
> +    int res;

Given the usage of this variable I would rename it to count.

> +
> +    status = acpi_get_table(ACPI_SIG_MADT, 0, &table);

For a generic ACPI device code it would make sense to pass the ACPI
table in parameter as you will likely have to retrieve the same table
within the same class of device (see Serial too).

> +
> +    if ( ACPI_FAILURE(status) )
> +    {

Wrong indentation within the block.

> +              const char *msg = acpi_format_exception(status);

Missing blank line

> +              printk("\nFailed to get MADT table, %s\n", msg);

The first "\n" is not necessary.

> +              return 1;

Please return a meaningful error code.

> +    }
> +
> +    /* Collect CPU base addresses */
> +    res = acpi_parse_entries(ACPI_SIG_MADT,
> +                                sizeof(struct acpi_table_madt),
> +                                gic_acpi_parse_madt_cpu, table,
> +                                ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> +                                0);

Wrong indentation

> +    if ( res < 0 )	

0 means the no GICC which should be throwing an error too. I would
replace the check by

res <= 0

> +    {

> +        printk("Error during GICC entries parsing\n");
> +        printk("\nFailed to initialize GIC IRQ controller\n");

One single printk("No valid GICC entries exists\n") would have been enough.

> +        return -EINVAL;
> +    }
> +
> +    /*
> +     * Find distributor base address. We expect one distributor entry since
> +     * ACPI 5.0 spec neither support multi-GIC instances nor GIC cascade.
> +     */
> +    res = acpi_parse_entries(ACPI_SIG_MADT,
> +                                sizeof(struct acpi_table_madt),
> +                                gic_acpi_parse_madt_distributor, table,
> +                                ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> +                                0);

Wrong indentation

> +
> +    if ( res < 0 )

0 means no distributor which means the ACPI is not valid. I would
replace the check by

res <= 0


You also need to check that we don't have multiple GICD entry.

> +    {
> +        printk("Error during GICD entries parsing\n");
> +        printk("\nFailed to initialize GIC IRQ controller\n");

One single printk is enough.

> +        return -EINVAL;
> +    }
> +
> +    checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, table), table->length);
> +    table->checksum -= checksum;
> +    clean_dcache_va_range(table, sizeof(struct acpi_table_header));

This is coming out of nowhere without any explanation.

I guess this is related to the change in the ACPI table for DOM0, right?
If so, this should not be in the GIC initialization but in a specific
DOM0 building code.

> +    return 0;
> +}
> +#else
> +static int __init acpi_gicv2_init(void)
> +{
> +/* Should never reach here */
> +    return -EINVAL;
> +}
> +#endif
> +
>  static int gicv2_init(void)
>  {
> -     dt_gicv2_init();
> +    if( acpi_disabled )
> +        dt_gicv2_init();
> +    else
> +        acpi_gicv2_init();

acpi_gicv2_init is returning an error code. You should not ignore it.

>  
>      /* TODO: Add check on distributor, cpu size */
>  
> @@ -790,7 +906,21 @@ DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC)
>          .dt_match = gicv2_dt_match,
>          .init = dt_gicv2_preinit,
>  DT_DEVICE_END

Missing blank line.

> +#ifdef CONFIG_ACPI
> +/* Set up the GIC */
> +static int __init acpi_gicv2_preinit(const void *data)
> +{
> +    gicv2_info.hw_version = GIC_V2;
> +    register_gic_ops(&gicv2_ops);
> +
> +    return 0;
> +}
>  
> +ACPI_DEVICE_START(agicv2, "GICv2", DEVICE_GIC)
> +        .class_type = GIC_V2,
> +        .init = acpi_gicv2_preinit,
> +ACPI_DEVICE_END
> +#endif
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 701c306..e33bfd7 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -27,6 +27,7 @@
>  #include <xen/softirq.h>
>  #include <xen/list.h>
>  #include <xen/device_tree.h>
> +#include <xen/acpi.h>
>  #include <asm/p2m.h>
>  #include <asm/domain.h>
>  #include <asm/platform.h>
> @@ -34,6 +35,7 @@
>  #include <asm/io.h>
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
> +#include <asm/acpi.h>
>  
>  static void gic_restore_pending_irqs(struct vcpu *v);
>  
> @@ -261,9 +263,23 @@ void __init dt_gic_preinit(void)
>      dt_device_set_used_by(node, DOMID_XEN);
>  }
>  
> +static void __init acpi_gic_init(void)

This function should be called acpi_gic_preinit as it's used during
pre-initialization.

> +{
> +    int rc;
> +
> +    /* NOTE: Only one GIC is supported */
> +    /* hardcode to gic v2 for now */
> +    rc = acpi_device_init(DEVICE_GIC, NULL, GIC_V2);

This won't work for multiple GIC later. You should not try to use a
generic implementation just because it's there.

If you don't find a way to deal with genericity, I prefer a call to
acpi_gicv2_preinit directly.

> +    if ( rc )
> +        panic("Unable to find compatible GIC in the ACPI table");

This is not true. You check the presence of the GIC ACPI table later.

> +}
> +
>  void __init gic_preinit(void)
>  {
> -    dt_gic_preinit();
> +    if( acpi_disabled )
> +        dt_gic_preinit();
> +    else
> +        acpi_gic_init();
>  }
>  
>  /* Set up the GIC */
> diff --git a/xen/drivers/char/arm-uart.c b/xen/drivers/char/arm-uart.c
> new file mode 100644
> index 0000000..2603508
> --- /dev/null
> +++ b/xen/drivers/char/arm-uart.c

This file doesn't belong to this patch. I will review it when it will be
placed in the right patch.

Regards,

-- 
Julien Grall