Re: [PATCH RFC 05/35] ARM64 / ACPI: Parse FADT table to get PSCI flags

[Julien Grall <julien.grall@linaro.org>]

Hi Parth,

On 04/02/2015 14:01, parth.dixit@linaro.org wrote:
> From: Naresh Bhat <naresh.bhat@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. At the same time, only ACPI 5.1 or higher verison
> supports PSCI, and FADT Major.Minor version was introduced in ACPI
> 5.1, so we will check the version and only parse FADT table with
> version >= 5.1.
>
> If firmware provides ACPI tables with ACPI version less than 5.1,
> OS will be messed up with those information, so disable ACPI if we
> get an FADT table with version less that 5.1.
>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> ---
>   xen/arch/arm/arm64/acpi/arm-core.c | 54 +++++++++++++++++++++++++++++++++++---
>   xen/arch/arm/setup.c               |  1 +
>   xen/include/asm-arm/acpi.h         |  2 ++
>   3 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c
> index 50a83d6..2b7e2ef 100644
> --- a/xen/arch/arm/arm64/acpi/arm-core.c
> +++ b/xen/arch/arm/arm64/acpi/arm-core.c
> @@ -25,6 +25,7 @@
>   #if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)
>   #include <xen/init.h>
>   #include <xen/acpi.h>
> +#include <xen/errno.h>
>
>   #include <asm/acpi.h>
>
> @@ -42,6 +43,12 @@ EXPORT_SYMBOL(acpi_disabled);
>   int acpi_pci_disabled;         /* skip ACPI PCI scan and IRQ initialization */
>   EXPORT_SYMBOL(acpi_pci_disabled);
>
> +/* 1 to indicate PSCI is implemented */
> +int acpi_psci_present;

bool

> +
> +/* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */
> +int acpi_psci_use_hvc;

bool

> +
>   enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
>
>   struct acpi_arm_root acpi_arm_rsdp_info;     /* info about RSDP from FDT */
> @@ -58,7 +65,7 @@ EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
>    * failure: return =< 0
>    */
>   //int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
> -unsigned int acpi_register_gsi (u32 gsi, int edge_level, int active_high_low)
> +unsigned int acpi_register_gsi(u32 gsi, int edge_level, int active_high_low)

Spurious change.

>   {
>       return -1;
>   }
> @@ -69,6 +76,33 @@ void acpi_unregister_gsi(u32 gsi)
>   }
>   EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
>
> +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 version,
> +        * and there is a minor version of FADT which was introduced
> +        * by ACPI 5.1, we only deal with ACPI 5.1 or higher version
> +        * to get arm boot flags, or we will disable ACPI.
> +        */
> +       if ( table->revision < 5 || fadt->minor_version < 1 ) {
> +               printk("FADT version is %d.%d, no PSCI support, should be 5.1 or higher\n",
> +                       table->revision, fadt->minor_version);
> +               acpi_psci_present = 0;
> +               disable_acpi();
> +               return -EINVAL;
> +       }
> +
> +       if ( acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT )
> +               acpi_psci_present = 1;

You never use this function, so either something is wrong in this 
series, or I may have miss a patch... Does SMP boot is supported with 
ACPI on Xen? If so, how do you do it? All the PSCI code is based on the 
device tree. So by any chance, does Xen still use the DT?

> +
> +       if ( acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC )
> +               acpi_psci_use_hvc = 1;

This flags should never be enabled when for SMP bring up in the 
hypervisor. If it's the case, we should print an error.

> +
> +       return 0;
> +}
> +
>   /*
>    * acpi_boot_table_init() called from setup_arch(), always.
>    *      1. find RSDP and get its address, and then find XSDT
> @@ -81,12 +115,12 @@ int __init acpi_boot_table_init(void)
>   {
>       int error;
>       /* If acpi_disabled, bail out */
> -    if (acpi_disabled)
> +    if ( acpi_disabled )

This coding style change should belong to the patch which add the code 
(if I'm not mistaken patch #3).

>           return 1;
>
>       /* Initialize the ACPI boot-time table parser. */
>       error = acpi_table_init();
> -    if (error)
> +    if ( error )

Ditto.

>       {
>           disable_acpi();
>           return error;
> @@ -94,4 +128,18 @@ int __init acpi_boot_table_init(void)
>
>       return 0;
>   }
> +
> +int __init acpi_boot_init(void)
> +{
> +    int err = 0;
> +    /* If acpi_disabled, bail out */
> +    if (acpi_disabled)

if ( acpi_disabled )

> +        return -ENODEV;
> +
> +    err = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
> +    if ( err )
> +        printk("Can't find FADT\n");
> +
> +    return err;
> +}
>   #endif
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 7ae126b..3531d47 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -749,6 +749,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>
>   #if defined(CONFIG_ACPI) && defined(CONFIG_ARM_64)
>       acpi_boot_table_init();
> +    acpi_boot_init();

Why didn't you merge the 2 functions? In fine, initialize the table is 
part of the boot...

>   #endif
>
>       dt_unflatten_host_device_tree();
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index f6284b5..03051ef 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -60,6 +60,8 @@ extern int acpi_disabled;
>   extern int acpi_noirq;
>   extern int acpi_pci_disabled;
>   extern int acpi_strict;
> +extern int acpi_psci_present;
> +extern int acpi_psci_use_hvc;
>
>   /* map logic cpu id to physical APIC id
>    * APIC = GIC cpu interface on ARM
>

Regards,

-- 
Julien Grall