Re: [PATCH v4 13/21] arm/gic-v2: Add ACPI boot support for GICv2

[Shannon Zhao <zhaoshenglong@huawei.com>]

On 2016/1/28 20:56, Stefano Stabellini wrote:
> On Sat, 23 Jan 2016, Shannon Zhao wrote:
>> From: Parth Dixit <parth.dixit@linaro.org>
>>
>> ACPI on Xen hypervisor uses MADT table for proper GIC initialization.
>> First get the GIC version from GIC Distributor. Then 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.
>>
>> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> V4: use container_of and fix coding style
>> ---
>>  xen/arch/arm/gic-v2.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 119 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index 668b863..7e46ee9 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -29,6 +29,8 @@
>>  #include <xen/device_tree.h>
>>  #include <xen/libfdt/libfdt.h>
>>  #include <xen/sizes.h>
>> +#include <xen/acpi.h>
>> +#include <acpi/actables.h>
>>  #include <asm/p2m.h>
>>  #include <asm/domain.h>
>>  #include <asm/platform.h>
>> @@ -36,6 +38,7 @@
>>  
>>  #include <asm/io.h>
>>  #include <asm/gic.h>
>> +#include <asm/acpi.h>
>>  
>>  /*
>>   * LR register definitions are GIC v2 specific.
>> @@ -681,11 +684,111 @@ static void __init gicv2_dt_init(void)
>>                 csize, vsize);
>>  }
>>  
>> +#ifdef CONFIG_ACPI
>> +static int __init
>> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>> +                        const unsigned long end)
>> +{
>> +    static int cpu_base_assigned = 0;
>> +    struct acpi_madt_generic_interrupt *processor =
>> +               container_of(header, struct acpi_madt_generic_interrupt, header);
>> +
>> +    if ( BAD_MADT_ENTRY(processor, end) )
>> +        return -EINVAL;
>> +
>> +    /* Read from APIC table and fill up the GIC variables */
>> +    if ( cpu_base_assigned == 0 )
>> +    {
>> +        cbase = processor->base_address;
>> +        csize = SZ_8K;
>> +        hbase = processor->gich_base_address;
>> +        vbase = processor->gicv_base_address;
>> +        gicv2_info.maintenance_irq = processor->vgic_interrupt;
>> +
>> +        if( processor->flags & ACPI_MADT_VGIC_IRQ_MODE )
> 
>             ^ if ( 
> 
> 
>> +            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
>> +        else
>> +            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
>> +
>> +        cpu_base_assigned = 1;
>> +    }
>> +    else
>> +    {
>> +        if ( cbase != processor->base_address
>> +             || hbase != processor->gich_base_address
>> +             || vbase != processor->gicv_base_address
>> +             || gicv2_info.maintenance_irq != processor->vgic_interrupt )
>> +        {
>> +            printk("GICv2: GICC entries are not same in MADT table\n");
>> +            return -EINVAL;
>> +        }
>> +    }
>> +
>> +    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 =
>> +             container_of(header, struct acpi_madt_generic_distributor, header);
>> +
>> +    if ( BAD_MADT_ENTRY(dist, end) )
>> +        return -EINVAL;
>> +
>> +    dbase = dist->base_address;
>> +
>> +    return 0;
>> +}
>> +
>> +static void __init gicv2_acpi_init(void)
>> +{
>> +    acpi_status status;
>> +    struct acpi_table_header *table;
>> +    int count;
>> +
>> +    status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
>> +
>> +    if ( ACPI_FAILURE(status) )
>> +    {
>> +        const char *msg = acpi_format_exception(status);
>> +
>> +        panic("GICv2: Failed to get MADT table, %s", msg);
>> +    }
>> +
>> +    /* Collect CPU base addresses */
>> +    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
>> +                               gic_acpi_parse_madt_cpu, table,
>> +                               ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
>> +    if ( count <= 0 )
>> +        panic("GICv2: No valid GICC entries exists");
>> +
>> +    /*
>> +     * 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(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
>> +                               gic_acpi_parse_madt_distributor, table,
>> +                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
>> +    if ( count <= 0 )
>> +        panic("GICv2: No valid GICD entries exists");
>> +}
>> +#else
>> +static void __init gicv2_acpi_init(void)
>> +{
>> +/* Should never reach here */
>> +}
>> +#endif
> 
> With acpi_disabled becoming an #define constant, this #else should not
> be needed.
> 
If I remove #else and define acpi_disabled as a constant as you
suggested, it has below compiling error on arm32.

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 9a9fcd5..28ec064 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -776,11 +776,6 @@ static void __init acpi_gicv2_init(void)
     if ( count <= 0 )
         panic("GICv2: No valid GICD entries exists");
 }
-#else
-static void __init acpi_gicv2_init(void)
-{
-/* Should never reach here */
-}
 #endif

gic-v2.c: In function 'gicv2_init':
gic-v2.c:793:9: error: implicit declaration of function
'acpi_gicv2_init' [-Werror=implicit-function-declaration]
gic-v2.c:793:9: error: nested extern declaration of 'acpi_gicv2_init'
[-Werror=nested-externs]
cc1: all warnings being treated as errors


> Provided that you make these 2 changes, you can add my Reviewed-by.
> 
> 
>>  static int __init gicv2_init(void)
>>  {
>>      uint32_t aliased_offset = 0;
>>  
>> -    gicv2_dt_init();
>> +    if ( acpi_disabled )
>> +        gicv2_dt_init();
>> +    else
>> +        gicv2_acpi_init();
>>  
>>      printk("GICv2 initialization:\n"
>>                "        gic_dist_addr=%"PRIpaddr"\n"
>> @@ -793,6 +896,21 @@ DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC)
>>          .init = gicv2_dt_preinit,
>>  DT_DEVICE_END
>>  
>> +#ifdef CONFIG_ACPI
>> +/* Set up the GIC */
>> +static int __init gicv2_acpi_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 = ACPI_MADT_GIC_VERSION_V2,
>> +        .init = gicv2_acpi_preinit,
>> +ACPI_DEVICE_END
>> +#endif
>>  /*
>>   * Local variables:
>>   * mode: C
>> -- 
>> 2.0.4
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
> 
> .
> 

-- 
Shannon


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