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

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Thu, 25 Feb 2016, Stefano Stabellini wrote:
> On Thu, 25 Feb 2016, Shannon Zhao wrote:
> > 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@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
> > > 
> > > 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);
> 
> It should be
> 
>    void gicv2_acpi_init(void);
> 
> without "static".

However I am starting to doubt that this is actually an good improvement
in terms of code quality. So I would be also OK with keeping things are
they are.

 
> >  #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 */
> > -}

but if you do that please make the stub function a single line:

  static void __init gicv2_acpi_init(void) { }


> >  #endif
> > 
> >  static int __init gicv2_init(void)

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