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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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();

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