| Message ID | 1407166105-17675-6-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 04, 2014 at 04:28:12PM +0100, Hanjun Guo wrote: > There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, > the former signals to the OS that the hardware is PSCI compliant. Actually it signals that the firmware is PSCI compliant. The hardware doesn't care much. > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index 6400312..6e04868 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -19,6 +19,18 @@ extern int acpi_disabled; > extern int acpi_noirq; > extern int acpi_pci_disabled; > > +/* 1 to indicate PSCI 0.2+ is implemented */ > +static inline bool acpi_psci_present(void) > +{ > + return !!(acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT); > +} > + > +/* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */ > +static inline bool acpi_psci_use_hvc(void) > +{ > + return !!(acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC); > +} Do we actually need !! here? Shouldn't the compiler figure out conversion to bool automatically? > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 9cf9127..69a315d 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -11,6 +11,8 @@ > * published by the Free Software Foundation. > */ > > +#define pr_fmt(fmt) "ACPI: " fmt > + > #include <linux/init.h> > #include <linux/acpi.h> > #include <linux/cpumask.h> > @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned long size) > early_memunmap(map, size); > } > > +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_revision < 1) { If we ever get revision 6.0, this would trigger. > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index 85c6326..dfc4e4f3 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -395,6 +395,8 @@ void __init setup_arch(char **cmdline_p) > efi_idmap_init(); > > cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; > + acpi_boot_init(); > + > unflatten_device_tree(); Unless that's changed in a subsequent patch, do we still need to call unflatten_device_tree() if ACPI was successful? > psci_init(); I would also rename this to something like psci_dt_init() and move the acpi_disabled check here rather than in the callee.
On 04/08/14 16:28, Hanjun Guo wrote: > 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 PSCI init. Since ACPI 5.1 doesn't support self defined PSCI > function IDs, which means that only PSCI 0.2+ is supported in ACPI. > > 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 and have no way to > bring up secondery CPUs, 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: Graeme Gregory <graeme.gregory@linaro.org> > --- > arch/arm64/include/asm/acpi.h | 12 ++++++ > arch/arm64/kernel/acpi.c | 37 +++++++++++++++++ > arch/arm64/kernel/psci.c | 89 ++++++++++++++++++++++++++++++----------- > arch/arm64/kernel/setup.c | 2 + > 4 files changed, 117 insertions(+), 23 deletions(-) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index 6400312..6e04868 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -19,6 +19,18 @@ extern int acpi_disabled; > extern int acpi_noirq; > extern int acpi_pci_disabled; > > +/* 1 to indicate PSCI 0.2+ is implemented */ > +static inline bool acpi_psci_present(void) > +{ > + return !!(acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT); > +} > + > +/* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */ > +static inline bool acpi_psci_use_hvc(void) > +{ > + return !!(acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC); > +} > + > static inline void disable_acpi(void) > { > acpi_disabled = 1; > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 9cf9127..69a315d 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -11,6 +11,8 @@ > * published by the Free Software Foundation. > */ > > +#define pr_fmt(fmt) "ACPI: " fmt > + > #include <linux/init.h> > #include <linux/acpi.h> > #include <linux/cpumask.h> > @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned long size) > early_memunmap(map, size); > } > > +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_revision < 1) { && > + pr_info("FADT version is %d.%d, no PSCI support, should be 5.1 or higher\n", > + table->revision, fadt->minor_revision); > + disable_acpi(); Instead of disabling ACPI at multiple places in the boot sequence, can't it be done once if acpi_boot_init failed ? > + return -EINVAL; > + } > + > + return 0; > +} > + > /* > * acpi_boot_table_init() called from setup_arch(), always. > * 1. find RSDP and get its address, and then find XSDT > @@ -68,6 +90,21 @@ void __init acpi_boot_table_init(void) > } > } > > +int __init acpi_boot_init(void) > +{ > + int err = 0; > + > + /* If acpi_disabled, bail out */ > + if (acpi_disabled) > + return -ENODEV; > + > + err = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt); > + if (err) > + pr_err("Can't find FADT\n"); > + > + return err; > +} > + > /* > * acpi_suspend_lowlevel() - save kernel state and suspend. > * > diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c > index 9e9798f..2ad2084 100644 > --- a/arch/arm64/kernel/psci.c > +++ b/arch/arm64/kernel/psci.c > @@ -15,6 +15,7 @@ > > #define pr_fmt(fmt) "psci: " fmt > > +#include <linux/acpi.h> > #include <linux/init.h> > #include <linux/of.h> > #include <linux/smp.h> > @@ -231,6 +232,33 @@ static void psci_sys_poweroff(void) > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); > } > > +static void psci_0_2_set_functions(void) > +{ > + pr_info("Using standard PSCI v0.2 function IDs\n"); > + psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND; > + psci_ops.cpu_suspend = psci_cpu_suspend; > + > + psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF; > + psci_ops.cpu_off = psci_cpu_off; > + > + psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON; > + psci_ops.cpu_on = psci_cpu_on; > + > + psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE; > + psci_ops.migrate = psci_migrate; > + > + psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO; > + psci_ops.affinity_info = psci_affinity_info; > + > + psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] = > + PSCI_0_2_FN_MIGRATE_INFO_TYPE; > + psci_ops.migrate_info_type = psci_migrate_info_type; > + > + arm_pm_restart = psci_sys_reset; > + > + pm_power_off = psci_sys_poweroff; > +} > + Nit: IMO may be you can move these refactoring as separate patch before ACPI related changes ? > /* > * PSCI Function IDs for v0.2+ are well defined so use > * standard values. > @@ -264,29 +292,7 @@ static int psci_0_2_init(struct device_node *np) > } > } > > - pr_info("Using standard PSCI v0.2 function IDs\n"); > - psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND; > - psci_ops.cpu_suspend = psci_cpu_suspend; > - > - psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF; > - psci_ops.cpu_off = psci_cpu_off; > - > - psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON; > - psci_ops.cpu_on = psci_cpu_on; > - > - psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE; > - psci_ops.migrate = psci_migrate; > - > - psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO; > - psci_ops.affinity_info = psci_affinity_info; > - > - psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] = > - PSCI_0_2_FN_MIGRATE_INFO_TYPE; > - psci_ops.migrate_info_type = psci_migrate_info_type; > - > - arm_pm_restart = psci_sys_reset; > - > - pm_power_off = psci_sys_poweroff; > + psci_0_2_set_functions(); > > out_put_node: > of_node_put(np); > @@ -333,6 +339,40 @@ out_put_node: > return err; > } > > +#ifdef CONFIG_ACPI > +static int get_set_conduit_method_acpi(void) > +{ > + pr_info("probing for conduit method from ACPI.\n"); > + > + if (acpi_psci_use_hvc()) > + invoke_psci_fn = __invoke_psci_fn_hvc; > + else > + invoke_psci_fn = __invoke_psci_fn_smc; > + > + return 0; > +} > + > +/* We use PSCI 0.2+ when ACPI is deployed */ > +static int psci_0_2_init_acpi(void) May be this can be renamed psci_acpi_init and called from setup directly instead of calling from DT function with acpi_disabled check. > +{ > + if (!acpi_psci_present()) { > + pr_info("is not implemented in ACPI.\n"); > + return -EOPNOTSUPP; > + } > + > + get_set_conduit_method_acpi(); > + > + psci_0_2_set_functions(); > + > + return 0; > +} > +#else > +static inline int psci_0_2_init_acpi(void) > +{ > + return -ENODEV; > +} > +#endif > + > static const struct of_device_id psci_of_match[] __initconst = { > { .compatible = "arm,psci", .data = psci_0_1_init}, > { .compatible = "arm,psci-0.2", .data = psci_0_2_init}, > @@ -345,6 +385,9 @@ int __init psci_init(void) > const struct of_device_id *matched_np; > psci_initcall_t init_fn; > > + if (!acpi_disabled) > + return psci_0_2_init_acpi(); > + As mentioned above you need not modify this. > np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np); > > if (!np) > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index 85c6326..dfc4e4f3 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -395,6 +395,8 @@ void __init setup_arch(char **cmdline_p) > efi_idmap_init(); > > cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; > + acpi_boot_init(); > + Check for the return error here and disable ACPI once rather than at multiple places all over. > unflatten_device_tree(); > > psci_init(); >
On 2014-8-18 22:27, Catalin Marinas wrote: > On Mon, Aug 04, 2014 at 04:28:12PM +0100, Hanjun Guo wrote: >> There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, >> the former signals to the OS that the hardware is PSCI compliant. > > Actually it signals that the firmware is PSCI compliant. The hardware > doesn't care much. Right, I will update it. > >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >> index 6400312..6e04868 100644 >> --- a/arch/arm64/include/asm/acpi.h >> +++ b/arch/arm64/include/asm/acpi.h >> @@ -19,6 +19,18 @@ extern int acpi_disabled; >> extern int acpi_noirq; >> extern int acpi_pci_disabled; >> >> +/* 1 to indicate PSCI 0.2+ is implemented */ >> +static inline bool acpi_psci_present(void) >> +{ >> + return !!(acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT); >> +} >> + >> +/* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */ >> +static inline bool acpi_psci_use_hvc(void) >> +{ >> + return !!(acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC); >> +} > > Do we actually need !! here? Shouldn't the compiler figure out > conversion to bool automatically? I thought !! will explicitly show that it's a bool value and improve the readability of the code, but I'm ok to remove !! here. > >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >> index 9cf9127..69a315d 100644 >> --- a/arch/arm64/kernel/acpi.c >> +++ b/arch/arm64/kernel/acpi.c >> @@ -11,6 +11,8 @@ >> * published by the Free Software Foundation. >> */ >> >> +#define pr_fmt(fmt) "ACPI: " fmt >> + >> #include <linux/init.h> >> #include <linux/acpi.h> >> #include <linux/cpumask.h> >> @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned long size) >> early_memunmap(map, size); >> } >> >> +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_revision < 1) { > > If we ever get revision 6.0, this would trigger. Yes, good catch, actually I already fixed that in my local git repo, + if (table->revision > 5 || + (table->revision == 5 && fadt->minor_revision >= 1)) { + return 0; + } else { + pr_info("FADT revision is %d.%d, no PSCI support, should be 5.1 or higher\n", + table->revision, fadt->minor_revision); + disable_acpi(); + return -EINVAL; + } > >> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c >> index 85c6326..dfc4e4f3 100644 >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -395,6 +395,8 @@ void __init setup_arch(char **cmdline_p) >> efi_idmap_init(); >> >> cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; >> + acpi_boot_init(); >> + >> unflatten_device_tree(); > > Unless that's changed in a subsequent patch, do we still need to call > unflatten_device_tree() if ACPI was successful? No, we don't. in [PATCH v2 16/18], we will not call unflatten_device_tree() if ACPI is successful. Since the CONFIG_ACPI is not enabled for ARM64 (will enable it in the last patch), so acpi_boot_init() is a stub empty function here. > >> psci_init(); > > I would also rename this to something like psci_dt_init() and move the > acpi_disabled check here rather than in the callee. thanks for the suggestion, I will update my patch :) Thanks Hanjun
On 2014-8-19 2:32, Sudeep Holla wrote: > On 04/08/14 16:28, Hanjun Guo wrote: >> 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 PSCI init. Since ACPI 5.1 doesn't support self defined PSCI >> function IDs, which means that only PSCI 0.2+ is supported in ACPI. >> >> 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 and have no way to >> bring up secondery CPUs, so disable ACPI if we get an FADT table >> with version less that 5.1. >> [...] >> >> +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_revision < 1) { > > && Catalin also pointed out this bug :), but && is not enough, for example, this would not trigger of revision 4.1, although 4.1 is not exist, but the code will still confusing people, so I updated the code as: + if (table->revision > 5 || + (table->revision == 5 && fadt->minor_revision >= 1)) { + return 0; + } else { + pr_info("FADT revision is %d.%d, no PSCI support, should be 5.1 or higher\n", + table->revision, fadt->minor_revision); + disable_acpi(); + return -EINVAL; + } > >> + pr_info("FADT version is %d.%d, no PSCI support, should be 5.1 or >> higher\n", >> + table->revision, fadt->minor_revision); >> + disable_acpi(); > > Instead of disabling ACPI at multiple places in the boot sequence, can't > it be done once if acpi_boot_init failed ? > >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> /* >> * acpi_boot_table_init() called from setup_arch(), always. >> * 1. find RSDP and get its address, and then find XSDT >> @@ -68,6 +90,21 @@ void __init acpi_boot_table_init(void) >> } >> } >> >> +int __init acpi_boot_init(void) >> +{ >> + int err = 0; >> + >> + /* If acpi_disabled, bail out */ >> + if (acpi_disabled) >> + return -ENODEV; >> + >> + err = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt); >> + if (err) >> + pr_err("Can't find FADT\n"); >> + >> + return err; >> +} >> + >> /* >> * acpi_suspend_lowlevel() - save kernel state and suspend. >> * >> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c >> index 9e9798f..2ad2084 100644 >> --- a/arch/arm64/kernel/psci.c >> +++ b/arch/arm64/kernel/psci.c >> @@ -15,6 +15,7 @@ >> >> #define pr_fmt(fmt) "psci: " fmt >> >> +#include <linux/acpi.h> >> #include <linux/init.h> >> #include <linux/of.h> >> #include <linux/smp.h> >> @@ -231,6 +232,33 @@ static void psci_sys_poweroff(void) >> invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); >> } >> >> +static void psci_0_2_set_functions(void) >> +{ >> + pr_info("Using standard PSCI v0.2 function IDs\n"); >> + psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND; >> + psci_ops.cpu_suspend = psci_cpu_suspend; >> + >> + psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF; >> + psci_ops.cpu_off = psci_cpu_off; >> + >> + psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON; >> + psci_ops.cpu_on = psci_cpu_on; >> + >> + psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE; >> + psci_ops.migrate = psci_migrate; >> + >> + psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO; >> + psci_ops.affinity_info = psci_affinity_info; >> + >> + psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] = >> + PSCI_0_2_FN_MIGRATE_INFO_TYPE; >> + psci_ops.migrate_info_type = psci_migrate_info_type; >> + >> + arm_pm_restart = psci_sys_reset; >> + >> + pm_power_off = psci_sys_poweroff; >> +} >> + > > Nit: IMO may be you can move these refactoring as separate patch before > ACPI related changes ? I'm ok with it, will format another patch and place it in the beginning of this patch set. > >> /* >> * PSCI Function IDs for v0.2+ are well defined so use >> * standard values. >> @@ -264,29 +292,7 @@ static int psci_0_2_init(struct device_node *np) >> } >> } >> >> - pr_info("Using standard PSCI v0.2 function IDs\n"); >> - psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND; >> - psci_ops.cpu_suspend = psci_cpu_suspend; >> - >> - psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF; >> - psci_ops.cpu_off = psci_cpu_off; >> - >> - psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON; >> - psci_ops.cpu_on = psci_cpu_on; >> - >> - psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE; >> - psci_ops.migrate = psci_migrate; >> - >> - psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO; >> - psci_ops.affinity_info = psci_affinity_info; >> - >> - psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] = >> - PSCI_0_2_FN_MIGRATE_INFO_TYPE; >> - psci_ops.migrate_info_type = psci_migrate_info_type; >> - >> - arm_pm_restart = psci_sys_reset; >> - >> - pm_power_off = psci_sys_poweroff; >> + psci_0_2_set_functions(); >> >> out_put_node: >> of_node_put(np); >> @@ -333,6 +339,40 @@ out_put_node: >> return err; >> } >> >> +#ifdef CONFIG_ACPI >> +static int get_set_conduit_method_acpi(void) >> +{ >> + pr_info("probing for conduit method from ACPI.\n"); >> + >> + if (acpi_psci_use_hvc()) >> + invoke_psci_fn = __invoke_psci_fn_hvc; >> + else >> + invoke_psci_fn = __invoke_psci_fn_smc; >> + >> + return 0; >> +} >> + >> +/* We use PSCI 0.2+ when ACPI is deployed */ >> +static int psci_0_2_init_acpi(void) > > May be this can be renamed psci_acpi_init and called from setup directly > instead of calling from DT function with acpi_disabled check. Also suggested by Catalin, will do it :) > >> +{ >> + if (!acpi_psci_present()) { >> + pr_info("is not implemented in ACPI.\n"); >> + return -EOPNOTSUPP; >> + } >> + >> + get_set_conduit_method_acpi(); >> + >> + psci_0_2_set_functions(); >> + >> + return 0; >> +} >> +#else >> +static inline int psci_0_2_init_acpi(void) >> +{ >> + return -ENODEV; >> +} >> +#endif >> + >> static const struct of_device_id psci_of_match[] __initconst = { >> { .compatible = "arm,psci", .data = psci_0_1_init}, >> { .compatible = "arm,psci-0.2", .data = psci_0_2_init}, >> @@ -345,6 +385,9 @@ int __init psci_init(void) >> const struct of_device_id *matched_np; >> psci_initcall_t init_fn; >> >> + if (!acpi_disabled) >> + return psci_0_2_init_acpi(); >> + > > As mentioned above you need not modify this. > >> np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np); >> >> if (!np) >> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c >> index 85c6326..dfc4e4f3 100644 >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -395,6 +395,8 @@ void __init setup_arch(char **cmdline_p) >> efi_idmap_init(); >> >> cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; >> + acpi_boot_init(); >> + > > Check for the return error here and disable ACPI once rather than at > multiple places all over. ok. Thanks Hanjun
On 19/08/14 11:39, Hanjun Guo wrote: > On 2014-8-19 2:32, Sudeep Holla wrote: >> On 04/08/14 16:28, Hanjun Guo wrote: >>> 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 PSCI init. Since ACPI 5.1 doesn't support self defined PSCI >>> function IDs, which means that only PSCI 0.2+ is supported in ACPI. >>> >>> 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 and have no way to >>> bring up secondery CPUs, so disable ACPI if we get an FADT table >>> with version less that 5.1. >>> > [...] >>> >>> +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_revision < 1) { >> >> && > > Catalin also pointed out this bug :), but && is not enough, for example, > this would not trigger of revision 4.1, although 4.1 is not exist, but > the code will still confusing people, so I updated the code as: > Right I missed it :), the below code looks fine. > + if (table->revision > 5 || > + (table->revision == 5 && fadt->minor_revision >= 1)) { > + return 0; > + } else { > + pr_info("FADT revision is %d.%d, no PSCI support, should be 5.1 > or higher\n", > + table->revision, fadt->minor_revision); > + disable_acpi(); As suggested elsewhere try to eliminate this and have it once if acpi_boot_init failed for any reasons. Regards, Sudeep
> >> @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned long size) > >> early_memunmap(map, size); > >> } > >> > >> +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_revision < 1) { > > > > If we ever get revision 6.0, this would trigger. > > Yes, good catch, actually I already fixed that in my local git repo, > > + if (table->revision > 5 || > + (table->revision == 5 && fadt->minor_revision >= 1)) { > + return 0; > + } else { > + pr_info("FADT revision is %d.%d, no PSCI support, should be 5.1 > or higher\n", > + table->revision, fadt->minor_revision); > + disable_acpi(); > + return -EINVAL; > + } Given you return in the first path, you don't need the remaining code to live in an else block. Mark.
On 2014-8-19 19:10, Mark Rutland wrote: >>>> @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned long size) >>>> early_memunmap(map, size); >>>> } >>>> >>>> +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_revision < 1) { >>> >>> If we ever get revision 6.0, this would trigger. >> >> Yes, good catch, actually I already fixed that in my local git repo, >> >> + if (table->revision > 5 || >> + (table->revision == 5 && fadt->minor_revision >= 1)) { >> + return 0; >> + } else { >> + pr_info("FADT revision is %d.%d, no PSCI support, should be 5.1 >> or higher\n", >> + table->revision, fadt->minor_revision); >> + disable_acpi(); >> + return -EINVAL; >> + } > > Given you return in the first path, you don't need the remaining code to > live in an else block. Agreed, I will update it, and move disable_acpi() outside this function and keep it in one place as Sudeep suggested. Thanks Hanjun
I should warn you that FADT version numbers are notoriously unreliable; In fact, in ACPICA we were eventually forced to abandon them entirely. We use the actual size of the FADT instead. Bob > -----Original Message----- > From: Hanjun Guo [mailto:hanjun.guo@linaro.org] > Sent: Tuesday, August 19, 2014 5:14 AM > To: Mark Rutland > Cc: Catalin Marinas; Rafael J. Wysocki; graeme.gregory@linaro.org; Arnd > Bergmann; Olof Johansson; grant.likely@linaro.org; Sudeep Holla; Will > Deacon; Jason Cooper; Marc Zyngier; Bjorn Helgaas; Daniel Lezcano; Mark > Brown; Rob Herring; Robert Richter; Zheng, Lv; Moore, Robert; Lorenzo > Pieralisi; Liviu Dudau; Randy Dunlap; Charles Garcia-Tobin; linux- > acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; linaro-acpi@lists.linaro.org > Subject: Re: [PATCH v2 05/18] ARM64 / ACPI: Parse FADT table to get PSCI > flags for PSCI init > > On 2014-8-19 19:10, Mark Rutland wrote: > >>>> @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned > long size) > >>>> early_memunmap(map, size); > >>>> } > >>>> > >>>> +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_revision < 1) { > >>> > >>> If we ever get revision 6.0, this would trigger. > >> > >> Yes, good catch, actually I already fixed that in my local git repo, > >> > >> + if (table->revision > 5 || > >> + (table->revision == 5 && fadt->minor_revision >= 1)) { > >> + return 0; > >> + } else { > >> + pr_info("FADT revision is %d.%d, no PSCI support, > >> + should be 5.1 > >> or higher\n", > >> + table->revision, fadt->minor_revision); > >> + disable_acpi(); > >> + return -EINVAL; > >> + } > > > > Given you return in the first path, you don't need the remaining code > > to live in an else block. > > Agreed, I will update it, and move disable_acpi() outside this function > and keep it in one place as Sudeep suggested. > > Thanks > Hanjun
Hi Bob, On 2014-8-20 6:55, Moore, Robert wrote: > I should warn you that FADT version numbers are notoriously unreliable; > In fact, in ACPICA we were eventually forced to abandon them entirely. > We use the actual size of the FADT instead. Yes, I heard that story, thanks for the reminding. But I also see that the revision number is used on x86 and ia64 now in parsing FADT, it is a firmware bug not to comply with the spec, and on ARM, only ACPI 5.1 or higher can be used in Linux, Major.Minor revision was introduced in ACPI 5.1 and it was a major change for it, so I think firmware should comply with that, if firmware just copy some code from somewhere else and leave the revision number unchanged, we will not boot (disable ACPI). The size of FADT for 5.0 and 5.1 is no difference, it is pretty hard to use that to identify the version of FADT, did I miss something? Thanks Hanjun > > Bob > > > >> -----Original Message----- >> From: Hanjun Guo [mailto:hanjun.guo@linaro.org] >> Sent: Tuesday, August 19, 2014 5:14 AM >> To: Mark Rutland >> Cc: Catalin Marinas; Rafael J. Wysocki; graeme.gregory@linaro.org; Arnd >> Bergmann; Olof Johansson; grant.likely@linaro.org; Sudeep Holla; Will >> Deacon; Jason Cooper; Marc Zyngier; Bjorn Helgaas; Daniel Lezcano; Mark >> Brown; Rob Herring; Robert Richter; Zheng, Lv; Moore, Robert; Lorenzo >> Pieralisi; Liviu Dudau; Randy Dunlap; Charles Garcia-Tobin; linux- >> acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org; linaro-acpi@lists.linaro.org >> Subject: Re: [PATCH v2 05/18] ARM64 / ACPI: Parse FADT table to get PSCI >> flags for PSCI init >> >> On 2014-8-19 19:10, Mark Rutland wrote: >>>>>> @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned >> long size) >>>>>> early_memunmap(map, size); >>>>>> } >>>>>> >>>>>> +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_revision < 1) { >>>>> >>>>> If we ever get revision 6.0, this would trigger. >>>> >>>> Yes, good catch, actually I already fixed that in my local git repo, >>>> >>>> + if (table->revision > 5 || >>>> + (table->revision == 5 && fadt->minor_revision >= 1)) { >>>> + return 0; >>>> + } else { >>>> + pr_info("FADT revision is %d.%d, no PSCI support, >>>> + should be 5.1 >>>> or higher\n", >>>> + table->revision, fadt->minor_revision); >>>> + disable_acpi(); >>>> + return -EINVAL; >>>> + } >>> >>> Given you return in the first path, you don't need the remaining code >>> to live in an else block. >> >> Agreed, I will update it, and move disable_acpi() outside this function >> and keep it in one place as Sudeep suggested. >> >> Thanks >> Hanjun >
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 6400312..6e04868 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,18 @@ extern int acpi_disabled; extern int acpi_noirq; extern int acpi_pci_disabled; +/* 1 to indicate PSCI 0.2+ is implemented */ +static inline bool acpi_psci_present(void) +{ + return !!(acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT); +} + +/* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */ +static inline bool acpi_psci_use_hvc(void) +{ + return !!(acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC); +} + static inline void disable_acpi(void) { acpi_disabled = 1; diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 9cf9127..69a315d 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -11,6 +11,8 @@ * published by the Free Software Foundation. */ +#define pr_fmt(fmt) "ACPI: " fmt + #include <linux/init.h> #include <linux/acpi.h> #include <linux/cpumask.h> @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned long size) early_memunmap(map, size); } +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_revision < 1) { + pr_info("FADT version is %d.%d, no PSCI support, should be 5.1 or higher\n", + table->revision, fadt->minor_revision); + disable_acpi(); + return -EINVAL; + } + + return 0; +} + /* * acpi_boot_table_init() called from setup_arch(), always. * 1. find RSDP and get its address, and then find XSDT @@ -68,6 +90,21 @@ void __init acpi_boot_table_init(void) } } +int __init acpi_boot_init(void) +{ + int err = 0; + + /* If acpi_disabled, bail out */ + if (acpi_disabled) + return -ENODEV; + + err = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt); + if (err) + pr_err("Can't find FADT\n"); + + return err; +} + /* * acpi_suspend_lowlevel() - save kernel state and suspend. * diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index 9e9798f..2ad2084 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -15,6 +15,7 @@ #define pr_fmt(fmt) "psci: " fmt +#include <linux/acpi.h> #include <linux/init.h> #include <linux/of.h> #include <linux/smp.h> @@ -231,6 +232,33 @@ static void psci_sys_poweroff(void) invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); } +static void psci_0_2_set_functions(void) +{ + pr_info("Using standard PSCI v0.2 function IDs\n"); + psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND; + psci_ops.cpu_suspend = psci_cpu_suspend; + + psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF; + psci_ops.cpu_off = psci_cpu_off; + + psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON; + psci_ops.cpu_on = psci_cpu_on; + + psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE; + psci_ops.migrate = psci_migrate; + + psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO; + psci_ops.affinity_info = psci_affinity_info; + + psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] = + PSCI_0_2_FN_MIGRATE_INFO_TYPE; + psci_ops.migrate_info_type = psci_migrate_info_type; + + arm_pm_restart = psci_sys_reset; + + pm_power_off = psci_sys_poweroff; +} + /* * PSCI Function IDs for v0.2+ are well defined so use * standard values. @@ -264,29 +292,7 @@ static int psci_0_2_init(struct device_node *np) } } - pr_info("Using standard PSCI v0.2 function IDs\n"); - psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND; - psci_ops.cpu_suspend = psci_cpu_suspend; - - psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF; - psci_ops.cpu_off = psci_cpu_off; - - psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON; - psci_ops.cpu_on = psci_cpu_on; - - psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE; - psci_ops.migrate = psci_migrate; - - psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO; - psci_ops.affinity_info = psci_affinity_info; - - psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] = - PSCI_0_2_FN_MIGRATE_INFO_TYPE; - psci_ops.migrate_info_type = psci_migrate_info_type; - - arm_pm_restart = psci_sys_reset; - - pm_power_off = psci_sys_poweroff; + psci_0_2_set_functions(); out_put_node: of_node_put(np); @@ -333,6 +339,40 @@ out_put_node: return err; } +#ifdef CONFIG_ACPI +static int get_set_conduit_method_acpi(void) +{ + pr_info("probing for conduit method from ACPI.\n"); + + if (acpi_psci_use_hvc()) + invoke_psci_fn = __invoke_psci_fn_hvc; + else + invoke_psci_fn = __invoke_psci_fn_smc; + + return 0; +} + +/* We use PSCI 0.2+ when ACPI is deployed */ +static int psci_0_2_init_acpi(void) +{ + if (!acpi_psci_present()) { + pr_info("is not implemented in ACPI.\n"); + return -EOPNOTSUPP; + } + + get_set_conduit_method_acpi(); + + psci_0_2_set_functions(); + + return 0; +} +#else +static inline int psci_0_2_init_acpi(void) +{ + return -ENODEV; +} +#endif + static const struct of_device_id psci_of_match[] __initconst = { { .compatible = "arm,psci", .data = psci_0_1_init}, { .compatible = "arm,psci-0.2", .data = psci_0_2_init}, @@ -345,6 +385,9 @@ int __init psci_init(void) const struct of_device_id *matched_np; psci_initcall_t init_fn; + if (!acpi_disabled) + return psci_0_2_init_acpi(); + np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np); if (!np) diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 85c6326..dfc4e4f3 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -395,6 +395,8 @@ void __init setup_arch(char **cmdline_p) efi_idmap_init(); cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK; + acpi_boot_init(); + unflatten_device_tree(); psci_init();