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

[Shannon Zhao <zhaoshenglong@xxxxxxxxxx>]

On 2016/2/25 18:42, Stefano Stabellini wrote:
> On Thu, 25 Feb 2016, Shannon Zhao wrote:
>> On 2016/1/28 20:56, Stefano Stabellini wrote:
>>> On Sat, 23 Jan 2016, Shannon Zhao wrote:
>>>> From: Parth Dixit <parth.dixit@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
>>>> Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>>>>
>>>> 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
> 
> Sorry, I should have been clearer. You still need to have a declaration
> of the function outside the #ifdef. Something like the following should
> work:
> 
>   static void acpi_gicv2_init(void);
> 
>   [...]
> 
>   #ifdef CONFIG_ACPI
>   static void __init acpi_gicv2_init(void)
>   {
>       /* do something */
>   }
>   #endif
> 
>   [...]
> 
>   if ( acpi_disabled )
>       gicv2_dt_init();
>   else
>       gicv2_acpi_init();
> 
> .
> 
I'm afraid it doesn't work. Still has below errors with the declaration:

gic-v2.c:687:13: error: 'gicv2_acpi_init' used but never defined [-Werror]
cc1: all warnings being treated as errors

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 4b6c4c0..7b7e094 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -684,6 +684,8 @@ static void __init gicv2_dt_init(void)
                csize, vsize);
 }

+static void gicv2_acpi_init(void);
+
 #ifdef CONFIG_ACPI
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
@@ -774,11 +776,6 @@ static void __init gicv2_acpi_init(void)
     if ( count <= 0 )
         panic("GICv2: No valid GICD entries exists");
 }
-#else
-static void __init gicv2_acpi_init(void)
-{
-/* Should never reach here */
-}
 #endif

 static int __init gicv2_init(void)

-- 
Shannon


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