Re: [PATCH v4 06/21] arm/acpi: Parse FADT table and get PSCI flags

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

On Sat, 23 Jan 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, the
> former signals to the OS that the hardware is PSCI compliant. The latter
> selects the appropriate conduit for PSCI calls by toggling between
> Hypervisor Calls (HVC) and Secure Monitor Calls (SMC). FADT table
> contains such information, parse FADT to get the flags for furture
> usage.
> 
> Since STAO table and the GIC version are introduced by ACPI 6.0, we will
> check the version and only parse FADT table with version >= 6.0. If
> firmware provides ACPI tables with ACPI version less than 6.0, OS will
> be messed up with those information, so disable ACPI if we get an FADT
> table with version less than 6.0.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> V4: drop disable_acpi in acpi_parse_fadt
> ---
>  xen/arch/arm/acpi/boot.c   | 30 ++++++++++++++++++++++++++++++
>  xen/arch/arm/acpi/lib.c    | 12 ++++++++++++
>  xen/include/asm-arm/acpi.h |  9 +++++++++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 1570f7e..6b33fbe 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -27,9 +27,32 @@
>  
>  #include <xen/init.h>
>  #include <xen/acpi.h>
> +#include <xen/errno.h>
> +#include <acpi/actables.h>
> +#include <xen/mm.h>
>  
>  #include <asm/acpi.h>
>  
> +static int __init acpi_parse_fadt(struct acpi_table_header *table)
> +{
> +    struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
> +
> +    /*
> +     * Revision in table header is the FADT Major revision, and there
> +     * is a minor revision of FADT which was introduced by ACPI 6.0,
> +     * we only deal with ACPI 6.0 or newer revision to get GIC and SMP
> +     * boot protocol configuration data, or we will disable ACPI.
> +     */
> +    if ( table->revision > 6
> +         || (table->revision == 6 && fadt->minor_revision >= 0) )
> +        return 0;
> +
> +    printk("Unsupported FADT revision %d.%d, should be 6.0+, will disable ACPI\n",
> +            table->revision, fadt->minor_revision);
> +
> +    return -EINVAL;
> +}
> +
>  /*
>   * acpi_boot_table_init() called from setup_arch(), always.
>   *      1. find RSDP and get its address, and then find XSDT
> @@ -54,5 +77,12 @@ int __init acpi_boot_table_init(void)
>          return error;
>      }
>  
> +    if ( acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt) )
> +    {
> +        /* disable ACPI if no FADT is found */
> +        disable_acpi();
> +        printk("Can't find FADT\n");
> +    }
> +
>      return 0;
>  }
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index f817fe6..a30e4e6 100644
> --- a/xen/arch/arm/acpi/lib.c
> +++ b/xen/arch/arm/acpi/lib.c
> @@ -50,3 +50,15 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>  
>  	return ((char *) base + offset);
>  }
> +
> +/* 1 to indicate PSCI 0.2+ is implemented */
> +bool_t __init acpi_psci_present(void)
> +{
> +    return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT;
> +}
> +
> +/* 1 to indicate HVC is present instead of SMC as the PSCI conduit */
> +bool_t __init acpi_psci_hvc_present(void)
> +{
> +    return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC;
> +}

So far so good.


> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 6a037c9..1ce88f8 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -31,6 +31,15 @@
>  #define ACPI_MAP_MEM_ATTR          PAGE_HYPERVISOR
>  
>  extern bool_t acpi_disabled;
> +
> +#ifdef CONFIG_ACPI
> +bool_t __init acpi_psci_present(void);
> +bool_t __init acpi_psci_hvc_present(void);
> +#else
> +static inline bool_t acpi_psci_present(void) { return false; }
> +static inline bool_t acpi_psci_hvc_present(void) {return false; }
> +#endif /* CONFIG_ACPI */
> +
>  /* Basic configuration for ACPI */
>  static inline void disable_acpi(void)
>  {

I would prefer if we only defined each function once, outside the ifdef
(no static inline needed). Then we could

#ifdef CONFIG_ACPI
extern bool_t acpi_disabled;
#else
#define acpi_disabled (1)
#endif

Which would solve the problem for !CONFIG_ACPI cases. But you need to be
careful to move bool_t acpi_disabled, enable_acpi and disable_acpi
inside an ifdef.