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

[Shannon Zhao <zhaoshenglong@huawei.com>]

On 2016/1/27 23:41, Stefano Stabellini wrote:
> 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
> 
Yes, we could do this to drop the #else (CONFIG_ACPI) case in some
places. But I think it still needs to stub out acpi_psci_present and
acpi_psci_hvc_present because they are used in some codes which are not
covered by #ifdef CONFIG_ACPI, see[1]. The file psci.c will be compiled
whether ACPI is enabled or not.

[1] [PATCH v4 09/21] arm/acpi: Add ACPI support for SMP initialization

> 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.
> 
> .
> 

-- 
Shannon