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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

+shannon

On 21 May 2015 at 17:59, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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@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>
>> ---
>>  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

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