This constitutes a major rewrite and restructuring of the FADT tests for ACPI. There is now a compliance test for every field that we can test. Some we cannot test since any possible value in the field is also an allowed value.
In some cases, the tests are an extension of existing tests -- for the PM blocks, SMI_CMD and the GPE blocks, for example. In the rest, these are new tests, and all of these are part of what used to be a much smaller test1. The original test2 and test3 are almost entirely intact since they did some solid testing of semantics across several related fields.
The largest structural change is around reduced hardware mode. There is now a test specific to that mode verifying that all of the fields in the FADT that are to be ignored are set to zero. Further, tests that are irrelevant to reduced hardware mode are skipped when we have tables in that mode.
Some of the patches are simply to enable compiling the FADT tests on non-x86 architectures; they were completely disabled previously. I did attempt to break up the patches into reasonable chunks but some of them still may have ended up fairly large.
This has been tested on arm64 and x86-64, using arm64 and x86 ACPI tables. I am sure the testing has not been exhaustive; there are a *lot* of fields in the FADT.
Al Stone (21): FADT: enable compiling on non-x86 architectures FADT: non-x86 machines need an FADT but x86 can survive without one FADT: disable SCI_EN and RESET_REG tests when in reduced hardware mode FADT: add in code to log basic info about the various FADT flag fields Add in bit masks for FACS flags. FADT: move log info out of test2, will provide it elsewhere ACPI: Add hypervisor ID field to FADT. FADT: minor cleanup and initial compliance tests FADT: expand the compliance test for FIRMWARE_CTRL fields FADT: expand compliance checks for DSDT and X_DSDT fields FADT: add compliance tests for reserved fields, PM profile, reduced hardware FADT: restructure test sequence around reduced hardware mode FADT: expand compliance tests for the SMI_CMD field FADT: add compliance tests for the ACPI_ENABLE and ACPI_DISABLE fields FADT: add compliance tests for S4BIOS_REQ and PSTATE_CNT fields FADT: extend and add PM address block compliance tests FADT: enhance compliance tests for GPE blocks FADT: add compliance test for the CST_CNT field FADT: add in compliance tests for C2/C3 latency fields FADT: add in SLEEP_CONTROL_REG and SLEEP_STATUS_REG compliance tests FADT: remove no longer useful variables from test1
src/acpi/fadt/fadt.c | 1718 ++++++++++++++++++++++++++++++++++++------- src/lib/include/fwts_acpi.h | 35 + 2 files changed, 1499 insertions(+), 254 deletions(-)
Instead of disabling the compilation of all FADT tests on non-x86 machines, enable as much as possible, even if the tests do not yet make sense. In subsequent patches, we will modify the tests to acknowledge the specifics of a given target arch. This does not change functionality on x86, but it does allow us to start testing portions of the FADT on other architectures.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 0f3739b..1a5560c 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -20,13 +20,18 @@ */ #include "fwts.h"
-#ifdef FWTS_ARCH_INTEL - #include <stdlib.h> #include <stdio.h> #include <sys/types.h> #include <sys/stat.h> +#ifdef FWTS_ARCH_INTEL #include <sys/io.h> +#else +static inline int ioperm(int a, ...) +{ + return a * 0; +} +#endif #include <unistd.h> #include <inttypes.h> #include <string.h> @@ -549,6 +554,7 @@ static fwts_framework_ops fadt_ops = { .minor_tests = fadt_tests };
-FWTS_REGISTER("fadt", &fadt_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_ROOT_PRIV | FWTS_FLAG_TEST_ACPI) +FWTS_REGISTER("fadt", &fadt_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | + FWTS_FLAG_ROOT_PRIV | FWTS_FLAG_TEST_ACPI | + FWTS_FLAG_TEST_COMPLIANCE_ACPI)
-#endif
On 09/02/16 01:32, Al Stone wrote:
Instead of disabling the compilation of all FADT tests on non-x86 machines, enable as much as possible, even if the tests do not yet make sense. In subsequent patches, we will modify the tests to acknowledge the specifics of a given target arch. This does not change functionality on x86, but it does allow us to start testing portions of the FADT on other architectures.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 0f3739b..1a5560c 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -20,13 +20,18 @@ */ #include "fwts.h" -#ifdef FWTS_ARCH_INTEL
#include <stdlib.h> #include <stdio.h> #include <sys/types.h> #include <sys/stat.h> +#ifdef FWTS_ARCH_INTEL #include <sys/io.h> +#else +static inline int ioperm(int a, ...) +{
- return a * 0;
+} +#endif #include <unistd.h> #include <inttypes.h> #include <string.h> @@ -549,6 +554,7 @@ static fwts_framework_ops fadt_ops = { .minor_tests = fadt_tests }; -FWTS_REGISTER("fadt", &fadt_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_ROOT_PRIV | FWTS_FLAG_TEST_ACPI) +FWTS_REGISTER("fadt", &fadt_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH |
FWTS_FLAG_ROOT_PRIV | FWTS_FLAG_TEST_ACPI |FWTS_FLAG_TEST_COMPLIANCE_ACPI)-#endif
Acked-by: Colin Ian King colin.king@canonucal.com
If there is no FADT, x86 machines can get by. Others cannot (or at least should not). On non-x86 machines, cause the initialization to fail instead of ignoring a zero-length FADT.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 1a5560c..aacf317 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -56,10 +56,17 @@ static int fadt_init(fwts_framework *fw) fadt = (const fwts_acpi_table_fadt*)table->data; fadt_size = table->length;
- /* Not having a FADT is not a failure */ + /* Not having a FADT is not a failure on x86 */ if (fadt_size == 0) { - fwts_log_info(fw, "FADT does not exist, this is not necessarily a failure, skipping tests."); - return FWTS_SKIP; + if (fw->target_arch == FWTS_ARCH_X86) { + fwts_log_info(fw, + "FADT does not exist, this is not " + "necessarily a failure, skipping tests."); + return FWTS_SKIP; + } else { + fwts_log_error(fw, "ACPI table FACP has zero length!"); + return FWTS_ERROR; + } }
return FWTS_OK;
Whatever values these fields have in them, they are to be ignored when ACPI is in reduced hardware mode. So, check for that mode and skip the tests if we are in reduced hardware.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index aacf317..cbe8f05 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -412,6 +412,11 @@ static int fadt_test2(fwts_framework *fw) fadt->preferred_pm_profile, FWTS_ACPI_FADT_PREFERRED_PM_PROFILE(fadt->preferred_pm_profile));
+ if (fwts_acpi_is_reduced_hardware(fadt)) { + fwts_skipped(fw, "In reduced hardware mode, skipping test."); + return FWTS_OK; + } + port = fadt->pm1a_cnt_blk; width = fadt->pm1_cnt_len * 8; /* In bits */
@@ -487,6 +492,11 @@ static int fadt_test2(fwts_framework *fw)
static int fadt_test3(fwts_framework *fw) { + if (fwts_acpi_is_reduced_hardware(fadt)) { + fwts_skipped(fw, "In reduced hardware mode, skipping test."); + return FWTS_OK; + } + if ((fadt->header.revision == 1) || (fadt->header.length < 244)) { fwts_skipped(fw, "Header size indicates an ACPI 1.0 FADT, skipping test."); return FWTS_SKIP;
Add in some log messages that report on the state of all of the flag fields in the FADT: flags, itself, plus the IA-PC boot architecture flags and the ARM boot architecture flags. Having this info in the log should make it more straightforward to understand test results later on in the FADT tests.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ src/lib/include/fwts_acpi.h | 30 +++++++++++++++++ 2 files changed, 111 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index cbe8f05..c4d3915 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -72,6 +72,86 @@ static int fadt_init(fwts_framework *fw) return FWTS_OK; }
+static void flag_info(fwts_framework *fw, const char *name, int state) +{ + const char *enabled = "set"; + const char *disabled = "not set"; + const char *ptr; + + ptr = (state) ? enabled : disabled; + fwts_log_info(fw, " %s is %s", name, ptr); +} + +static int fadt_info(fwts_framework *fw) +{ + fwts_log_info(fw, "FADT: flag states"); + flag_info(fw, "WBINVD", + fadt->flags & FWTS_FACP_FLAG_WBINVD); + flag_info(fw, "WBINVD_FLUSH", + fadt->flags & FWTS_FACP_FLAG_WBINVD_FLUSH); + flag_info(fw, "PROC_C1", + fadt->flags & FWTS_FACP_FLAG_PROC_C1); + flag_info(fw, "P_LVL2_UP", + fadt->flags & FWTS_FACP_FLAG_P_LVL2_UP); + flag_info(fw, "PWR_BUTTON", + fadt->flags & FWTS_FACP_FLAG_PWR_BUTTON); + flag_info(fw, "SLP_BUTTON", + fadt->flags & FWTS_FACP_FLAG_SLP_BUTTON); + flag_info(fw, "FIX_RTC", + fadt->flags & FWTS_FACP_FLAG_FIX_RTC); + flag_info(fw, "RTC_S4", + fadt->flags & FWTS_FACP_FLAG_RTC_S4); + flag_info(fw, "TMR_VAL_EXT", + fadt->flags & FWTS_FACP_FLAG_TMR_VAL_EXT); + flag_info(fw, "DCK_CAP", + fadt->flags & FWTS_FACP_FLAG_DCK_CAP); + flag_info(fw, "RESET_REG_SUP", + fadt->flags & FWTS_FACP_FLAG_RESET_REG_SUP); + flag_info(fw, "HEADLESS", + fadt->flags & FWTS_FACP_FLAG_HEADLESS); + flag_info(fw, "CPU_SW_SLP", + fadt->flags & FWTS_FACP_FLAG_CPU_SW_SLP); + flag_info(fw, "PCI_EXP_WAK", + fadt->flags & FWTS_FACP_FLAG_PCI_EXP_WAK); + flag_info(fw, "USE_PLATFORM_CLOCK", + fadt->flags & FWTS_FACP_FLAG_USE_PLATFORM_CLOCK); + flag_info(fw, "S4_RTC_STS_VALID", + fadt->flags & FWTS_FACP_FLAG_S4_RTC_STS_VALID); + flag_info(fw, "REMOTE_POWER_ON_CAPABLE", + fadt->flags & FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE); + flag_info(fw, "FORCE_APIC_CLUSTER_MODEL", + fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL); + flag_info(fw, "FORCE_APIC_PHYSICAL_DESTINATION_MODE", + fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE); + flag_info(fw, "HW_REDUCED_ACPI", + fadt->flags & FWTS_FACP_FLAG_HW_REDUCED_ACPI); + flag_info(fw, "LOW_POWER_S0_IDLE_CAPABLE", + fadt->flags & FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE); + + fwts_log_info(fw, "FADT: IA-PC Boot Architecture flag states"); + flag_info(fw, "LEGACY_DEVICES", fadt->iapc_boot_arch & + FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES); + flag_info(fw, "8042", fadt->iapc_boot_arch & + FWTS_FACP_IAPC_BOOT_ARCH_8042); + flag_info(fw, "VGA_NOT_PRESENT", fadt->iapc_boot_arch & + FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT); + flag_info(fw, "MSI_NOT_SUPPORTED", fadt->iapc_boot_arch & + FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED); + flag_info(fw, "PCIE_ASPM_CONTROLS", fadt->iapc_boot_arch & + FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS); + flag_info(fw, "CMOS_RTC_NOT_PRESENT", fadt->iapc_boot_arch & + FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT); + + fwts_log_info(fw, "FADT: ARM Boot Architecture flag states"); + flag_info(fw, "PSCI_COMPLIANT", fadt->arm_boot_flags & + FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT); + flag_info(fw, "PSCI_USE_HVC", fadt->arm_boot_flags & + FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC); + + fwts_infoonly(fw); + return FWTS_OK; +} + static void acpi_table_check_fadt_firmware_control( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -559,6 +639,7 @@ static int fadt_test3(fwts_framework *fw) }
static fwts_framework_minor_test fadt_tests[] = { + { fadt_info, "FADT ACPI Description Table flag info." }, { fadt_test1, "Test FADT ACPI Description Table tests." }, { fadt_test2, "Test FADT SCI_EN bit is enabled." }, { fadt_test3, "Test FADT reset register." }, diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index a8a8276..8f80286 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -32,11 +32,41 @@ #define FWTS_FACP_PERFORMANCE_SERVER (0x07) #define FWTS_FACP_TABLET (0x08)
+#define FWTS_FACP_FLAG_WBINVD (0x00000001) +#define FWTS_FACP_FLAG_WBINVD_FLUSH (0x00000002) +#define FWTS_FACP_FLAG_PROC_C1 (0x00000004) +#define FWTS_FACP_FLAG_P_LVL2_UP (0x00000008) +#define FWTS_FACP_FLAG_PWR_BUTTON (0x00000010) +#define FWTS_FACP_FLAG_SLP_BUTTON (0x00000020) +#define FWTS_FACP_FLAG_FIX_RTC (0x00000040) +#define FWTS_FACP_FLAG_RTC_S4 (0x00000080) +#define FWTS_FACP_FLAG_TMR_VAL_EXT (0x00000100) +#define FWTS_FACP_FLAG_DCK_CAP (0x00000200) +#define FWTS_FACP_FLAG_RESET_REG_SUP (0x00000400) +#define FWTS_FACP_FLAG_SEALED_CASE (0x00000800) +#define FWTS_FACP_FLAG_HEADLESS (0x00001000) +#define FWTS_FACP_FLAG_CPU_SW_SLP (0x00002000) +#define FWTS_FACP_FLAG_PCI_EXP_WAK (0x00004000) +#define FWTS_FACP_FLAG_USE_PLATFORM_CLOCK (0x00008000) +#define FWTS_FACP_FLAG_S4_RTC_STS_VALID (0x00010000) +#define FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE (0x00020000) +#define FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL (0x00040000) +#define FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE (0x00080000) +#define FWTS_FACP_FLAG_HW_REDUCED_ACPI (0x00100000) +#define FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE (0x00200000) +#define FWTS_FACP_FLAG_RESERVED_MASK (0xffc00000) + #define FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES (0x0001) #define FWTS_FACP_IAPC_BOOT_ARCH_8042 (0x0002) #define FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT (0x0004) #define FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED (0x0008) #define FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS (0x0010) +#define FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT (0x0020) +#define FWTS_FACP_IAPC_BOOT_ARCH_RESERVED_MASK (0xffc0) + +#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT (0x0001) +#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC (0x0002) +#define FWTS_FACP_ARM_BOOT_ARCH_RESERVED_MASK (0xfffc)
#define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY (0x00) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO (0x01)
On 09/02/16 01:32, Al Stone wrote:
Add in some log messages that report on the state of all of the flag fields in the FADT: flags, itself, plus the IA-PC boot architecture flags and the ARM boot architecture flags. Having this info in the log should make it more straightforward to understand test results later on in the FADT tests.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ src/lib/include/fwts_acpi.h | 30 +++++++++++++++++ 2 files changed, 111 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index cbe8f05..c4d3915 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -72,6 +72,86 @@ static int fadt_init(fwts_framework *fw) return FWTS_OK; } +static void flag_info(fwts_framework *fw, const char *name, int state) +{
- const char *enabled = "set";
- const char *disabled = "not set";
- const char *ptr;
- ptr = (state) ? enabled : disabled;
- fwts_log_info(fw, " %s is %s", name, ptr);
+}
+static int fadt_info(fwts_framework *fw) +{
- fwts_log_info(fw, "FADT: flag states");
- flag_info(fw, "WBINVD",
fadt->flags & FWTS_FACP_FLAG_WBINVD);- flag_info(fw, "WBINVD_FLUSH",
fadt->flags & FWTS_FACP_FLAG_WBINVD_FLUSH);- flag_info(fw, "PROC_C1",
fadt->flags & FWTS_FACP_FLAG_PROC_C1);- flag_info(fw, "P_LVL2_UP",
fadt->flags & FWTS_FACP_FLAG_P_LVL2_UP);- flag_info(fw, "PWR_BUTTON",
fadt->flags & FWTS_FACP_FLAG_PWR_BUTTON);- flag_info(fw, "SLP_BUTTON",
fadt->flags & FWTS_FACP_FLAG_SLP_BUTTON);- flag_info(fw, "FIX_RTC",
fadt->flags & FWTS_FACP_FLAG_FIX_RTC);- flag_info(fw, "RTC_S4",
fadt->flags & FWTS_FACP_FLAG_RTC_S4);- flag_info(fw, "TMR_VAL_EXT",
fadt->flags & FWTS_FACP_FLAG_TMR_VAL_EXT);- flag_info(fw, "DCK_CAP",
fadt->flags & FWTS_FACP_FLAG_DCK_CAP);- flag_info(fw, "RESET_REG_SUP",
fadt->flags & FWTS_FACP_FLAG_RESET_REG_SUP);
I think bit 11, SEALED_CASE is missing here.
- flag_info(fw, "HEADLESS",
fadt->flags & FWTS_FACP_FLAG_HEADLESS);- flag_info(fw, "CPU_SW_SLP",
fadt->flags & FWTS_FACP_FLAG_CPU_SW_SLP);- flag_info(fw, "PCI_EXP_WAK",
fadt->flags & FWTS_FACP_FLAG_PCI_EXP_WAK);- flag_info(fw, "USE_PLATFORM_CLOCK",
fadt->flags & FWTS_FACP_FLAG_USE_PLATFORM_CLOCK);- flag_info(fw, "S4_RTC_STS_VALID",
fadt->flags & FWTS_FACP_FLAG_S4_RTC_STS_VALID);- flag_info(fw, "REMOTE_POWER_ON_CAPABLE",
fadt->flags & FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE);- flag_info(fw, "FORCE_APIC_CLUSTER_MODEL",
fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL);- flag_info(fw, "FORCE_APIC_PHYSICAL_DESTINATION_MODE",
fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE);- flag_info(fw, "HW_REDUCED_ACPI",
fadt->flags & FWTS_FACP_FLAG_HW_REDUCED_ACPI);- flag_info(fw, "LOW_POWER_S0_IDLE_CAPABLE",
fadt->flags & FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE);- fwts_log_info(fw, "FADT: IA-PC Boot Architecture flag states");
- flag_info(fw, "LEGACY_DEVICES", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES);- flag_info(fw, "8042", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_8042);- flag_info(fw, "VGA_NOT_PRESENT", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT);- flag_info(fw, "MSI_NOT_SUPPORTED", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED);- flag_info(fw, "PCIE_ASPM_CONTROLS", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS);- flag_info(fw, "CMOS_RTC_NOT_PRESENT", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT);- fwts_log_info(fw, "FADT: ARM Boot Architecture flag states");
- flag_info(fw, "PSCI_COMPLIANT", fadt->arm_boot_flags &
FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT);- flag_info(fw, "PSCI_USE_HVC", fadt->arm_boot_flags &
FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC);- fwts_infoonly(fw);
- return FWTS_OK;
+}
static void acpi_table_check_fadt_firmware_control( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -559,6 +639,7 @@ static int fadt_test3(fwts_framework *fw) } static fwts_framework_minor_test fadt_tests[] = {
- { fadt_info, "FADT ACPI Description Table flag info." }, { fadt_test1, "Test FADT ACPI Description Table tests." }, { fadt_test2, "Test FADT SCI_EN bit is enabled." }, { fadt_test3, "Test FADT reset register." },
diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index a8a8276..8f80286 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -32,11 +32,41 @@ #define FWTS_FACP_PERFORMANCE_SERVER (0x07) #define FWTS_FACP_TABLET (0x08) +#define FWTS_FACP_FLAG_WBINVD (0x00000001) +#define FWTS_FACP_FLAG_WBINVD_FLUSH (0x00000002) +#define FWTS_FACP_FLAG_PROC_C1 (0x00000004) +#define FWTS_FACP_FLAG_P_LVL2_UP (0x00000008) +#define FWTS_FACP_FLAG_PWR_BUTTON (0x00000010) +#define FWTS_FACP_FLAG_SLP_BUTTON (0x00000020) +#define FWTS_FACP_FLAG_FIX_RTC (0x00000040) +#define FWTS_FACP_FLAG_RTC_S4 (0x00000080) +#define FWTS_FACP_FLAG_TMR_VAL_EXT (0x00000100) +#define FWTS_FACP_FLAG_DCK_CAP (0x00000200) +#define FWTS_FACP_FLAG_RESET_REG_SUP (0x00000400) +#define FWTS_FACP_FLAG_SEALED_CASE (0x00000800) +#define FWTS_FACP_FLAG_HEADLESS (0x00001000) +#define FWTS_FACP_FLAG_CPU_SW_SLP (0x00002000) +#define FWTS_FACP_FLAG_PCI_EXP_WAK (0x00004000) +#define FWTS_FACP_FLAG_USE_PLATFORM_CLOCK (0x00008000) +#define FWTS_FACP_FLAG_S4_RTC_STS_VALID (0x00010000) +#define FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE (0x00020000) +#define FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL (0x00040000) +#define FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE (0x00080000) +#define FWTS_FACP_FLAG_HW_REDUCED_ACPI (0x00100000) +#define FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE (0x00200000) +#define FWTS_FACP_FLAG_RESERVED_MASK (0xffc00000)
#define FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES (0x0001) #define FWTS_FACP_IAPC_BOOT_ARCH_8042 (0x0002) #define FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT (0x0004) #define FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED (0x0008) #define FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS (0x0010) +#define FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT (0x0020) +#define FWTS_FACP_IAPC_BOOT_ARCH_RESERVED_MASK (0xffc0)
+#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT (0x0001) +#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC (0x0002) +#define FWTS_FACP_ARM_BOOT_ARCH_RESERVED_MASK (0xfffc) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY (0x00) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO (0x01)
On 02/09/2016 04:51 AM, Colin Ian King wrote:
On 09/02/16 01:32, Al Stone wrote:
Add in some log messages that report on the state of all of the flag fields in the FADT: flags, itself, plus the IA-PC boot architecture flags and the ARM boot architecture flags. Having this info in the log should make it more straightforward to understand test results later on in the FADT tests.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ src/lib/include/fwts_acpi.h | 30 +++++++++++++++++ 2 files changed, 111 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index cbe8f05..c4d3915 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -72,6 +72,86 @@ static int fadt_init(fwts_framework *fw) return FWTS_OK; } +static void flag_info(fwts_framework *fw, const char *name, int state) +{
- const char *enabled = "set";
- const char *disabled = "not set";
- const char *ptr;
- ptr = (state) ? enabled : disabled;
- fwts_log_info(fw, " %s is %s", name, ptr);
+}
+static int fadt_info(fwts_framework *fw) +{
- fwts_log_info(fw, "FADT: flag states");
- flag_info(fw, "WBINVD",
fadt->flags & FWTS_FACP_FLAG_WBINVD);- flag_info(fw, "WBINVD_FLUSH",
fadt->flags & FWTS_FACP_FLAG_WBINVD_FLUSH);- flag_info(fw, "PROC_C1",
fadt->flags & FWTS_FACP_FLAG_PROC_C1);- flag_info(fw, "P_LVL2_UP",
fadt->flags & FWTS_FACP_FLAG_P_LVL2_UP);- flag_info(fw, "PWR_BUTTON",
fadt->flags & FWTS_FACP_FLAG_PWR_BUTTON);- flag_info(fw, "SLP_BUTTON",
fadt->flags & FWTS_FACP_FLAG_SLP_BUTTON);- flag_info(fw, "FIX_RTC",
fadt->flags & FWTS_FACP_FLAG_FIX_RTC);- flag_info(fw, "RTC_S4",
fadt->flags & FWTS_FACP_FLAG_RTC_S4);- flag_info(fw, "TMR_VAL_EXT",
fadt->flags & FWTS_FACP_FLAG_TMR_VAL_EXT);- flag_info(fw, "DCK_CAP",
fadt->flags & FWTS_FACP_FLAG_DCK_CAP);- flag_info(fw, "RESET_REG_SUP",
fadt->flags & FWTS_FACP_FLAG_RESET_REG_SUP);I think bit 11, SEALED_CASE is missing here.
Nice catch. Yup, should be these lines added:
flag_info(fw, "SEALED_CASE", fadt->flags & FWTS_FACP_FLAG_SEALED_CASE);
Shall I redo the patch or can you just add that in? Either way works for me.
- flag_info(fw, "HEADLESS",
fadt->flags & FWTS_FACP_FLAG_HEADLESS);- flag_info(fw, "CPU_SW_SLP",
fadt->flags & FWTS_FACP_FLAG_CPU_SW_SLP);- flag_info(fw, "PCI_EXP_WAK",
fadt->flags & FWTS_FACP_FLAG_PCI_EXP_WAK);- flag_info(fw, "USE_PLATFORM_CLOCK",
fadt->flags & FWTS_FACP_FLAG_USE_PLATFORM_CLOCK);- flag_info(fw, "S4_RTC_STS_VALID",
fadt->flags & FWTS_FACP_FLAG_S4_RTC_STS_VALID);- flag_info(fw, "REMOTE_POWER_ON_CAPABLE",
fadt->flags & FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE);- flag_info(fw, "FORCE_APIC_CLUSTER_MODEL",
fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL);- flag_info(fw, "FORCE_APIC_PHYSICAL_DESTINATION_MODE",
fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE);- flag_info(fw, "HW_REDUCED_ACPI",
fadt->flags & FWTS_FACP_FLAG_HW_REDUCED_ACPI);- flag_info(fw, "LOW_POWER_S0_IDLE_CAPABLE",
fadt->flags & FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE);- fwts_log_info(fw, "FADT: IA-PC Boot Architecture flag states");
- flag_info(fw, "LEGACY_DEVICES", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES);- flag_info(fw, "8042", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_8042);- flag_info(fw, "VGA_NOT_PRESENT", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT);- flag_info(fw, "MSI_NOT_SUPPORTED", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED);- flag_info(fw, "PCIE_ASPM_CONTROLS", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS);- flag_info(fw, "CMOS_RTC_NOT_PRESENT", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT);- fwts_log_info(fw, "FADT: ARM Boot Architecture flag states");
- flag_info(fw, "PSCI_COMPLIANT", fadt->arm_boot_flags &
FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT);- flag_info(fw, "PSCI_USE_HVC", fadt->arm_boot_flags &
FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC);- fwts_infoonly(fw);
- return FWTS_OK;
+}
static void acpi_table_check_fadt_firmware_control( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -559,6 +639,7 @@ static int fadt_test3(fwts_framework *fw) } static fwts_framework_minor_test fadt_tests[] = {
- { fadt_info, "FADT ACPI Description Table flag info." }, { fadt_test1, "Test FADT ACPI Description Table tests." }, { fadt_test2, "Test FADT SCI_EN bit is enabled." }, { fadt_test3, "Test FADT reset register." },
diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index a8a8276..8f80286 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -32,11 +32,41 @@ #define FWTS_FACP_PERFORMANCE_SERVER (0x07) #define FWTS_FACP_TABLET (0x08) +#define FWTS_FACP_FLAG_WBINVD (0x00000001) +#define FWTS_FACP_FLAG_WBINVD_FLUSH (0x00000002) +#define FWTS_FACP_FLAG_PROC_C1 (0x00000004) +#define FWTS_FACP_FLAG_P_LVL2_UP (0x00000008) +#define FWTS_FACP_FLAG_PWR_BUTTON (0x00000010) +#define FWTS_FACP_FLAG_SLP_BUTTON (0x00000020) +#define FWTS_FACP_FLAG_FIX_RTC (0x00000040) +#define FWTS_FACP_FLAG_RTC_S4 (0x00000080) +#define FWTS_FACP_FLAG_TMR_VAL_EXT (0x00000100) +#define FWTS_FACP_FLAG_DCK_CAP (0x00000200) +#define FWTS_FACP_FLAG_RESET_REG_SUP (0x00000400) +#define FWTS_FACP_FLAG_SEALED_CASE (0x00000800) +#define FWTS_FACP_FLAG_HEADLESS (0x00001000) +#define FWTS_FACP_FLAG_CPU_SW_SLP (0x00002000) +#define FWTS_FACP_FLAG_PCI_EXP_WAK (0x00004000) +#define FWTS_FACP_FLAG_USE_PLATFORM_CLOCK (0x00008000) +#define FWTS_FACP_FLAG_S4_RTC_STS_VALID (0x00010000) +#define FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE (0x00020000) +#define FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL (0x00040000) +#define FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE (0x00080000) +#define FWTS_FACP_FLAG_HW_REDUCED_ACPI (0x00100000) +#define FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE (0x00200000) +#define FWTS_FACP_FLAG_RESERVED_MASK (0xffc00000)
#define FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES (0x0001) #define FWTS_FACP_IAPC_BOOT_ARCH_8042 (0x0002) #define FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT (0x0004) #define FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED (0x0008) #define FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS (0x0010) +#define FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT (0x0020) +#define FWTS_FACP_IAPC_BOOT_ARCH_RESERVED_MASK (0xffc0)
+#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT (0x0001) +#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC (0x0002) +#define FWTS_FACP_ARM_BOOT_ARCH_RESERVED_MASK (0xfffc) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY (0x00) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO (0x01)
Hi Al,
Thanks for your patch, please redo and resend the patch in case we don't mess up something. :)
Cheers, Ivan
On 2016年02月10日 07:20, Al Stone wrote:
On 02/09/2016 04:51 AM, Colin Ian King wrote:
On 09/02/16 01:32, Al Stone wrote:
Add in some log messages that report on the state of all of the flag fields in the FADT: flags, itself, plus the IA-PC boot architecture flags and the ARM boot architecture flags. Having this info in the log should make it more straightforward to understand test results later on in the FADT tests.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ src/lib/include/fwts_acpi.h | 30 +++++++++++++++++ 2 files changed, 111 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index cbe8f05..c4d3915 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -72,6 +72,86 @@ static int fadt_init(fwts_framework *fw) return FWTS_OK; }
+static void flag_info(fwts_framework *fw, const char *name, int state) +{
- const char *enabled = "set";
- const char *disabled = "not set";
- const char *ptr;
- ptr = (state) ? enabled : disabled;
- fwts_log_info(fw, " %s is %s", name, ptr);
+}
+static int fadt_info(fwts_framework *fw) +{
- fwts_log_info(fw, "FADT: flag states");
- flag_info(fw, "WBINVD",
fadt->flags & FWTS_FACP_FLAG_WBINVD);- flag_info(fw, "WBINVD_FLUSH",
fadt->flags & FWTS_FACP_FLAG_WBINVD_FLUSH);- flag_info(fw, "PROC_C1",
fadt->flags & FWTS_FACP_FLAG_PROC_C1);- flag_info(fw, "P_LVL2_UP",
fadt->flags & FWTS_FACP_FLAG_P_LVL2_UP);- flag_info(fw, "PWR_BUTTON",
fadt->flags & FWTS_FACP_FLAG_PWR_BUTTON);- flag_info(fw, "SLP_BUTTON",
fadt->flags & FWTS_FACP_FLAG_SLP_BUTTON);- flag_info(fw, "FIX_RTC",
fadt->flags & FWTS_FACP_FLAG_FIX_RTC);- flag_info(fw, "RTC_S4",
fadt->flags & FWTS_FACP_FLAG_RTC_S4);- flag_info(fw, "TMR_VAL_EXT",
fadt->flags & FWTS_FACP_FLAG_TMR_VAL_EXT);- flag_info(fw, "DCK_CAP",
fadt->flags & FWTS_FACP_FLAG_DCK_CAP);- flag_info(fw, "RESET_REG_SUP",
fadt->flags & FWTS_FACP_FLAG_RESET_REG_SUP);I think bit 11, SEALED_CASE is missing here.
Nice catch. Yup, should be these lines added:
flag_info(fw, "SEALED_CASE", fadt->flags & FWTS_FACP_FLAG_SEALED_CASE);
Shall I redo the patch or can you just add that in? Either way works for me.
- flag_info(fw, "HEADLESS",
fadt->flags & FWTS_FACP_FLAG_HEADLESS);- flag_info(fw, "CPU_SW_SLP",
fadt->flags & FWTS_FACP_FLAG_CPU_SW_SLP);- flag_info(fw, "PCI_EXP_WAK",
fadt->flags & FWTS_FACP_FLAG_PCI_EXP_WAK);- flag_info(fw, "USE_PLATFORM_CLOCK",
fadt->flags & FWTS_FACP_FLAG_USE_PLATFORM_CLOCK);- flag_info(fw, "S4_RTC_STS_VALID",
fadt->flags & FWTS_FACP_FLAG_S4_RTC_STS_VALID);- flag_info(fw, "REMOTE_POWER_ON_CAPABLE",
fadt->flags & FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE);- flag_info(fw, "FORCE_APIC_CLUSTER_MODEL",
fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL);- flag_info(fw, "FORCE_APIC_PHYSICAL_DESTINATION_MODE",
fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE);- flag_info(fw, "HW_REDUCED_ACPI",
fadt->flags & FWTS_FACP_FLAG_HW_REDUCED_ACPI);- flag_info(fw, "LOW_POWER_S0_IDLE_CAPABLE",
fadt->flags & FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE);- fwts_log_info(fw, "FADT: IA-PC Boot Architecture flag states");
- flag_info(fw, "LEGACY_DEVICES", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES);- flag_info(fw, "8042", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_8042);- flag_info(fw, "VGA_NOT_PRESENT", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT);- flag_info(fw, "MSI_NOT_SUPPORTED", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED);- flag_info(fw, "PCIE_ASPM_CONTROLS", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS);- flag_info(fw, "CMOS_RTC_NOT_PRESENT", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT);- fwts_log_info(fw, "FADT: ARM Boot Architecture flag states");
- flag_info(fw, "PSCI_COMPLIANT", fadt->arm_boot_flags &
FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT);- flag_info(fw, "PSCI_USE_HVC", fadt->arm_boot_flags &
FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC);- fwts_infoonly(fw);
- return FWTS_OK;
+}
- static void acpi_table_check_fadt_firmware_control( fwts_framework *fw, const fwts_acpi_table_fadt *fadt,
@@ -559,6 +639,7 @@ static int fadt_test3(fwts_framework *fw) }
static fwts_framework_minor_test fadt_tests[] = {
- { fadt_info, "FADT ACPI Description Table flag info." }, { fadt_test1, "Test FADT ACPI Description Table tests." }, { fadt_test2, "Test FADT SCI_EN bit is enabled." }, { fadt_test3, "Test FADT reset register." },
diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index a8a8276..8f80286 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -32,11 +32,41 @@ #define FWTS_FACP_PERFORMANCE_SERVER (0x07) #define FWTS_FACP_TABLET (0x08)
+#define FWTS_FACP_FLAG_WBINVD (0x00000001) +#define FWTS_FACP_FLAG_WBINVD_FLUSH (0x00000002) +#define FWTS_FACP_FLAG_PROC_C1 (0x00000004) +#define FWTS_FACP_FLAG_P_LVL2_UP (0x00000008) +#define FWTS_FACP_FLAG_PWR_BUTTON (0x00000010) +#define FWTS_FACP_FLAG_SLP_BUTTON (0x00000020) +#define FWTS_FACP_FLAG_FIX_RTC (0x00000040) +#define FWTS_FACP_FLAG_RTC_S4 (0x00000080) +#define FWTS_FACP_FLAG_TMR_VAL_EXT (0x00000100) +#define FWTS_FACP_FLAG_DCK_CAP (0x00000200) +#define FWTS_FACP_FLAG_RESET_REG_SUP (0x00000400) +#define FWTS_FACP_FLAG_SEALED_CASE (0x00000800) +#define FWTS_FACP_FLAG_HEADLESS (0x00001000) +#define FWTS_FACP_FLAG_CPU_SW_SLP (0x00002000) +#define FWTS_FACP_FLAG_PCI_EXP_WAK (0x00004000) +#define FWTS_FACP_FLAG_USE_PLATFORM_CLOCK (0x00008000) +#define FWTS_FACP_FLAG_S4_RTC_STS_VALID (0x00010000) +#define FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE (0x00020000) +#define FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL (0x00040000) +#define FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE (0x00080000) +#define FWTS_FACP_FLAG_HW_REDUCED_ACPI (0x00100000) +#define FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE (0x00200000) +#define FWTS_FACP_FLAG_RESERVED_MASK (0xffc00000)
- #define FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES (0x0001) #define FWTS_FACP_IAPC_BOOT_ARCH_8042 (0x0002) #define FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT (0x0004) #define FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED (0x0008) #define FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS (0x0010)
+#define FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT (0x0020) +#define FWTS_FACP_IAPC_BOOT_ARCH_RESERVED_MASK (0xffc0)
+#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT (0x0001) +#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC (0x0002) +#define FWTS_FACP_ARM_BOOT_ARCH_RESERVED_MASK (0xfffc)
#define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY (0x00) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO (0x01)
On 02/15/2016 06:04 PM, ivanhu wrote:
Hi Al,
Thanks for your patch, please redo and resend the patch in case we don't mess up something. :)
Cheers, Ivan
On 2016年02月10日 07:20, Al Stone wrote:
On 02/09/2016 04:51 AM, Colin Ian King wrote:
On 09/02/16 01:32, Al Stone wrote:
Add in some log messages that report on the state of all of the flag fields in the FADT: flags, itself, plus the IA-PC boot architecture flags and the ARM boot architecture flags. Having this info in the log should make it more straightforward to understand test results later on in the FADT tests.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ src/lib/include/fwts_acpi.h | 30 +++++++++++++++++ 2 files changed, 111 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index cbe8f05..c4d3915 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -72,6 +72,86 @@ static int fadt_init(fwts_framework *fw) return FWTS_OK; }
+static void flag_info(fwts_framework *fw, const char *name, int state) +{
- const char *enabled = "set";
- const char *disabled = "not set";
- const char *ptr;
- ptr = (state) ? enabled : disabled;
- fwts_log_info(fw, " %s is %s", name, ptr);
+}
+static int fadt_info(fwts_framework *fw) +{
- fwts_log_info(fw, "FADT: flag states");
- flag_info(fw, "WBINVD",
fadt->flags & FWTS_FACP_FLAG_WBINVD);- flag_info(fw, "WBINVD_FLUSH",
fadt->flags & FWTS_FACP_FLAG_WBINVD_FLUSH);- flag_info(fw, "PROC_C1",
fadt->flags & FWTS_FACP_FLAG_PROC_C1);- flag_info(fw, "P_LVL2_UP",
fadt->flags & FWTS_FACP_FLAG_P_LVL2_UP);- flag_info(fw, "PWR_BUTTON",
fadt->flags & FWTS_FACP_FLAG_PWR_BUTTON);- flag_info(fw, "SLP_BUTTON",
fadt->flags & FWTS_FACP_FLAG_SLP_BUTTON);- flag_info(fw, "FIX_RTC",
fadt->flags & FWTS_FACP_FLAG_FIX_RTC);- flag_info(fw, "RTC_S4",
fadt->flags & FWTS_FACP_FLAG_RTC_S4);- flag_info(fw, "TMR_VAL_EXT",
fadt->flags & FWTS_FACP_FLAG_TMR_VAL_EXT);- flag_info(fw, "DCK_CAP",
fadt->flags & FWTS_FACP_FLAG_DCK_CAP);- flag_info(fw, "RESET_REG_SUP",
fadt->flags & FWTS_FACP_FLAG_RESET_REG_SUP);I think bit 11, SEALED_CASE is missing here.
Nice catch. Yup, should be these lines added:
flag_info(fw, "SEALED_CASE", fadt->flags & FWTS_FACP_FLAG_SEALED_CASE);Shall I redo the patch or can you just add that in? Either way works for me.
- flag_info(fw, "HEADLESS",
fadt->flags & FWTS_FACP_FLAG_HEADLESS);- flag_info(fw, "CPU_SW_SLP",
fadt->flags & FWTS_FACP_FLAG_CPU_SW_SLP);- flag_info(fw, "PCI_EXP_WAK",
fadt->flags & FWTS_FACP_FLAG_PCI_EXP_WAK);- flag_info(fw, "USE_PLATFORM_CLOCK",
fadt->flags & FWTS_FACP_FLAG_USE_PLATFORM_CLOCK);- flag_info(fw, "S4_RTC_STS_VALID",
fadt->flags & FWTS_FACP_FLAG_S4_RTC_STS_VALID);- flag_info(fw, "REMOTE_POWER_ON_CAPABLE",
fadt->flags & FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE);- flag_info(fw, "FORCE_APIC_CLUSTER_MODEL",
fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL);- flag_info(fw, "FORCE_APIC_PHYSICAL_DESTINATION_MODE",
fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE);- flag_info(fw, "HW_REDUCED_ACPI",
fadt->flags & FWTS_FACP_FLAG_HW_REDUCED_ACPI);- flag_info(fw, "LOW_POWER_S0_IDLE_CAPABLE",
fadt->flags & FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE);- fwts_log_info(fw, "FADT: IA-PC Boot Architecture flag states");
- flag_info(fw, "LEGACY_DEVICES", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES);- flag_info(fw, "8042", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_8042);- flag_info(fw, "VGA_NOT_PRESENT", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT);- flag_info(fw, "MSI_NOT_SUPPORTED", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED);- flag_info(fw, "PCIE_ASPM_CONTROLS", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS);- flag_info(fw, "CMOS_RTC_NOT_PRESENT", fadt->iapc_boot_arch &
FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT);- fwts_log_info(fw, "FADT: ARM Boot Architecture flag states");
- flag_info(fw, "PSCI_COMPLIANT", fadt->arm_boot_flags &
FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT);- flag_info(fw, "PSCI_USE_HVC", fadt->arm_boot_flags &
FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC);- fwts_infoonly(fw);
- return FWTS_OK;
+}
- static void acpi_table_check_fadt_firmware_control( fwts_framework *fw, const fwts_acpi_table_fadt *fadt,
@@ -559,6 +639,7 @@ static int fadt_test3(fwts_framework *fw) }
static fwts_framework_minor_test fadt_tests[] = {
- { fadt_info, "FADT ACPI Description Table flag info." }, { fadt_test1, "Test FADT ACPI Description Table tests." }, { fadt_test2, "Test FADT SCI_EN bit is enabled." }, { fadt_test3, "Test FADT reset register." },
diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index a8a8276..8f80286 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -32,11 +32,41 @@ #define FWTS_FACP_PERFORMANCE_SERVER (0x07) #define FWTS_FACP_TABLET (0x08)
+#define FWTS_FACP_FLAG_WBINVD (0x00000001) +#define FWTS_FACP_FLAG_WBINVD_FLUSH (0x00000002) +#define FWTS_FACP_FLAG_PROC_C1 (0x00000004) +#define FWTS_FACP_FLAG_P_LVL2_UP (0x00000008) +#define FWTS_FACP_FLAG_PWR_BUTTON (0x00000010) +#define FWTS_FACP_FLAG_SLP_BUTTON (0x00000020) +#define FWTS_FACP_FLAG_FIX_RTC (0x00000040) +#define FWTS_FACP_FLAG_RTC_S4 (0x00000080) +#define FWTS_FACP_FLAG_TMR_VAL_EXT (0x00000100) +#define FWTS_FACP_FLAG_DCK_CAP (0x00000200) +#define FWTS_FACP_FLAG_RESET_REG_SUP (0x00000400) +#define FWTS_FACP_FLAG_SEALED_CASE (0x00000800) +#define FWTS_FACP_FLAG_HEADLESS (0x00001000) +#define FWTS_FACP_FLAG_CPU_SW_SLP (0x00002000) +#define FWTS_FACP_FLAG_PCI_EXP_WAK (0x00004000) +#define FWTS_FACP_FLAG_USE_PLATFORM_CLOCK (0x00008000) +#define FWTS_FACP_FLAG_S4_RTC_STS_VALID (0x00010000) +#define FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE (0x00020000) +#define FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL (0x00040000) +#define FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE (0x00080000) +#define FWTS_FACP_FLAG_HW_REDUCED_ACPI (0x00100000) +#define FWTS_FACP_FLAG_LOW_POWER_S0_IDLE_CAPABLE (0x00200000) +#define FWTS_FACP_FLAG_RESERVED_MASK (0xffc00000)
- #define FWTS_FACP_IAPC_BOOT_ARCH_LEGACY_DEVICES (0x0001) #define FWTS_FACP_IAPC_BOOT_ARCH_8042 (0x0002) #define FWTS_FACP_IAPC_BOOT_ARCH_VGA_NOT_PRESENT (0x0004) #define FWTS_FACP_IAPC_BOOT_ARCH_MSI_NOT_SUPPORTED (0x0008) #define FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS (0x0010)
+#define FWTS_FACP_IAPC_BOOT_ARCH_CMOS_RTC_NOT_PRESENT (0x0020) +#define FWTS_FACP_IAPC_BOOT_ARCH_RESERVED_MASK (0xffc0)
+#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_COMPLIANT (0x0001) +#define FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC (0x0002) +#define FWTS_FACP_ARM_BOOT_ARCH_RESERVED_MASK (0xfffc)
#define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY (0x00) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO (0x01)
Thanks for all the ACKs. I'll redo the patches and send in v2.
These bit masks were simply missing but will be needed to verify some of the FADT information.
Signed-off-by: Al Stone al.stone@linaro.org --- src/lib/include/fwts_acpi.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index 8f80286..bab62ec 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -68,6 +68,10 @@ #define FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC (0x0002) #define FWTS_FACP_ARM_BOOT_ARCH_RESERVED_MASK (0xfffc)
+#define FWTS_FACS_FLAG_S4BIOS_F (0x00000001) +#define FWTS_FACS_FLAG_64BIT_WAKE_SUPPORTED (0x00000002) +#define FWTS_FACS_FLAG_RESERVED (0xfffffffc) + #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY (0x00) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO (0x01) #define FWTS_GAS_ADDR_SPACE_ID_PCI_CONFIG (0x02)
On 09/02/16 01:32, Al Stone wrote:
These bit masks were simply missing but will be needed to verify some of the FADT information.
Signed-off-by: Al Stone al.stone@linaro.org
src/lib/include/fwts_acpi.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index 8f80286..bab62ec 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -68,6 +68,10 @@ #define FWTS_FACP_ARM_BOOT_ARCH_PSCI_USE_HVC (0x0002) #define FWTS_FACP_ARM_BOOT_ARCH_RESERVED_MASK (0xfffc) +#define FWTS_FACS_FLAG_S4BIOS_F (0x00000001) +#define FWTS_FACS_FLAG_64BIT_WAKE_SUPPORTED (0x00000002) +#define FWTS_FACS_FLAG_RESERVED (0xfffffffc)
#define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY (0x00) #define FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO (0x01) #define FWTS_GAS_ADDR_SPACE_ID_PCI_CONFIG (0x02)
Acked-by: Colin Ian King colin.king@canonucal.com
In subsequent patches, we will test the PM profile field explicitly, so remove code to log the value from test2 and log it when we do the direct testing of the value.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index c4d3915..5106e4e 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -488,10 +488,6 @@ static int fadt_test2(fwts_framework *fw) uint32_t port, width, val32; int ret = FWTS_OK;
- fwts_log_info(fw, "FADT Preferred PM Profile: %hhu (%s)\n", - fadt->preferred_pm_profile, - FWTS_ACPI_FADT_PREFERRED_PM_PROFILE(fadt->preferred_pm_profile)); - if (fwts_acpi_is_reduced_hardware(fadt)) { fwts_skipped(fw, "In reduced hardware mode, skipping test."); return FWTS_OK;
On 09/02/16 01:32, Al Stone wrote:
In subsequent patches, we will test the PM profile field explicitly, so remove code to log the value from test2 and log it when we do the direct testing of the value.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index c4d3915..5106e4e 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -488,10 +488,6 @@ static int fadt_test2(fwts_framework *fw) uint32_t port, width, val32; int ret = FWTS_OK;
- fwts_log_info(fw, "FADT Preferred PM Profile: %hhu (%s)\n",
fadt->preferred_pm_profile,FWTS_ACPI_FADT_PREFERRED_PM_PROFILE(fadt->preferred_pm_profile));- if (fwts_acpi_is_reduced_hardware(fadt)) { fwts_skipped(fw, "In reduced hardware mode, skipping test."); return FWTS_OK;
Acked-by: Colin Ian King colin.king@canonucal.com
Add a field that is now defined in the spec but was not in the FADT definition.
Signed-off-by: Al Stone al.stone@linaro.org --- src/lib/include/fwts_acpi.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index bab62ec..b57022c 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -298,6 +298,7 @@ typedef struct { fwts_acpi_gas x_gpe1_blk; fwts_acpi_gas sleep_control_reg; fwts_acpi_gas sleep_status_reg; + uint64_t hypervisor_id; } __attribute__ ((packed)) fwts_acpi_table_fadt;
/*
On 09/02/16 01:32, Al Stone wrote:
Add a field that is now defined in the spec but was not in the FADT definition.
Signed-off-by: Al Stone al.stone@linaro.org
src/lib/include/fwts_acpi.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h index bab62ec..b57022c 100644 --- a/src/lib/include/fwts_acpi.h +++ b/src/lib/include/fwts_acpi.h @@ -298,6 +298,7 @@ typedef struct { fwts_acpi_gas x_gpe1_blk; fwts_acpi_gas sleep_control_reg; fwts_acpi_gas sleep_status_reg;
- uint64_t hypervisor_id;
} __attribute__ ((packed)) fwts_acpi_table_fadt; /*
Thanks for spotting this change.
Acked-by: Colin Ian King colin.king@canonucal.com
The primary purpose of this patch is to catch some very minor white space edits while adding in two very simple compliance tests -- is the table checksum correct, and is the revision number current?
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 5106e4e..6e5ee26 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -20,8 +20,8 @@ */ #include "fwts.h"
-#include <stdlib.h> #include <stdio.h> +#include <stdlib.h> #include <sys/types.h> #include <sys/stat.h> #ifdef FWTS_ARCH_INTEL @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw) fwts_log_error(fw, "ACPI table FACP does not exist!"); return FWTS_ERROR; } - fadt = (const fwts_acpi_table_fadt*)table->data; + fadt = (const fwts_acpi_table_fadt *)table->data; fadt_size = table->length;
/* Not having a FADT is not a failure on x86 */ @@ -152,6 +152,47 @@ static int fadt_info(fwts_framework *fw) return FWTS_OK; }
+static int fadt_checksum(fwts_framework *fw) +{ + const uint8_t *data = (const uint8_t *)fadt; + ssize_t length = fadt->header.length; + uint8_t checksum = 0; + + /* verify the table checksum */ + checksum = fwts_checksum(data, length); + if (checksum == 0) + fwts_passed(fw, "FADT checksum is correct"); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SPECMADTChecksum", + "FADT checksum is incorrect: 0x%x", checksum); + + return FWTS_OK; +} + +static int fadt_revision(fwts_framework *fw) +{ + uint8_t major; + uint8_t minor; + + major = fadt->header.revision; + minor = 0; + if (major >= 5 && fadt->header.length >= 268) + minor = fadt->minor_version; /* field added ACPI 5.1 */ + + fwts_log_info(fw, "FADT revision: %d.%d", major, minor); + fwts_log_info(fw, "FADT table length: %d", fadt->header.length); + + if (major == 6) + fwts_passed(fw, "FADT revision is up to date."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "SPECFADTRevision", + "FADT revision is outdated: %d.%d", major, minor); + + return FWTS_OK; +} + + static void acpi_table_check_fadt_firmware_control( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -635,8 +676,10 @@ static int fadt_test3(fwts_framework *fw) }
static fwts_framework_minor_test fadt_tests[] = { - { fadt_info, "FADT ACPI Description Table flag info." }, - { fadt_test1, "Test FADT ACPI Description Table tests." }, + { fadt_info, "ACPI FADT Description Table flag info." }, + { fadt_checksum, "FADT checksum test." }, + { fadt_revision, "FADT revision test." }, + { fadt_test1, "ACPI FADT Description Table tests." }, { fadt_test2, "Test FADT SCI_EN bit is enabled." }, { fadt_test3, "Test FADT reset register." }, { NULL, NULL }
On 09/02/16 01:32, Al Stone wrote:
The primary purpose of this patch is to catch some very minor white space edits while adding in two very simple compliance tests -- is the table checksum correct, and is the revision number current?
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 5106e4e..6e5ee26 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -20,8 +20,8 @@ */ #include "fwts.h" -#include <stdlib.h> #include <stdio.h> +#include <stdlib.h> #include <sys/types.h> #include <sys/stat.h> #ifdef FWTS_ARCH_INTEL @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw) fwts_log_error(fw, "ACPI table FACP does not exist!"); return FWTS_ERROR; }
- fadt = (const fwts_acpi_table_fadt*)table->data;
- fadt = (const fwts_acpi_table_fadt *)table->data; fadt_size = table->length;
/* Not having a FADT is not a failure on x86 */ @@ -152,6 +152,47 @@ static int fadt_info(fwts_framework *fw) return FWTS_OK; } +static int fadt_checksum(fwts_framework *fw) +{
- const uint8_t *data = (const uint8_t *)fadt;
- ssize_t length = fadt->header.length;
- uint8_t checksum = 0;
- /* verify the table checksum */
- checksum = fwts_checksum(data, length);
- if (checksum == 0)
fwts_passed(fw, "FADT checksum is correct");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM,"SPECMADTChecksum","FADT checksum is incorrect: 0x%x", checksum);- return FWTS_OK;
+}
+static int fadt_revision(fwts_framework *fw) +{
- uint8_t major;
- uint8_t minor;
- major = fadt->header.revision;
- minor = 0;
- if (major >= 5 && fadt->header.length >= 268)
minor = fadt->minor_version; /* field added ACPI 5.1 */- fwts_log_info(fw, "FADT revision: %d.%d", major, minor);
- fwts_log_info(fw, "FADT table length: %d", fadt->header.length);
- if (major == 6)
fwts_passed(fw, "FADT revision is up to date.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM, "SPECFADTRevision","FADT revision is outdated: %d.%d", major, minor);- return FWTS_OK;
+}
static void acpi_table_check_fadt_firmware_control( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -635,8 +676,10 @@ static int fadt_test3(fwts_framework *fw) } static fwts_framework_minor_test fadt_tests[] = {
- { fadt_info, "FADT ACPI Description Table flag info." },
- { fadt_test1, "Test FADT ACPI Description Table tests." },
- { fadt_info, "ACPI FADT Description Table flag info." },
- { fadt_checksum, "FADT checksum test." },
- { fadt_revision, "FADT revision test." },
- { fadt_test1, "ACPI FADT Description Table tests." }, { fadt_test2, "Test FADT SCI_EN bit is enabled." }, { fadt_test3, "Test FADT reset register." }, { NULL, NULL }
Acked-by: Colin Ian King colin.king@canonucal.com
Expand the testing of the FIRMWARE_CTRL -- and by extension, the X_FIRMWARE_CTRL field -- to check for full compliance with the spec. Only one or the other may be used at any one time, per 6.1, but we also have to acknowledge there are tables that do use both the 32- and 64-bit values. At that point, we re-use parts of the existing test to verify that they are at least consistent.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 146 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 95 insertions(+), 51 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 6e5ee26..84f4e09 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -192,61 +192,105 @@ static int fadt_revision(fwts_framework *fw) return FWTS_OK; }
- -static void acpi_table_check_fadt_firmware_control( - fwts_framework *fw, - const fwts_acpi_table_fadt *fadt, - bool *passed) +static void acpi_table_check_fadt_firmware_ctrl(fwts_framework *fw) { - if (fadt->firmware_control == 0) { - if (fadt->header.length >= 140) { - if ((fadt->x_firmware_ctrl == 0) && - !(fwts_acpi_is_reduced_hardware(fadt))) { - *passed = false; - fwts_failed(fw, LOG_LEVEL_CRITICAL, - "FADTFACSZero", - "FADT 32 bit FIRMWARE_CONTROL and " - "64 bit X_FIRMWARE_CONTROL (FACS " - "address) are null."); - fwts_advice(fw, - "The 32 bit FIRMWARE_CTRL or 64 " - "bit X_FIRMWARE_CTRL should point " - "to a valid Firmware ACPI Control " - "Structure (FACS) when ACPI hardware " - "reduced mode is not set. "); - } + /* tests for very old FADTs */ + if (fadt->header.length < 140 && fadt->firmware_control == 0) { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADT32BitFACSNull", + "FADT 32 bit FIRMWARE_CONTROL is null."); + fwts_advice(fw, + "The ACPI version 1.0 FADT has a NULL " + "FIRMWARE_CTRL and it needs to be defined " + "to point to a valid Firmware ACPI Control " + "Structure (FACS)."); + + /* can't do much else */ + return; + } + + /* for more recent FADTs, things get more complicated */ + if (fadt->firmware_control == 0 && fadt->x_firmware_ctrl == 0) { + if (fwts_acpi_is_reduced_hardware(fadt)) { + fwts_passed(fw, + "FADT 32 bit FIRMWARE_CONTROL and " + "64 bit X_FIRMWARE_CONTROL (FACS " + "address) are null in hardware reduced " + "mode."); + fwts_advice(fw, + "When in hardware reduced mode, it is " + "allowed to have both the 32-bit " + "FIRMWARE_CTRL and 64-bit X_FIRMWARE_CTRL " + "fields set to zero as they are. This " + "indicates that no FACS is available."); } else { - *passed = false; - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FADT32BitFACSNull", - "FADT 32 bit FIRMWARE_CONTROL is null."); + fwts_failed(fw, LOG_LEVEL_CRITICAL, + "FADTFACSZero", + "FADT 32 bit FIRMWARE_CONTROL and " + "64 bit X_FIRMWARE_CONTROL (FACS " + "address) are both null."); fwts_advice(fw, - "The ACPI version 1.0 FADT has a NULL " - "FIRMWARE_CTRL and it needs to be defined " - "to point to a valid Firmware ACPI Control " - "Structure (FACS)."); + "The 32 bit FIRMWARE_CTRL or 64 " + "bit X_FIRMWARE_CTRL should point " + "to a valid Firmware ACPI Control " + "Structure (FACS) when ACPI hardware " + "reduced mode is not set. "); } + + } + + if ((fadt->firmware_control != 0 && fadt->x_firmware_ctrl == 0) || + (fadt->firmware_control == 0 && fadt->x_firmware_ctrl != 0)) { + fwts_passed(fw, + "Only one of FIRMWARE_CTRL and X_FIRMWARE_CTRL " + "is non-zero."); } else { - if (fadt->header.length >= 140) { - if (fadt->x_firmware_ctrl != 0) { - if (((uint64_t)fadt->firmware_control != fadt->x_firmware_ctrl)) { - *passed = false; - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FwCtrl32and64Differ", - "FIRMWARE_CONTROL is 0x%" PRIx32 - " and differs from X_FIRMWARE_CONTROL " - "0x%" PRIx64, - fadt->firmware_control, - fadt->x_firmware_ctrl); - fwts_advice(fw, - "One would expect the 32 bit FIRMWARE_CTRL " - "and 64 bit X_FIRMWARE_CTRL pointers to " - "point to the same FACS, however they don't " - "which is clearly ambiguous and wrong. " - "The kernel works around this by using the " - "64 bit X_FIRMWARE_CTRL pointer to the FACS. "); - } - } + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTFACSBothSet", + "Both 32-bit FIRMWARE_CTRL and 64-bit " + "X_FIRMWARE_CTRL pointers to the FACS are " + "set but only one is allowed."); + fwts_advice(fw, + "Having both FACS pointers set is ambiguous; " + "there is no way to determine which one is " + "the correct address. The kernel workaround " + "is to always use the 64-bit X_FIRMWARE_CTRL."); + } + + + if (fadt->firmware_control != 0 && fadt->x_firmware_ctrl != 0) { + if ((uint64_t)fadt->firmware_control == fadt->x_firmware_ctrl) { + fwts_passed(fw, + "Both FIRMWARE_CTRL and X_FIRMWARE_CTRL " + "are being used and contain the same FACS " + "address."); + fwts_advice(fw, + "While having FIRMWARE_CTRL and " + "X_FIRMWARE_CTRL both set to an address " + "is not compliant with the ACPI " + "specification, they are both set to " + "the same address, which at least " + "mitigates the ambiguity in determining " + "which address is the correct one to use " + "for the FACS. Ideally, only one of the " + "two addresses should be set but as a " + "practical matter, this will work."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FwCtrl32and64Differ", + "FIRMWARE_CONTROL is 0x%" PRIx32 + " and differs from X_FIRMWARE_CONTROL " + "0x%" PRIx64, + fadt->firmware_control, + fadt->x_firmware_ctrl); + fwts_advice(fw, + "One would expect the 32 bit FIRMWARE_CTRL " + "and 64 bit X_FIRMWARE_CTRL pointers to " + "point to the same FACS, however they do " + "not which is clearly ambiguous and wrong. " + "The kernel works around this by using the " + "64-bit X_FIRMWARE_CTRL pointer to the " + "FACS. "); } } } @@ -486,7 +530,7 @@ static int fadt_test1(fwts_framework *fw) { bool passed = true;
- acpi_table_check_fadt_firmware_control(fw, fadt, &passed); + acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw, fadt, &passed); acpi_table_check_fadt_smi(fw, fadt, &passed); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed);
On 09/02/16 01:32, Al Stone wrote:
Expand the testing of the FIRMWARE_CTRL -- and by extension, the X_FIRMWARE_CTRL field -- to check for full compliance with the spec. Only one or the other may be used at any one time, per 6.1, but we also have to acknowledge there are tables that do use both the 32- and 64-bit values. At that point, we re-use parts of the existing test to verify that they are at least consistent.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 146 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 95 insertions(+), 51 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 6e5ee26..84f4e09 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -192,61 +192,105 @@ static int fadt_revision(fwts_framework *fw) return FWTS_OK; }
-static void acpi_table_check_fadt_firmware_control(
- fwts_framework *fw,
- const fwts_acpi_table_fadt *fadt,
- bool *passed)
+static void acpi_table_check_fadt_firmware_ctrl(fwts_framework *fw) {
- if (fadt->firmware_control == 0) {
if (fadt->header.length >= 140) {if ((fadt->x_firmware_ctrl == 0) &&!(fwts_acpi_is_reduced_hardware(fadt))) {*passed = false;fwts_failed(fw, LOG_LEVEL_CRITICAL,"FADTFACSZero","FADT 32 bit FIRMWARE_CONTROL and ""64 bit X_FIRMWARE_CONTROL (FACS ""address) are null.");fwts_advice(fw,"The 32 bit FIRMWARE_CTRL or 64 ""bit X_FIRMWARE_CTRL should point ""to a valid Firmware ACPI Control ""Structure (FACS) when ACPI hardware ""reduced mode is not set. ");}
- /* tests for very old FADTs */
- if (fadt->header.length < 140 && fadt->firmware_control == 0) {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADT32BitFACSNull","FADT 32 bit FIRMWARE_CONTROL is null.");fwts_advice(fw,"The ACPI version 1.0 FADT has a NULL ""FIRMWARE_CTRL and it needs to be defined ""to point to a valid Firmware ACPI Control ""Structure (FACS).");/* can't do much else */return;- }
- /* for more recent FADTs, things get more complicated */
- if (fadt->firmware_control == 0 && fadt->x_firmware_ctrl == 0) {
if (fwts_acpi_is_reduced_hardware(fadt)) {fwts_passed(fw,"FADT 32 bit FIRMWARE_CONTROL and ""64 bit X_FIRMWARE_CONTROL (FACS ""address) are null in hardware reduced ""mode.");fwts_advice(fw,"When in hardware reduced mode, it is ""allowed to have both the 32-bit ""FIRMWARE_CTRL and 64-bit X_FIRMWARE_CTRL ""fields set to zero as they are. This ""indicates that no FACS is available.");
*passed = false;fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADT32BitFACSNull","FADT 32 bit FIRMWARE_CONTROL is null.");
fwts_failed(fw, LOG_LEVEL_CRITICAL,"FADTFACSZero","FADT 32 bit FIRMWARE_CONTROL and ""64 bit X_FIRMWARE_CONTROL (FACS ""address) are both null."); fwts_advice(fw,
"The ACPI version 1.0 FADT has a NULL ""FIRMWARE_CTRL and it needs to be defined ""to point to a valid Firmware ACPI Control ""Structure (FACS).");
"The 32 bit FIRMWARE_CTRL or 64 ""bit X_FIRMWARE_CTRL should point ""to a valid Firmware ACPI Control ""Structure (FACS) when ACPI hardware ""reduced mode is not set. ");- }
- if ((fadt->firmware_control != 0 && fadt->x_firmware_ctrl == 0) ||
(fadt->firmware_control == 0 && fadt->x_firmware_ctrl != 0)) {fwts_passed(fw,"Only one of FIRMWARE_CTRL and X_FIRMWARE_CTRL ""is non-zero.");
if (fadt->header.length >= 140) {if (fadt->x_firmware_ctrl != 0) {if (((uint64_t)fadt->firmware_control != fadt->x_firmware_ctrl)) {*passed = false;fwts_failed(fw, LOG_LEVEL_MEDIUM,"FwCtrl32and64Differ","FIRMWARE_CONTROL is 0x%" PRIx32" and differs from X_FIRMWARE_CONTROL ""0x%" PRIx64,fadt->firmware_control,fadt->x_firmware_ctrl);fwts_advice(fw,"One would expect the 32 bit FIRMWARE_CTRL ""and 64 bit X_FIRMWARE_CTRL pointers to ""point to the same FACS, however they don't ""which is clearly ambiguous and wrong. ""The kernel works around this by using the ""64 bit X_FIRMWARE_CTRL pointer to the FACS. ");}}
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTFACSBothSet","Both 32-bit FIRMWARE_CTRL and 64-bit ""X_FIRMWARE_CTRL pointers to the FACS are ""set but only one is allowed.");fwts_advice(fw,"Having both FACS pointers set is ambiguous; ""there is no way to determine which one is ""the correct address. The kernel workaround ""is to always use the 64-bit X_FIRMWARE_CTRL.");- }
- if (fadt->firmware_control != 0 && fadt->x_firmware_ctrl != 0) {
if ((uint64_t)fadt->firmware_control == fadt->x_firmware_ctrl) {fwts_passed(fw,"Both FIRMWARE_CTRL and X_FIRMWARE_CTRL ""are being used and contain the same FACS ""address.");fwts_advice(fw,"While having FIRMWARE_CTRL and ""X_FIRMWARE_CTRL both set to an address ""is not compliant with the ACPI ""specification, they are both set to ""the same address, which at least ""mitigates the ambiguity in determining ""which address is the correct one to use ""for the FACS. Ideally, only one of the ""two addresses should be set but as a ""practical matter, this will work.");} else {fwts_failed(fw, LOG_LEVEL_MEDIUM,"FwCtrl32and64Differ","FIRMWARE_CONTROL is 0x%" PRIx32" and differs from X_FIRMWARE_CONTROL ""0x%" PRIx64,fadt->firmware_control,fadt->x_firmware_ctrl);fwts_advice(fw,"One would expect the 32 bit FIRMWARE_CTRL ""and 64 bit X_FIRMWARE_CTRL pointers to ""point to the same FACS, however they do ""not which is clearly ambiguous and wrong. ""The kernel works around this by using the ""64-bit X_FIRMWARE_CTRL pointer to the ""FACS. ");} @@ -486,7 +530,7 @@ static int fadt_test1(fwts_framework *fw) { bool passed = true;
- acpi_table_check_fadt_firmware_control(fw, fadt, &passed);
- acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw, fadt, &passed); acpi_table_check_fadt_smi(fw, fadt, &passed); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed);
Yep, this is a good improvement.
Acked-by: Colin Ian King colin.king@canonucal.com
Expand the testing of the DSDT address -- and by extension, the X_DSDT address field -- to check for full compliance with the spec. Only one or the other may be used at any one time, per 6.1, but we also have to acknowledge there are tables that do use both the 32- and 64-bit values. At that point, we re-use parts of the existing test to verify that they are at least consistent.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 99 +++++++++++++++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 43 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 84f4e09..b21653d 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -295,54 +295,67 @@ static void acpi_table_check_fadt_firmware_ctrl(fwts_framework *fw) } }
-static void acpi_table_check_fadt_dsdt( - fwts_framework *fw, - const fwts_acpi_table_fadt *fadt, - bool *passed) +static void acpi_table_check_fadt_dsdt(fwts_framework *fw) { - + /* check out older FADTs */ if (fadt->header.length < 148) { - if (fadt->dsdt == 0) { - *passed = false; + if (fadt->dsdt == 0) fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTDSDTNull", "FADT DSDT address is null."); - } - } else { - if (fadt->x_dsdt == 0) { - if (fadt->dsdt == 0) { - *passed = false; - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FADTXDSDTNull", - "FADT X_DSDT and DSDT address are null."); - } else { - *passed = false; - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FADTXDSDTNull", - "FADT X_DSDT address is null."); - fwts_advice(fw, - "An ACPI 2.0 FADT is being used however " - "the 64 bit X_DSDT is null." - "The kernel will fall back to using " - "the 32 bit DSDT pointer instead."); - } - } else if ((uint64_t)fadt->dsdt != fadt->x_dsdt && fadt->dsdt != 0) { - *passed = false; + } + + /* if one field is being used, the other must be null */ + if ((fadt->dsdt != 0 && fadt->x_dsdt == 0) || + (fadt->dsdt == 0 && fadt->x_dsdt != 0)) + fwts_passed(fw, + "FADT has only one of X_DSDT or DSDT addresses " + "being used."); + else { + if (fadt->dsdt == 0 && fadt->x_dsdt == 0) fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FADT32And64Mismatch", - "FADT 32 bit DSDT (0x%" PRIx32 ") " - "does not point to same " - "physical address as 64 bit X_DSDT " - "(0x%" PRIx64 ").", - fadt->dsdt, fadt->x_dsdt); - fwts_advice(fw, - "One would expect the 32 bit DSDT and " - "64 bit X_DSDT pointers to point to the " - "same DSDT, however they don't which is " - "clearly ambiguous and wrong. " - "The kernel works around this by using the " - "64 bit X_DSDT pointer to the DSDT. "); - } + "FADTOneDSDTNull", + "FADT X_DSDT and DSDT addresses cannot " + "both be null."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTBothDSDTSet", + "FADT X_DSDT and DSDT addresses cannot " + "both be set to a value."); + } + + /* unexpected use of addresses */ + if (fadt->dsdt != 0 && fadt->x_dsdt == 0) { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTXDSDTNull", + "FADT X_DSDT address is null."); + fwts_advice(fw, + "An ACPI 2.0 or newer FADT is being used however " + "the 64 bit X_DSDT is null." + "The kernel will fall back to using " + "the 32 bit DSDT pointer instead."); + } + + /* + * If you are going to insist on using both fields, even though + * that is incorrect, at least make it unambiguous as to which + * address is the one to use. + */ + if ((uint64_t)fadt->dsdt != fadt->x_dsdt && fadt->dsdt != 0) { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADT32And64Mismatch", + "FADT 32 bit DSDT (0x%" PRIx32 ") " + "does not point to same " + "physical address as 64 bit X_DSDT " + "(0x%" PRIx64 ").", + fadt->dsdt, fadt->x_dsdt); + fwts_advice(fw, + "One would expect the 32 bit DSDT and " + "64 bit X_DSDT pointers to point to the " + "same DSDT, however they don't which is " + "clearly ambiguous and wrong. " + "The kernel works around this by using the " + "64 bit X_DSDT pointer to the DSDT. "); } }
@@ -531,7 +544,7 @@ static int fadt_test1(fwts_framework *fw) bool passed = true;
acpi_table_check_fadt_firmware_ctrl(fw); - acpi_table_check_fadt_dsdt(fw, fadt, &passed); + acpi_table_check_fadt_dsdt(fw); acpi_table_check_fadt_smi(fw, fadt, &passed); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); acpi_table_check_fadt_gpe(fw, fadt, &passed);
On 09/02/16 01:32, Al Stone wrote:
Expand the testing of the DSDT address -- and by extension, the X_DSDT address field -- to check for full compliance with the spec. Only one or the other may be used at any one time, per 6.1, but we also have to acknowledge there are tables that do use both the 32- and 64-bit values. At that point, we re-use parts of the existing test to verify that they are at least consistent.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 99 +++++++++++++++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 43 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 84f4e09..b21653d 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -295,54 +295,67 @@ static void acpi_table_check_fadt_firmware_ctrl(fwts_framework *fw) } } -static void acpi_table_check_fadt_dsdt(
- fwts_framework *fw,
- const fwts_acpi_table_fadt *fadt,
- bool *passed)
+static void acpi_table_check_fadt_dsdt(fwts_framework *fw) {
- /* check out older FADTs */ if (fadt->header.length < 148) {
if (fadt->dsdt == 0) {*passed = false;
if (fadt->dsdt == 0) fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTDSDTNull", "FADT DSDT address is null.");
}- } else {
if (fadt->x_dsdt == 0) {if (fadt->dsdt == 0) {*passed = false;fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTXDSDTNull","FADT X_DSDT and DSDT address are null.");} else {*passed = false;fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTXDSDTNull","FADT X_DSDT address is null.");fwts_advice(fw,"An ACPI 2.0 FADT is being used however ""the 64 bit X_DSDT is null.""The kernel will fall back to using ""the 32 bit DSDT pointer instead.");}} else if ((uint64_t)fadt->dsdt != fadt->x_dsdt && fadt->dsdt != 0) {*passed = false;
- }
- /* if one field is being used, the other must be null */
- if ((fadt->dsdt != 0 && fadt->x_dsdt == 0) ||
(fadt->dsdt == 0 && fadt->x_dsdt != 0))fwts_passed(fw,"FADT has only one of X_DSDT or DSDT addresses ""being used.");- else {
if (fadt->dsdt == 0 && fadt->x_dsdt == 0) fwts_failed(fw, LOG_LEVEL_MEDIUM,
"FADT32And64Mismatch","FADT 32 bit DSDT (0x%" PRIx32 ") ""does not point to same ""physical address as 64 bit X_DSDT ""(0x%" PRIx64 ").",fadt->dsdt, fadt->x_dsdt);fwts_advice(fw,"One would expect the 32 bit DSDT and ""64 bit X_DSDT pointers to point to the ""same DSDT, however they don't which is ""clearly ambiguous and wrong. ""The kernel works around this by using the ""64 bit X_DSDT pointer to the DSDT. ");}
"FADTOneDSDTNull","FADT X_DSDT and DSDT addresses cannot ""both be null.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTBothDSDTSet","FADT X_DSDT and DSDT addresses cannot ""both be set to a value.");- }
- /* unexpected use of addresses */
- if (fadt->dsdt != 0 && fadt->x_dsdt == 0) {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTXDSDTNull","FADT X_DSDT address is null.");fwts_advice(fw,"An ACPI 2.0 or newer FADT is being used however ""the 64 bit X_DSDT is null.""The kernel will fall back to using ""the 32 bit DSDT pointer instead.");- }
- /*
* If you are going to insist on using both fields, even though* that is incorrect, at least make it unambiguous as to which* address is the one to use.*/- if ((uint64_t)fadt->dsdt != fadt->x_dsdt && fadt->dsdt != 0) {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADT32And64Mismatch","FADT 32 bit DSDT (0x%" PRIx32 ") ""does not point to same ""physical address as 64 bit X_DSDT ""(0x%" PRIx64 ").",fadt->dsdt, fadt->x_dsdt);fwts_advice(fw,"One would expect the 32 bit DSDT and ""64 bit X_DSDT pointers to point to the ""same DSDT, however they don't which is ""clearly ambiguous and wrong. ""The kernel works around this by using the ""64 bit X_DSDT pointer to the DSDT. ");} @@ -531,7 +544,7 @@ static int fadt_test1(fwts_framework *fw) bool passed = true; acpi_table_check_fadt_firmware_ctrl(fw);
- acpi_table_check_fadt_dsdt(fw, fadt, &passed);
- acpi_table_check_fadt_dsdt(fw); acpi_table_check_fadt_smi(fw, fadt, &passed); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); acpi_table_check_fadt_gpe(fw, fadt, &passed);
Acked-by: Colin Ian King colin.king@canonucal.com
Add three new compliance tests that verify that the FADT reserved fields are zero as they should be, that the PM (Power Management) profile is a legal value, and that all of the fields and flags that are to be ignored when the OSPM is in reduced hardware mode are zero.
Note that the spec says to ignore many FADT fields when in reduced hardware mode. As of 6.1, it does not explicitly state that they must be zero. But, zero seems to be a reasonable value -- or at least not destructive -- on the OSPMs that use them when they should be ignored. The case can also be made that the fields should be zero; section 5.2.1.4 indicates ignored fields should be treated the same as reserved fields, which _are_ required to be zero. As a practical matter, some vendors may have chosen to sidestep the spec here and set reduced hardware mode but used the ignored field anyway.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 303 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 303 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index b21653d..fd430ec 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -359,6 +359,306 @@ static void acpi_table_check_fadt_dsdt(fwts_framework *fw) } }
+static void acpi_table_check_fadt_reserved(fwts_framework *fw) +{ + if (fadt->reserved == (uint8_t)0) + fwts_passed(fw, "FADT first reserved field is zero."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTReservedZero", + "FADT first reserved field is not zero: 0x%02x", + fadt->reserved); + + if (fadt->reserved1 == (uint8_t)0) + fwts_passed(fw, "FADT second reserved field is zero."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTReservedZero", + "FADT second reserved field is not zero: 0x%02x", + fadt->reserved1); +} + +static void acpi_table_check_fadt_pm_profile(fwts_framework *fw) +{ + fwts_log_info(fw, "FADT Preferred PM Profile: %hhu (%s)\n", + fadt->preferred_pm_profile, + FWTS_ACPI_FADT_PREFERRED_PM_PROFILE(fadt->preferred_pm_profile)); + + if (fadt->preferred_pm_profile <= 8) + fwts_passed(fw, "FADT has a valid preferred PM profile."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTPMProfile", + "FADT preferred PM profile is invalid."); +} + +static void acpi_table_check_fadt_reduced_hardware(fwts_framework *fw) +{ + const char *IS = "IS"; + const char *IS_NOT = "IS NOT"; + bool rhw; + bool passed; + const fwts_acpi_gas null_gas = { 0 }; + uint32_t flag_mask; + + rhw = fwts_acpi_is_reduced_hardware(fadt); + fwts_log_info(fw, "FADT indicates ACPI %s in reduced hardware mode.", + rhw ? IS : IS_NOT); + + if (!rhw) + return; + + passed = true; + + /* check all the fields that will be ignored */ + if (fadt->smi_cmd != 0) { + passed = false; + fwts_log_info(fw, "SMI_CMD is non-zero: 0x%x", + fadt->smi_cmd); + } + if (fadt->acpi_enable != 0) { + passed = false; + fwts_log_info(fw, "ACPI_ENABLE is non-zero: 0x%x", + fadt->acpi_enable); + } + if (fadt->acpi_disable != 0) { + passed = false; + fwts_log_info(fw, "ACPI_DISABLE is non-zero: 0x%x", + fadt->acpi_disable); + } + if (fadt->s4bios_req != 0) { + passed = false; + fwts_log_info(fw, "S4BIOS_REQ is non-zero: 0x%x", + fadt->s4bios_req); + } + if (fadt->pstate_cnt != 0) { + passed = false; + fwts_log_info(fw, "PSTATE_CNT is non-zero: 0x%x", + fadt->pstate_cnt); + } + if (fadt->pm1a_evt_blk != 0) { + passed = false; + fwts_log_info(fw, "PM1A_EVT_BLK is non-zero: 0x%x", + fadt->pm1a_evt_blk); + } + if (fadt->pm1b_evt_blk != 0) { + passed = false; + fwts_log_info(fw, "PM1B_EVT_BLK is non-zero: 0x%x", + fadt->pm1b_evt_blk); + } + if (fadt->pm1a_cnt_blk != 0) { + passed = false; + fwts_log_info(fw, "PM1A_CNT_BLK is non-zero: 0x%x", + fadt->pm1a_cnt_blk); + } + if (fadt->pm1b_cnt_blk != 0) { + passed = false; + fwts_log_info(fw, "PM1B_CNT_BLK is non-zero: 0x%x", + fadt->pm1b_cnt_blk); + } + if (fadt->pm2_cnt_blk != 0) { + passed = false; + fwts_log_info(fw, "PM2_CNT_BLK is non-zero: 0x%x", + fadt->pm2_cnt_blk); + } + if (fadt->pm_tmr_blk != 0) { + passed = false; + fwts_log_info(fw, "PM_TMR_BLK is non-zero: 0x%x", + fadt->pm_tmr_blk); + } + if (fadt->gpe0_blk != 0) { + passed = false; + fwts_log_info(fw, "GPE0_BLK is non-zero: 0x%x", + fadt->gpe0_blk); + } + if (fadt->gpe1_blk != 0) { + passed = false; + fwts_log_info(fw, "GPE1_BLK is non-zero: 0x%x", + fadt->gpe1_blk); + } + if (fadt->pm1_evt_len != 0) { + passed = false; + fwts_log_info(fw, "PM1_EVT_LEN is non-zero: 0x%x", + fadt->pm1_evt_len); + } + if (fadt->pm1_cnt_len != 0) { + passed = false; + fwts_log_info(fw, "PM1_CNT_LEN is non-zero: 0x%x", + fadt->pm1_cnt_len); + } + if (fadt->pm2_cnt_len != 0) { + passed = false; + fwts_log_info(fw, "PM2_CNT_LEN is non-zero: 0x%x", + fadt->pm2_cnt_len); + } + if (fadt->pm_tmr_len != 0) { + passed = false; + fwts_log_info(fw, "PM_TMR_LEN is non-zero: 0x%x", + fadt->pm_tmr_len); + } + if (fadt->gpe0_blk_len != 0) { + passed = false; + fwts_log_info(fw, "GPE0_BLK_LEN is non-zero: 0x%x", + fadt->gpe0_blk_len); + } + if (fadt->gpe1_blk_len != 0) { + passed = false; + fwts_log_info(fw, "GPE1_BLK_LEN is non-zero: 0x%x", + fadt->gpe1_blk_len); + } + if (fadt->gpe1_base != 0) { + passed = false; + fwts_log_info(fw, "GPE1_BASE is non-zero: 0x%x", + fadt->gpe1_base); + } + if (fadt->cst_cnt != 0) { + passed = false; + fwts_log_info(fw, "CST_CNT is non-zero: 0x%x", + fadt->cst_cnt); + } + if (fadt->p_lvl2_lat != 0) { + passed = false; + fwts_log_info(fw, "P_LVL2_LAT is non-zero: 0x%x", + fadt->p_lvl2_lat); + } + if (fadt->p_lvl3_lat != 0) { + passed = false; + fwts_log_info(fw, "P_LVL3_LAT is non-zero: 0x%x", + fadt->p_lvl3_lat); + } + if (fadt->flush_size != 0) { + passed = false; + fwts_log_info(fw, "FLUSH_SIZE is non-zero: 0x%x", + fadt->flush_size); + } + if (fadt->flush_stride != 0) { + passed = false; + fwts_log_info(fw, "FLUSH_STRIDE is non-zero: 0x%x", + fadt->flush_stride); + } + if (fadt->duty_offset != 0) { + passed = false; + fwts_log_info(fw, "DUTY_OFFSET is non-zero: 0x%x", + fadt->duty_offset); + } + if (fadt->duty_width != 0) { + passed = false; + fwts_log_info(fw, "DUTY_WIDTH is non-zero: 0x%x", + fadt->duty_width); + } + if (fadt->day_alrm != 0) { + passed = false; + fwts_log_info(fw, "DAY_ALRM is non-zero: 0x%x", + fadt->day_alrm); + } + if (fadt->mon_alrm != 0) { + passed = false; + fwts_log_info(fw, "MON_ALRM is non-zero: 0x%x", + fadt->mon_alrm); + } + if (fadt->century != 0) { + passed = false; + fwts_log_info(fw, "CENTURY is non-zero: 0x%x", + fadt->century); + } + if (memcmp((void *)&fadt->x_pm1a_evt_blk, + (void *)&null_gas, + sizeof(fwts_acpi_gas))) { + passed = false; + fwts_log_info(fw, + "X_PM1A_EVT_BLK is a non-zero general " + "address structure."); + } + if (memcmp((void *)&fadt->x_pm1b_evt_blk, + (void *)&null_gas, + sizeof(fwts_acpi_gas))) { + passed = false; + fwts_log_info(fw, + "X_PM1B_EVT_BLK is a non-zero general " + "address structure."); + } + if (memcmp((void *)&fadt->x_pm1a_cnt_blk, + (void *)&null_gas, + sizeof(fwts_acpi_gas))) { + passed = false; + fwts_log_info(fw, + "X_PM1A_CNT_BLK is a non-zero general " + "address structure."); + } + if (memcmp((void *)&fadt->x_pm1b_cnt_blk, + (void *)&null_gas, + sizeof(fwts_acpi_gas))) { + passed = false; + fwts_log_info(fw, + "X_PM1B_CNT_BLK is a non-zero general " + "address structure."); + } + if (memcmp((void *)&fadt->x_pm2_cnt_blk, + (void *)&null_gas, + sizeof(fwts_acpi_gas))) { + passed = false; + fwts_log_info(fw, + "X_PM2_CNT_BLK is a non-zero general " + "address structure."); + } + if (memcmp((void *)&fadt->x_pm_tmr_blk, + (void *)&null_gas, + sizeof(fwts_acpi_gas))) { + passed = false; + fwts_log_info(fw, + "X_PM_TMR_BLK is a non-zero general " + "address structure."); + } + if (memcmp((void *)&fadt->x_gpe0_blk, + (void *)&null_gas, + sizeof(fwts_acpi_gas))) { + passed = false; + fwts_log_info(fw, + "X_GPE0_BLK is a non-zero general " + "address structure."); + } + if (memcmp((void *)&fadt->x_gpe1_blk, + (void *)&null_gas, + sizeof(fwts_acpi_gas))) { + passed = false; + fwts_log_info(fw, + "X_GPE1_BLK is a non-zero general " + "address structure."); + } + + if (passed) + fwts_passed(fw, "All FADT reduced hardware fields are zero."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTRHWNonZero", + "Some FADT reduced hardware fields are non-zero."); + + /* now check all the reserved flags */ + flag_mask = FWTS_FACP_FLAG_WBINVD_FLUSH | + FWTS_FACP_FLAG_PROC_C1 | + FWTS_FACP_FLAG_P_LVL2_UP | + FWTS_FACP_FLAG_RTC_S4 | + FWTS_FACP_FLAG_TMR_VAL_EXT | + FWTS_FACP_FLAG_HEADLESS | + FWTS_FACP_FLAG_CPU_SW_SLP | + FWTS_FACP_FLAG_PCI_EXP_WAK | + FWTS_FACP_FLAG_S4_RTC_STS_VALID | + FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE; + + if (fadt->flags & flag_mask) + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTRHWFlagsNonZero", + "Some FADT reduced hardware flags are set."); + else + fwts_passed(fw, "All FADT reduced hardware flags are not set."); + + + if ((fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL) || + (fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE)) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTRHWAPICFlags", + "FADT APIC flags are set for reduced hardware " + "mode but may be irrelevant."); + else + fwts_passed(fw, + "FADT APIC flags are not set in reduced " + "hardware mode."); +}
static void acpi_table_check_fadt_smi( fwts_framework *fw, @@ -545,6 +845,9 @@ static int fadt_test1(fwts_framework *fw)
acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw); + acpi_table_check_fadt_reserved(fw); + acpi_table_check_fadt_pm_profile(fw); + acpi_table_check_fadt_reduced_hardware(fw); acpi_table_check_fadt_smi(fw, fadt, &passed); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); acpi_table_check_fadt_gpe(fw, fadt, &passed);
On 09/02/16 01:32, Al Stone wrote:
Add three new compliance tests that verify that the FADT reserved fields are zero as they should be, that the PM (Power Management) profile is a legal value, and that all of the fields and flags that are to be ignored when the OSPM is in reduced hardware mode are zero.
Note that the spec says to ignore many FADT fields when in reduced hardware mode. As of 6.1, it does not explicitly state that they must be zero. But, zero seems to be a reasonable value -- or at least not destructive -- on the OSPMs that use them when they should be ignored. The case can also be made that the fields should be zero; section 5.2.1.4 indicates ignored fields should be treated the same as reserved fields, which _are_ required to be zero. As a practical matter, some vendors may have chosen to sidestep the spec here and set reduced hardware mode but used the ignored field anyway.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 303 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 303 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index b21653d..fd430ec 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -359,6 +359,306 @@ static void acpi_table_check_fadt_dsdt(fwts_framework *fw) } } +static void acpi_table_check_fadt_reserved(fwts_framework *fw) +{
- if (fadt->reserved == (uint8_t)0)
fwts_passed(fw, "FADT first reserved field is zero.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTReservedZero","FADT first reserved field is not zero: 0x%02x",fadt->reserved);- if (fadt->reserved1 == (uint8_t)0)
fwts_passed(fw, "FADT second reserved field is zero.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTReservedZero","FADT second reserved field is not zero: 0x%02x",fadt->reserved1);+}
+static void acpi_table_check_fadt_pm_profile(fwts_framework *fw) +{
- fwts_log_info(fw, "FADT Preferred PM Profile: %hhu (%s)\n",
fadt->preferred_pm_profile,FWTS_ACPI_FADT_PREFERRED_PM_PROFILE(fadt->preferred_pm_profile));- if (fadt->preferred_pm_profile <= 8)
fwts_passed(fw, "FADT has a valid preferred PM profile.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTPMProfile","FADT preferred PM profile is invalid.");+}
+static void acpi_table_check_fadt_reduced_hardware(fwts_framework *fw) +{
- const char *IS = "IS";
- const char *IS_NOT = "IS NOT";
- bool rhw;
- bool passed;
- const fwts_acpi_gas null_gas = { 0 };
- uint32_t flag_mask;
- rhw = fwts_acpi_is_reduced_hardware(fadt);
- fwts_log_info(fw, "FADT indicates ACPI %s in reduced hardware mode.",
rhw ? IS : IS_NOT);- if (!rhw)
return;- passed = true;
- /* check all the fields that will be ignored */
- if (fadt->smi_cmd != 0) {
passed = false;fwts_log_info(fw, "SMI_CMD is non-zero: 0x%x",fadt->smi_cmd);- }
- if (fadt->acpi_enable != 0) {
passed = false;fwts_log_info(fw, "ACPI_ENABLE is non-zero: 0x%x",fadt->acpi_enable);- }
- if (fadt->acpi_disable != 0) {
passed = false;fwts_log_info(fw, "ACPI_DISABLE is non-zero: 0x%x",fadt->acpi_disable);- }
- if (fadt->s4bios_req != 0) {
passed = false;fwts_log_info(fw, "S4BIOS_REQ is non-zero: 0x%x",fadt->s4bios_req);- }
- if (fadt->pstate_cnt != 0) {
passed = false;fwts_log_info(fw, "PSTATE_CNT is non-zero: 0x%x",fadt->pstate_cnt);- }
- if (fadt->pm1a_evt_blk != 0) {
passed = false;fwts_log_info(fw, "PM1A_EVT_BLK is non-zero: 0x%x",fadt->pm1a_evt_blk);- }
- if (fadt->pm1b_evt_blk != 0) {
passed = false;fwts_log_info(fw, "PM1B_EVT_BLK is non-zero: 0x%x",fadt->pm1b_evt_blk);- }
- if (fadt->pm1a_cnt_blk != 0) {
passed = false;fwts_log_info(fw, "PM1A_CNT_BLK is non-zero: 0x%x",fadt->pm1a_cnt_blk);- }
- if (fadt->pm1b_cnt_blk != 0) {
passed = false;fwts_log_info(fw, "PM1B_CNT_BLK is non-zero: 0x%x",fadt->pm1b_cnt_blk);- }
- if (fadt->pm2_cnt_blk != 0) {
passed = false;fwts_log_info(fw, "PM2_CNT_BLK is non-zero: 0x%x",fadt->pm2_cnt_blk);- }
- if (fadt->pm_tmr_blk != 0) {
passed = false;fwts_log_info(fw, "PM_TMR_BLK is non-zero: 0x%x",fadt->pm_tmr_blk);- }
- if (fadt->gpe0_blk != 0) {
passed = false;fwts_log_info(fw, "GPE0_BLK is non-zero: 0x%x",fadt->gpe0_blk);- }
- if (fadt->gpe1_blk != 0) {
passed = false;fwts_log_info(fw, "GPE1_BLK is non-zero: 0x%x",fadt->gpe1_blk);- }
- if (fadt->pm1_evt_len != 0) {
passed = false;fwts_log_info(fw, "PM1_EVT_LEN is non-zero: 0x%x",fadt->pm1_evt_len);- }
- if (fadt->pm1_cnt_len != 0) {
passed = false;fwts_log_info(fw, "PM1_CNT_LEN is non-zero: 0x%x",fadt->pm1_cnt_len);- }
- if (fadt->pm2_cnt_len != 0) {
passed = false;fwts_log_info(fw, "PM2_CNT_LEN is non-zero: 0x%x",fadt->pm2_cnt_len);- }
- if (fadt->pm_tmr_len != 0) {
passed = false;fwts_log_info(fw, "PM_TMR_LEN is non-zero: 0x%x",fadt->pm_tmr_len);- }
- if (fadt->gpe0_blk_len != 0) {
passed = false;fwts_log_info(fw, "GPE0_BLK_LEN is non-zero: 0x%x",fadt->gpe0_blk_len);- }
- if (fadt->gpe1_blk_len != 0) {
passed = false;fwts_log_info(fw, "GPE1_BLK_LEN is non-zero: 0x%x",fadt->gpe1_blk_len);- }
- if (fadt->gpe1_base != 0) {
passed = false;fwts_log_info(fw, "GPE1_BASE is non-zero: 0x%x",fadt->gpe1_base);- }
- if (fadt->cst_cnt != 0) {
passed = false;fwts_log_info(fw, "CST_CNT is non-zero: 0x%x",fadt->cst_cnt);- }
- if (fadt->p_lvl2_lat != 0) {
passed = false;fwts_log_info(fw, "P_LVL2_LAT is non-zero: 0x%x",fadt->p_lvl2_lat);- }
- if (fadt->p_lvl3_lat != 0) {
passed = false;fwts_log_info(fw, "P_LVL3_LAT is non-zero: 0x%x",fadt->p_lvl3_lat);- }
- if (fadt->flush_size != 0) {
passed = false;fwts_log_info(fw, "FLUSH_SIZE is non-zero: 0x%x",fadt->flush_size);- }
- if (fadt->flush_stride != 0) {
passed = false;fwts_log_info(fw, "FLUSH_STRIDE is non-zero: 0x%x",fadt->flush_stride);- }
- if (fadt->duty_offset != 0) {
passed = false;fwts_log_info(fw, "DUTY_OFFSET is non-zero: 0x%x",fadt->duty_offset);- }
- if (fadt->duty_width != 0) {
passed = false;fwts_log_info(fw, "DUTY_WIDTH is non-zero: 0x%x",fadt->duty_width);- }
- if (fadt->day_alrm != 0) {
passed = false;fwts_log_info(fw, "DAY_ALRM is non-zero: 0x%x",fadt->day_alrm);- }
- if (fadt->mon_alrm != 0) {
passed = false;fwts_log_info(fw, "MON_ALRM is non-zero: 0x%x",fadt->mon_alrm);- }
- if (fadt->century != 0) {
passed = false;fwts_log_info(fw, "CENTURY is non-zero: 0x%x",fadt->century);- }
- if (memcmp((void *)&fadt->x_pm1a_evt_blk,
(void *)&null_gas,sizeof(fwts_acpi_gas))) {passed = false;fwts_log_info(fw,"X_PM1A_EVT_BLK is a non-zero general ""address structure.");- }
- if (memcmp((void *)&fadt->x_pm1b_evt_blk,
(void *)&null_gas,sizeof(fwts_acpi_gas))) {passed = false;fwts_log_info(fw,"X_PM1B_EVT_BLK is a non-zero general ""address structure.");- }
- if (memcmp((void *)&fadt->x_pm1a_cnt_blk,
(void *)&null_gas,sizeof(fwts_acpi_gas))) {passed = false;fwts_log_info(fw,"X_PM1A_CNT_BLK is a non-zero general ""address structure.");- }
- if (memcmp((void *)&fadt->x_pm1b_cnt_blk,
(void *)&null_gas,sizeof(fwts_acpi_gas))) {passed = false;fwts_log_info(fw,"X_PM1B_CNT_BLK is a non-zero general ""address structure.");- }
- if (memcmp((void *)&fadt->x_pm2_cnt_blk,
(void *)&null_gas,sizeof(fwts_acpi_gas))) {passed = false;fwts_log_info(fw,"X_PM2_CNT_BLK is a non-zero general ""address structure.");- }
- if (memcmp((void *)&fadt->x_pm_tmr_blk,
(void *)&null_gas,sizeof(fwts_acpi_gas))) {passed = false;fwts_log_info(fw,"X_PM_TMR_BLK is a non-zero general ""address structure.");- }
- if (memcmp((void *)&fadt->x_gpe0_blk,
(void *)&null_gas,sizeof(fwts_acpi_gas))) {passed = false;fwts_log_info(fw,"X_GPE0_BLK is a non-zero general ""address structure.");- }
- if (memcmp((void *)&fadt->x_gpe1_blk,
(void *)&null_gas,sizeof(fwts_acpi_gas))) {passed = false;fwts_log_info(fw,"X_GPE1_BLK is a non-zero general ""address structure.");- }
- if (passed)
fwts_passed(fw, "All FADT reduced hardware fields are zero.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTRHWNonZero","Some FADT reduced hardware fields are non-zero.");- /* now check all the reserved flags */
- flag_mask = FWTS_FACP_FLAG_WBINVD_FLUSH |
FWTS_FACP_FLAG_PROC_C1 |FWTS_FACP_FLAG_P_LVL2_UP |FWTS_FACP_FLAG_RTC_S4 |FWTS_FACP_FLAG_TMR_VAL_EXT |FWTS_FACP_FLAG_HEADLESS |FWTS_FACP_FLAG_CPU_SW_SLP |FWTS_FACP_FLAG_PCI_EXP_WAK |FWTS_FACP_FLAG_S4_RTC_STS_VALID |FWTS_FACP_FLAG_REMOTE_POWER_ON_CAPABLE;- if (fadt->flags & flag_mask)
fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTRHWFlagsNonZero","Some FADT reduced hardware flags are set.");- else
fwts_passed(fw, "All FADT reduced hardware flags are not set.");- if ((fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_CLUSTER_MODEL) ||
(fadt->flags & FWTS_FACP_FLAG_FORCE_APIC_PHYSICAL_DESTINATION_MODE))fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTRHWAPICFlags","FADT APIC flags are set for reduced hardware ""mode but may be irrelevant.");- else
fwts_passed(fw,"FADT APIC flags are not set in reduced ""hardware mode.");+} static void acpi_table_check_fadt_smi( fwts_framework *fw, @@ -545,6 +845,9 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw);
- acpi_table_check_fadt_reserved(fw);
- acpi_table_check_fadt_pm_profile(fw);
- acpi_table_check_fadt_reduced_hardware(fw); acpi_table_check_fadt_smi(fw, fadt, &passed); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); acpi_table_check_fadt_gpe(fw, fadt, &passed);
Acked-by: Colin Ian King colin.king@canonucal.com
Since a prior patch added a test of of the reduced hardware fields, we can now resequence the tests so that we only test the fields we must.
If we are not in reduced hardware mode, we test everything. If we are, we ignore tests for many fields. They are simply irrelevant, and if the fields are non-zero, that would have been caught in the prior patch.
There are also several fields that really cannot be reliably tested; we cannot gather enough info to know whether they are correct or not, or the field is allowed to be any value that it can hold. Simply log the value for these fields, but only when in the proper mode.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index fd430ec..430bfd4 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -848,10 +848,34 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_reserved(fw); acpi_table_check_fadt_pm_profile(fw); acpi_table_check_fadt_reduced_hardware(fw); - acpi_table_check_fadt_smi(fw, fadt, &passed); - acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); - acpi_table_check_fadt_gpe(fw, fadt, &passed); - acpi_table_check_fadt_pm_addr(fw, fadt, &passed); + + /* + * If a field can be tested, we call a function to do so. If + * any value is reasonable and allowable, we simply log the value. + * For example, the SCI_INT is one byte and can be from 0..255, and + * there is no other info (as far as this author knows) that can be + * used to verify that the value is correct. + */ + if (!fwts_acpi_is_reduced_hardware(fadt)) { + fwts_log_info(fw, "FADT SCI_INT is %" PRIu8, fadt->sci_int); + acpi_table_check_fadt_smi(fw, fadt, &passed); + acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); + acpi_table_check_fadt_gpe(fw, fadt, &passed); + acpi_table_check_fadt_pm_addr(fw, fadt, &passed); + fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base); + + fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16, + fadt->flush_size); + fwts_log_info(fw, "FADT FLUSH_STRIDE is %" PRIu16, + fadt->flush_stride); + fwts_log_info(fw, "FADT DUTY_OFFSET is %" PRIu8, + fadt->duty_offset); + fwts_log_info(fw, "FADT DUTY_WIDTH is %" PRIu8, + fadt->duty_width); + fwts_log_info(fw, "FADT DAY_ALRM is %" PRIu8, fadt->day_alrm); + fwts_log_info(fw, "FADT MON_ALRM is %" PRIu8, fadt->mon_alrm); + fwts_log_info(fw, "FADT CENTURY is %" PRIu8, fadt->century); + }
/* * Bug LP: #833644 @@ -878,6 +902,14 @@ static int fadt_test1(fwts_framework *fw) } */
+ /* + * Cannot really test the Hypervisor Vendor Identity since + * the value is provided by the hypervisor to the OS (as a + * sign that the ACPI tables have been fabricated), if it + * being used at all. Or, it's set to zero. + */ + fwts_log_info(fw, "FADT Hypervisor Vendor Identity is %" PRIu64, + fadt->hypervisor_id); if (passed) fwts_passed(fw, "No issues found in FADT table.");
On 09/02/16 01:32, Al Stone wrote:
Since a prior patch added a test of of the reduced hardware fields, we can now resequence the tests so that we only test the fields we must.
If we are not in reduced hardware mode, we test everything. If we are, we ignore tests for many fields. They are simply irrelevant, and if the fields are non-zero, that would have been caught in the prior patch.
There are also several fields that really cannot be reliably tested; we cannot gather enough info to know whether they are correct or not, or the field is allowed to be any value that it can hold. Simply log the value for these fields, but only when in the proper mode.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index fd430ec..430bfd4 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -848,10 +848,34 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_reserved(fw); acpi_table_check_fadt_pm_profile(fw); acpi_table_check_fadt_reduced_hardware(fw);
- acpi_table_check_fadt_smi(fw, fadt, &passed);
- acpi_table_check_fadt_pm_tmr(fw, fadt, &passed);
- acpi_table_check_fadt_gpe(fw, fadt, &passed);
- acpi_table_check_fadt_pm_addr(fw, fadt, &passed);
- /*
* If a field can be tested, we call a function to do so. If* any value is reasonable and allowable, we simply log the value.* For example, the SCI_INT is one byte and can be from 0..255, and* there is no other info (as far as this author knows) that can be* used to verify that the value is correct.*/- if (!fwts_acpi_is_reduced_hardware(fadt)) {
fwts_log_info(fw, "FADT SCI_INT is %" PRIu8, fadt->sci_int);acpi_table_check_fadt_smi(fw, fadt, &passed);acpi_table_check_fadt_pm_tmr(fw, fadt, &passed);acpi_table_check_fadt_gpe(fw, fadt, &passed);acpi_table_check_fadt_pm_addr(fw, fadt, &passed);fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base);fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16,fadt->flush_size);fwts_log_info(fw, "FADT FLUSH_STRIDE is %" PRIu16,fadt->flush_stride);fwts_log_info(fw, "FADT DUTY_OFFSET is %" PRIu8,fadt->duty_offset);fwts_log_info(fw, "FADT DUTY_WIDTH is %" PRIu8,fadt->duty_width);fwts_log_info(fw, "FADT DAY_ALRM is %" PRIu8, fadt->day_alrm);fwts_log_info(fw, "FADT MON_ALRM is %" PRIu8, fadt->mon_alrm);fwts_log_info(fw, "FADT CENTURY is %" PRIu8, fadt->century);- }
/* * Bug LP: #833644 @@ -878,6 +902,14 @@ static int fadt_test1(fwts_framework *fw) } */
- /*
* Cannot really test the Hypervisor Vendor Identity since* the value is provided by the hypervisor to the OS (as a* sign that the ACPI tables have been fabricated), if it* being used at all. Or, it's set to zero.*/- fwts_log_info(fw, "FADT Hypervisor Vendor Identity is %" PRIu64,
fadt->hypervisor_id);
Seems very reasonable to me.
Acked-by: Colin Ian King colin.king@canonucal.com
Minor tweaks to the tests of the SMI_CMD to make them only slightly more extensive, but also to fit within the resequencing of tests for reduced hardware mode..
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 68 ++++++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 34 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 430bfd4..3da297e 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -660,19 +660,8 @@ static void acpi_table_check_fadt_reduced_hardware(fwts_framework *fw) "hardware mode."); }
-static void acpi_table_check_fadt_smi( - fwts_framework *fw, - const fwts_acpi_table_fadt *fadt, - bool *passed) +static void acpi_table_check_fadt_smi_cmd(fwts_framework *fw) { - if (fwts_acpi_is_reduced_hardware(fadt)) { - if (fadt->smi_cmd != 0) { - fwts_warning(fw, "FADT SMI_CMD is not zero " - "but should be in reduced hardware mode."); - } - return; - } - /* * Section 5.2.9 (Fixed ACPI Description Table) of the ACPI 5.0 * specification states that if SMI_CMD is zero then it is @@ -680,38 +669,49 @@ static void acpi_table_check_fadt_smi( * in that case, don't check SCI_INT being valid. */ if (fadt->smi_cmd != 0) { - if (fadt->sci_int == 0) { - *passed = false; + if (fadt->sci_int == 0) fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FADTSCIIRQZero", - "FADT SCI Interrupt is 0x00, should be defined."); - } + "FADTSCIIRQZero", + "FADT SCI Interrupt is 0x00, but must " + "be defined since SMI command indicates " + "System Management Mode is supported."); + else + fwts_passed(fw, + "FADT SMI_CMD indicates System Management " + "Mode is supported, and the SCI Interrupt " + "is non-zero."); } else { if ((fadt->acpi_enable == 0) && (fadt->acpi_disable == 0) && (fadt->s4bios_req == 0) && (fadt->pstate_cnt == 0) && (fadt->cst_cnt == 0)) { - /* Not an error, but intentional, but feedback this finding anyhow */ - fwts_log_info(fw, "The FADT SMI_CMD is zero, system " - "does not support System Management Mode."); + /* + * Not an error, but intentional, so feedback + * this finding. + */ + fwts_passed(fw, "The FADT SMI_CMD is zero, system " + "does not support System Management Mode."); } else { - *passed = false; fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FADTSMICMDZero", - "FADT SMI_CMD is 0x00, however, one or more of " - "ACPI_ENABLE, ACPI_DISABLE, S4BIOS_REQ, PSTATE_CNT " - "and CST_CNT are defined which means SMI_CMD should " - "be defined otherwise SMI commands cannot be sent."); + "FADTSMICMDZero", + "FADT SMI_CMD is 0x00, however, one or " + "more of ACPI_ENABLE, ACPI_DISABLE, " + "S4BIOS_REQ, PSTATE_CNT and CST_CNT are " + "defined which means SMI_CMD should be " + "defined, otherwise SMI commands cannot " + "be sent."); fwts_advice(fw, - "The configuration seems to suggest that SMI command " - "should be defined to allow the kernel to trigger " - "system management interrupts via the SMD_CMD port. " - "The fact that SMD_CMD is zero which is invalid means " - "that SMIs are not possible through the normal ACPI " - "mechanisms. This means some firmware based machine " - "specific functions will not work."); + "The configuration seems to suggest that " + "SMI command should be defined to allow " + "the kernel to trigger system management " + "interrupts via the SMD_CMD port. The " + "fact that SMD_CMD is zero which is " + "invalid means that SMIs are not possible " + "through the normal ACPI mechanisms. This " + "means some firmware based machine " + "specific functions will not work."); } } } @@ -858,7 +858,7 @@ static int fadt_test1(fwts_framework *fw) */ if (!fwts_acpi_is_reduced_hardware(fadt)) { fwts_log_info(fw, "FADT SCI_INT is %" PRIu8, fadt->sci_int); - acpi_table_check_fadt_smi(fw, fadt, &passed); + acpi_table_check_fadt_smi_cmd(fw); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); acpi_table_check_fadt_gpe(fw, fadt, &passed); acpi_table_check_fadt_pm_addr(fw, fadt, &passed);
On 09/02/16 01:32, Al Stone wrote:
Minor tweaks to the tests of the SMI_CMD to make them only slightly more extensive, but also to fit within the resequencing of tests for reduced hardware mode..
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 68 ++++++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 34 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 430bfd4..3da297e 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -660,19 +660,8 @@ static void acpi_table_check_fadt_reduced_hardware(fwts_framework *fw) "hardware mode."); } -static void acpi_table_check_fadt_smi(
- fwts_framework *fw,
- const fwts_acpi_table_fadt *fadt,
- bool *passed)
+static void acpi_table_check_fadt_smi_cmd(fwts_framework *fw) {
- if (fwts_acpi_is_reduced_hardware(fadt)) {
if (fadt->smi_cmd != 0) {fwts_warning(fw, "FADT SMI_CMD is not zero ""but should be in reduced hardware mode.");}return;- }
- /*
- Section 5.2.9 (Fixed ACPI Description Table) of the ACPI 5.0
- specification states that if SMI_CMD is zero then it is
@@ -680,38 +669,49 @@ static void acpi_table_check_fadt_smi( * in that case, don't check SCI_INT being valid. */ if (fadt->smi_cmd != 0) {
if (fadt->sci_int == 0) {*passed = false;
if (fadt->sci_int == 0) fwts_failed(fw, LOG_LEVEL_MEDIUM,
"FADTSCIIRQZero","FADT SCI Interrupt is 0x00, should be defined.");}
"FADTSCIIRQZero","FADT SCI Interrupt is 0x00, but must ""be defined since SMI command indicates ""System Management Mode is supported.");elsefwts_passed(fw,"FADT SMI_CMD indicates System Management ""Mode is supported, and the SCI Interrupt ""is non-zero.");
/* Not an error, but intentional, but feedback this finding anyhow */fwts_log_info(fw, "The FADT SMI_CMD is zero, system ""does not support System Management Mode.");
/** Not an error, but intentional, so feedback* this finding.*/fwts_passed(fw, "The FADT SMI_CMD is zero, system ""does not support System Management Mode.");
*passed = false; fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTSMICMDZero","FADT SMI_CMD is 0x00, however, one or more of ""ACPI_ENABLE, ACPI_DISABLE, S4BIOS_REQ, PSTATE_CNT ""and CST_CNT are defined which means SMI_CMD should ""be defined otherwise SMI commands cannot be sent.");
"FADTSMICMDZero","FADT SMI_CMD is 0x00, however, one or ""more of ACPI_ENABLE, ACPI_DISABLE, ""S4BIOS_REQ, PSTATE_CNT and CST_CNT are ""defined which means SMI_CMD should be ""defined, otherwise SMI commands cannot ""be sent."); fwts_advice(fw,
"The configuration seems to suggest that SMI command ""should be defined to allow the kernel to trigger ""system management interrupts via the SMD_CMD port. ""The fact that SMD_CMD is zero which is invalid means ""that SMIs are not possible through the normal ACPI ""mechanisms. This means some firmware based machine ""specific functions will not work.");
"The configuration seems to suggest that ""SMI command should be defined to allow ""the kernel to trigger system management ""interrupts via the SMD_CMD port. The ""fact that SMD_CMD is zero which is ""invalid means that SMIs are not possible ""through the normal ACPI mechanisms. This ""means some firmware based machine ""specific functions will not work.");} @@ -858,7 +858,7 @@ static int fadt_test1(fwts_framework *fw) */ if (!fwts_acpi_is_reduced_hardware(fadt)) { fwts_log_info(fw, "FADT SCI_INT is %" PRIu8, fadt->sci_int);
acpi_table_check_fadt_smi(fw, fadt, &passed);
acpi_table_check_fadt_smi_cmd(fw);
Acked-by: Colin Ian King colin.king@canonucal.com
Do some checking of allowed values for ACPI_ENABLE and ACPI_DISABLE fields, particularly if SMI_CMD is defined.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 3da297e..a2535e8 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -716,6 +716,59 @@ static void acpi_table_check_fadt_smi_cmd(fwts_framework *fw) } }
+static void acpi_table_check_fadt_acpi_enable(fwts_framework *fw) +{ + if (fadt->acpi_enable == 0) + if (fadt->smi_cmd == 0) + fwts_passed(fw, "FADT SMI ACPI enable command is zero, " + "which is allowed since SMM is not " + "supported, or machine is in legacy mode."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SMMHasNoAcpiEnableCmd", + "FADT SMI ACPI enable command is zero, " + "but this is not allowed when SMM " + "is supported."); + else + if (fadt->smi_cmd == 0) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SMMNeedsAcpiEnableCmd", + "FADT SMI ACPI enable command is non-zero, " + "but SMM is not supported."); + else + fwts_passed(fw, "FADT SMI ACPI enable command is " + "non-zero, and SMM is supported."); + + return; +} + +static void acpi_table_check_fadt_acpi_disable(fwts_framework *fw) +{ + if (fadt->acpi_disable == 0) + if (fadt->smi_cmd == 0) + fwts_passed(fw, + "FADT SMI ACPI disable command is zero, " + "which is allowed since SMM is not " + "supported, or machine is in legacy mode."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SMMHasNoAcpiDisableCmd", + "FADT SMI ACPI disable command is zero, " + "but this is not allowed when SMM " + "is supported."); + else + if (fadt->smi_cmd == 0) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SMMNeedsAcpiDisableCmd", + "FADT SMI ACPI disable command is " + "non-zero, but SMM is not supported."); + else + fwts_passed(fw, "FADT SMI ACPI disable command is " + "non-zero, and SMM is supported."); + + return; +} + static void acpi_table_check_fadt_pm_tmr( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -859,6 +912,8 @@ static int fadt_test1(fwts_framework *fw) if (!fwts_acpi_is_reduced_hardware(fadt)) { fwts_log_info(fw, "FADT SCI_INT is %" PRIu8, fadt->sci_int); acpi_table_check_fadt_smi_cmd(fw); + acpi_table_check_fadt_acpi_enable(fw); + acpi_table_check_fadt_acpi_disable(fw); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); acpi_table_check_fadt_gpe(fw, fadt, &passed); acpi_table_check_fadt_pm_addr(fw, fadt, &passed);
On 09/02/16 01:32, Al Stone wrote:
Do some checking of allowed values for ACPI_ENABLE and ACPI_DISABLE fields, particularly if SMI_CMD is defined.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 3da297e..a2535e8 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -716,6 +716,59 @@ static void acpi_table_check_fadt_smi_cmd(fwts_framework *fw) } } +static void acpi_table_check_fadt_acpi_enable(fwts_framework *fw) +{
- if (fadt->acpi_enable == 0)
if (fadt->smi_cmd == 0)fwts_passed(fw, "FADT SMI ACPI enable command is zero, ""which is allowed since SMM is not ""supported, or machine is in legacy mode.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"SMMHasNoAcpiEnableCmd","FADT SMI ACPI enable command is zero, ""but this is not allowed when SMM ""is supported.");- else
if (fadt->smi_cmd == 0)fwts_failed(fw, LOG_LEVEL_MEDIUM,"SMMNeedsAcpiEnableCmd","FADT SMI ACPI enable command is non-zero, ""but SMM is not supported.");elsefwts_passed(fw, "FADT SMI ACPI enable command is ""non-zero, and SMM is supported.");- return;
+}
+static void acpi_table_check_fadt_acpi_disable(fwts_framework *fw) +{
- if (fadt->acpi_disable == 0)
if (fadt->smi_cmd == 0)fwts_passed(fw,"FADT SMI ACPI disable command is zero, ""which is allowed since SMM is not ""supported, or machine is in legacy mode.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"SMMHasNoAcpiDisableCmd","FADT SMI ACPI disable command is zero, ""but this is not allowed when SMM ""is supported.");- else
if (fadt->smi_cmd == 0)fwts_failed(fw, LOG_LEVEL_MEDIUM,"SMMNeedsAcpiDisableCmd","FADT SMI ACPI disable command is ""non-zero, but SMM is not supported.");elsefwts_passed(fw, "FADT SMI ACPI disable command is ""non-zero, and SMM is supported.");- return;
+}
static void acpi_table_check_fadt_pm_tmr( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -859,6 +912,8 @@ static int fadt_test1(fwts_framework *fw) if (!fwts_acpi_is_reduced_hardware(fadt)) { fwts_log_info(fw, "FADT SCI_INT is %" PRIu8, fadt->sci_int); acpi_table_check_fadt_smi_cmd(fw);
acpi_table_check_fadt_acpi_enable(fw);acpi_table_check_fadt_acpi_disable(fw);
Acked-by: Colin Ian King colin.king@canonucal.com
These are the last two field that are related to the SMI_CMD field and whether or not it is defined. Additionally, this is the first set of tests that require information from the FACS, also, so add in that table to the initialization step.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index a2535e8..3c4cdda 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -41,6 +41,8 @@ static inline int ioperm(int a, ...) static const fwts_acpi_table_fadt *fadt; static int fadt_size;
+static const fwts_acpi_table_facs *facs; + static int fadt_init(fwts_framework *fw) { fwts_acpi_table_info *table; @@ -69,6 +71,23 @@ static int fadt_init(fwts_framework *fw) } }
+ /* + * Some tests require data from the FACS, also, which is + * required (5.2.10) is we are not in reduced hardware + * mode + */ + if (!fwts_acpi_is_reduced_hardware(fadt)) { + if (fwts_acpi_find_table(fw, "FACS", 0, &table) != FWTS_OK) { + fwts_log_error(fw, "Cannot read ACPI table FACS."); + return FWTS_ERROR; + } + if (table == NULL) { + fwts_log_error(fw, "ACPI table FACS does not exist!"); + return FWTS_ERROR; + } + facs = (const fwts_acpi_table_facs *)table->data; + } + return FWTS_OK; }
@@ -769,6 +788,102 @@ static void acpi_table_check_fadt_acpi_disable(fwts_framework *fw) return; }
+static void acpi_table_check_fadt_s4bios_req(fwts_framework *fw) +{ + if (facs && facs->length >= 24) + fwts_passed(fw, + "FADT indicates we are not in reduced hardware " + "mode, and required FACS is present."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FACSMustBePresent", + "FADT indicates we are not in reduced hardware " + "mode, which requires an FACS be present, but " + "none has been found."); + + if (facs && (facs->flags & FWTS_FACS_FLAG_S4BIOS_F)) { + if (fadt->s4bios_req == 0) { + if (fadt->smi_cmd == 0) { + fwts_passed(fw, + "FADT indicates System Management " + "Mode is not supported, which " + "allows a zero S4BIOS_REQ value."); + fwts_advice(fw, + "There is an inconsistency between " + "the FADT and FACS. The FADT " + "indicates no SMM support, and " + "no S4BIOS_REQ command, but the " + "FACS indicates S4BIOS_REQ is " + "supported. One of these may " + "be incorrect."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SMMHasNoS4BIOSReqCmd", + "FADT SMI S4BIOS_REQ command is " + "zero, but this is not allowed " + "when SMM is supported, and the " + "S4BIOS_F flag is set in the " + "FACS."); + } + } else { + if (fadt->smi_cmd == 0) { + fwts_passed(fw, + "FADT indicates System Management " + "Mode is not supported, but a " + "S4BIOS_REQ command is defined."); + fwts_advice(fw, + "There is an inconsistency between " + "the FADT and FACS. The FADT " + "indicates no SMM support, but it " + "defines an S4BIOS_REQ command, " + "and the FACS indicates " + "S4BIOS_REQ is supported. One " + "of these may be incorrect."); + } else { + fwts_passed(fw, + "FADT S4BIOS_REQ command is " + "non-zero, SMM is supported so " + "the command can be used, and " + "the FACS indicates S4BIOS_REQ " + "is supported."); + } + } + } else { + if (fadt->s4bios_req == 0) + fwts_passed(fw, "FADT S4BIOS_REQ command is not set " + "and FACS indicates it is not supported."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SMMS4BIOSCmdDefined", + "FADT S4BIOS_REQ command is defined " + "but FACS indicates it is not supported."); + } + + return; +} + +static void acpi_table_check_fadt_pstate_cnt(fwts_framework *fw) +{ + if (fadt->pstate_cnt == 0) { + if (fadt->smi_cmd == 0) + fwts_passed(fw, + "FADT SMI PSTATE_CNT command is zero, " + "which is allowed since SMM is not " + "supported."); + } else { + if (fadt->smi_cmd == 0) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SMMHasExtraPstateCntCmd", + "FADT SMI PSTATE_CNT command is " + "non-zero, but SMM is not supported."); + else + fwts_passed(fw, "FADT SMI PSTATE_CNT command is " + "non-zero, and SMM is supported."); + } + + return; +} + static void acpi_table_check_fadt_pm_tmr( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -914,6 +1029,8 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_smi_cmd(fw); acpi_table_check_fadt_acpi_enable(fw); acpi_table_check_fadt_acpi_disable(fw); + acpi_table_check_fadt_s4bios_req(fw); + acpi_table_check_fadt_pstate_cnt(fw); acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); acpi_table_check_fadt_gpe(fw, fadt, &passed); acpi_table_check_fadt_pm_addr(fw, fadt, &passed);
On 09/02/16 01:32, Al Stone wrote:
These are the last two field that are related to the SMI_CMD field and whether or not it is defined. Additionally, this is the first set of tests that require information from the FACS, also, so add in that table to the initialization step.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index a2535e8..3c4cdda 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -41,6 +41,8 @@ static inline int ioperm(int a, ...) static const fwts_acpi_table_fadt *fadt; static int fadt_size; +static const fwts_acpi_table_facs *facs;
static int fadt_init(fwts_framework *fw) { fwts_acpi_table_info *table; @@ -69,6 +71,23 @@ static int fadt_init(fwts_framework *fw) } }
- /*
* Some tests require data from the FACS, also, which is* required (5.2.10) is we are not in reduced hardware* mode*/- if (!fwts_acpi_is_reduced_hardware(fadt)) {
if (fwts_acpi_find_table(fw, "FACS", 0, &table) != FWTS_OK) {fwts_log_error(fw, "Cannot read ACPI table FACS.");return FWTS_ERROR;}if (table == NULL) {fwts_log_error(fw, "ACPI table FACS does not exist!");return FWTS_ERROR;}facs = (const fwts_acpi_table_facs *)table->data;- }
- return FWTS_OK;
} @@ -769,6 +788,102 @@ static void acpi_table_check_fadt_acpi_disable(fwts_framework *fw) return; } +static void acpi_table_check_fadt_s4bios_req(fwts_framework *fw) +{
- if (facs && facs->length >= 24)
fwts_passed(fw,"FADT indicates we are not in reduced hardware ""mode, and required FACS is present.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FACSMustBePresent","FADT indicates we are not in reduced hardware ""mode, which requires an FACS be present, but ""none has been found.");- if (facs && (facs->flags & FWTS_FACS_FLAG_S4BIOS_F)) {
if (fadt->s4bios_req == 0) {if (fadt->smi_cmd == 0) {fwts_passed(fw,"FADT indicates System Management ""Mode is not supported, which ""allows a zero S4BIOS_REQ value.");fwts_advice(fw,"There is an inconsistency between ""the FADT and FACS. The FADT ""indicates no SMM support, and ""no S4BIOS_REQ command, but the ""FACS indicates S4BIOS_REQ is ""supported. One of these may ""be incorrect.");} else {fwts_failed(fw, LOG_LEVEL_MEDIUM,"SMMHasNoS4BIOSReqCmd","FADT SMI S4BIOS_REQ command is ""zero, but this is not allowed ""when SMM is supported, and the ""S4BIOS_F flag is set in the ""FACS.");}} else {if (fadt->smi_cmd == 0) {fwts_passed(fw,"FADT indicates System Management ""Mode is not supported, but a ""S4BIOS_REQ command is defined.");fwts_advice(fw,"There is an inconsistency between ""the FADT and FACS. The FADT ""indicates no SMM support, but it ""defines an S4BIOS_REQ command, ""and the FACS indicates ""S4BIOS_REQ is supported. One ""of these may be incorrect.");} else {fwts_passed(fw,"FADT S4BIOS_REQ command is ""non-zero, SMM is supported so ""the command can be used, and ""the FACS indicates S4BIOS_REQ ""is supported.");}}- } else {
if (fadt->s4bios_req == 0)fwts_passed(fw, "FADT S4BIOS_REQ command is not set ""and FACS indicates it is not supported.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"SMMS4BIOSCmdDefined","FADT S4BIOS_REQ command is defined ""but FACS indicates it is not supported.");- }
- return;
+}
+static void acpi_table_check_fadt_pstate_cnt(fwts_framework *fw) +{
- if (fadt->pstate_cnt == 0) {
if (fadt->smi_cmd == 0)fwts_passed(fw,"FADT SMI PSTATE_CNT command is zero, ""which is allowed since SMM is not ""supported.");- } else {
if (fadt->smi_cmd == 0)fwts_failed(fw, LOG_LEVEL_MEDIUM,"SMMHasExtraPstateCntCmd","FADT SMI PSTATE_CNT command is ""non-zero, but SMM is not supported.");elsefwts_passed(fw, "FADT SMI PSTATE_CNT command is ""non-zero, and SMM is supported.");- }
- return;
+}
static void acpi_table_check_fadt_pm_tmr( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -914,6 +1029,8 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_smi_cmd(fw); acpi_table_check_fadt_acpi_enable(fw); acpi_table_check_fadt_acpi_disable(fw);
acpi_table_check_fadt_s4bios_req(fw);acpi_table_check_fadt_pstate_cnt(fw);
Yep, these are good extra sanity checks.
Acked-by: Colin Ian King colin.king@canonical.com
There are several address fields for power management in the FADT: PM1A_EVT_BLK PM1B_EVT_BLK PM1A_CNT_BLK PM1B_CNT_BLK PM2_CNT_BLK PM_TMR_BLK PM1_EVT_LEN PM1_CNT_LEN PM2_CNT_LEN PM_TMR_LEN
The tests that existed before only touched a few of these, so extend the tests so that now all of them are checked for proper values. Most of the tests are very similar, hence they are grouped together into this one patch.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 397 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 336 insertions(+), 61 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 3c4cdda..3b5547f 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -884,20 +884,336 @@ static void acpi_table_check_fadt_pstate_cnt(fwts_framework *fw) return; }
-static void acpi_table_check_fadt_pm_tmr( - fwts_framework *fw, - const fwts_acpi_table_fadt *fadt, - bool *passed) +static void acpi_table_check_fadt_pm1a_evt_blk(fwts_framework *fw) { - if (fwts_acpi_is_reduced_hardware(fadt)) { - if (fadt->pm_tmr_len != 0) - fwts_warning(fw, "FADT PM_TMR_LEN is not zero " - "but should be in reduced hardware mode."); + bool both_zero; + bool both_nonzero; + + if (fadt->pm1a_evt_blk == 0 && fadt->x_pm1a_evt_blk.address == 0) { + both_zero = true; + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1aEvtBlkNotSet", + "FADT PM1A_EVT_BLK is a required field and must " + "have either a 32-bit or 64-bit address set."); + } else { + both_zero = false; + fwts_passed(fw, + "FADT required PM1A_EVT_BLK field is non-zero"); + } + + if (fadt->pm1a_evt_blk != 0 && fadt->x_pm1a_evt_blk.address != 0) { + both_nonzero = true; + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1aEvtBlkBothtSet", + "FADT PM1A_EVT_BLK has both a 32-bit and a " + "64-bit address set; only one should be used."); + } else { + both_nonzero = false; + if (!both_zero) + fwts_passed(fw, + "FADT one required PM1A_EVT_BLK field " + "is non-zero"); + } + + if (both_nonzero && + ((uint64_t)fadt->pm1a_evt_blk == fadt->x_pm1a_evt_blk.address)) { + fwts_passed(fw, + "FADT 32- and 64-bit PM1A_EVT_BLK fields are " + "at least equal."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM1A_EVT_BLK " + "fields are being used, but only one should be " + "non-zero. However, they are at least equal so " + "the kernel will at least have a usable value."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1aEvtBlkNotSet", + "FADT PM1A_EVT_BLK is a required field and must " + "have either a 32-bit or 64-bit address set."); + if (!both_zero) + fwts_advice(fw, + "Both FADT 32- and 64-bit PM1A_EVT_BLK " + "fields are being used, but only one " + "should be non-zero. Since the fields " + "value are not equal the kernel cannot " + "unambiguously determine which value " + "is the correct one."); + } +} + +static void acpi_table_check_fadt_pm1b_evt_blk(fwts_framework *fw) +{ + if (fadt->pm1b_evt_blk == 0 && fadt->x_pm1b_evt_blk.address == 0) { + fwts_skipped(fw, "FADT PM1B_EVT_BLK not being used."); return; }
- if (fadt->pm_tmr_len != 4) { - *passed = false; + if ((fadt->pm1b_evt_blk != 0 && fadt->x_pm1b_evt_blk.address == 0) || + (fadt->pm1b_evt_blk == 0 && fadt->x_pm1b_evt_blk.address != 0)) + fwts_passed(fw, + "FADT only one of the 32-bit or 64-bit " + "PM1B_EVT_BLK fields is being used."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1bEvtBlkOnlyOneField", + "FADT PM1B_EVT_BLK field has both the 32-bit " + "and the 64-bit field set."); + + if ((uint64_t)fadt->pm1b_evt_blk == fadt->x_pm1b_evt_blk.address) { + fwts_passed(fw, + "FADT 32- and 64-bit PM1B_EVT_BLK fields are " + "at least equal."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM1B_EVT_BLK " + "fields are being used, but only one should be " + "non-zero. However, they are at least equal so " + "the kernel will at least have a usable value."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1bEvtBlkNotSet", + "FADT PM1A_EVT_BLK is a required field and must " + "have either a 32-bit or 64-bit address set."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM1B_EVT_BLK " + "fields are being used, but only one should be " + "non-zero. Since the fields value are not equal " + "the kernel cannot unambiguously determine which " + "value is the correct one."); + } +} + +static void acpi_table_check_fadt_pm1a_cnt_blk(fwts_framework *fw) +{ + if (fadt->pm1a_cnt_blk != 0 || fadt->x_pm1a_cnt_blk.address != 0) + fwts_passed(fw, + "FADT required PM1A_CNT_BLK field is non-zero"); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1aCntBlkNotSet", + "FADT PM1A_CNT_BLK is a required field and must " + "have either a 32-bit or 64-bit address set."); + + if ((fadt->pm1a_cnt_blk != 0 && fadt->x_pm1a_cnt_blk.address == 0) || + (fadt->pm1a_cnt_blk == 0 && fadt->x_pm1a_cnt_blk.address != 0)) + fwts_passed(fw, + "FADT only one of the 32-bit or 64-bit " + "PM1A_CNT_BLK fields is being used."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1aCntBlkOnlyOneField", + "FADT PM1A_CNT_BLK field has both the 32-bit " + "and the 64-bit field set."); + + if ((uint64_t)fadt->pm1a_cnt_blk == fadt->x_pm1a_cnt_blk.address) { + fwts_passed(fw, + "FADT 32- and 64-bit PM1A_CNT_BLK fields are " + "at least equal."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM1A_CNT_BLK " + "fields are being used, but only one should be " + "non-zero. However, they are at least equal so " + "the kernel will at least have a usable value."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1aCntBlkNotSet", + "FADT PM1A_CNT_BLK is a required field and must " + "have either a 32-bit or 64-bit address set."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM1A_CNT_BLK " + "fields are being used, but only one should be " + "non-zero. Since the fields value are not equal " + "the kernel cannot unambiguously determine which " + "value is the correct one."); + } +} + +static void acpi_table_check_fadt_pm1b_cnt_blk(fwts_framework *fw) +{ + if (fadt->pm1b_cnt_blk == 0 && fadt->x_pm1b_cnt_blk.address == 0) { + fwts_skipped(fw, "FADT PM1B_CNT_BLK not being used."); + return; + } + + if ((fadt->pm1b_cnt_blk != 0 && fadt->x_pm1b_cnt_blk.address == 0) || + (fadt->pm1b_cnt_blk == 0 && fadt->x_pm1b_cnt_blk.address != 0)) + fwts_passed(fw, + "FADT only one of the 32-bit or 64-bit " + "PM1B_CNT_BLK fields is being used."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1bCntBlkOnlyOneField", + "FADT PM1B_CNT_BLK field has both the 32-bit " + "and the 64-bit field set."); + + if ((uint64_t)fadt->pm1b_cnt_blk == fadt->x_pm1b_cnt_blk.address) { + fwts_passed(fw, + "FADT 32- and 64-bit PM1B_CNT_BLK fields are " + "at least equal."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM1B_CNT_BLK " + "fields are being used, but only one should be " + "non-zero. However, they are at least equal so " + "the kernel will at least have a usable value."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm1bCntBlkNotSet", + "FADT PM1A_CNT_BLK is a required field and must " + "have either a 32-bit or 64-bit address set."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM1B_CNT_BLK " + "fields are being used, but only one should be " + "non-zero. Since the fields value are not equal " + "the kernel cannot unambiguously determine which " + "value is the correct one."); + } +} + +static void acpi_table_check_fadt_pm2_cnt_blk(fwts_framework *fw) +{ + if (fadt->pm2_cnt_blk == 0 && fadt->x_pm2_cnt_blk.address == 0) { + fwts_skipped(fw, "FADT PM2_CNT_BLK not being used."); + return; + } + + if ((fadt->pm2_cnt_blk != 0 && fadt->x_pm2_cnt_blk.address == 0) || + (fadt->pm2_cnt_blk == 0 && fadt->x_pm2_cnt_blk.address != 0)) + fwts_passed(fw, + "FADT only one of the 32-bit or 64-bit " + "PM2_CNT_BLK fields is being used."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm2CntBlkOnlyOneField", + "FADT PM2_CNT_BLK field has both the 32-bit " + "and the 64-bit field set."); + + if ((uint64_t)fadt->pm2_cnt_blk == fadt->x_pm2_cnt_blk.address) { + fwts_passed(fw, + "FADT 32- and 64-bit PM2_CNT_BLK fields are " + "at least equal."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM2_CNT_BLK " + "fields are being used, but only one should be " + "non-zero. However, they are at least equal so " + "the kernel will at least have a usable value."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm2CntBlkNotSet", + "FADT PM2_CNT_BLK is a required field and must " + "have either a 32-bit or 64-bit address set."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM2_CNT_BLK " + "fields are being used, but only one should be " + "non-zero. Since the fields value are not equal " + "the kernel cannot unambiguously determine which " + "value is the correct one."); + } +} + +static void acpi_table_check_fadt_pm_tmr_blk(fwts_framework *fw) +{ + if (fadt->pm_tmr_blk == 0 && fadt->x_pm_tmr_blk.address == 0) { + fwts_skipped(fw, "FADT PM_TMR_BLK not being used."); + return; + } + + if ((fadt->pm_tmr_blk != 0 && fadt->x_pm_tmr_blk.address == 0) || + (fadt->pm_tmr_blk == 0 && fadt->x_pm_tmr_blk.address != 0)) + fwts_passed(fw, + "FADT only one of the 32-bit or 64-bit " + "PM_TMR_BLK fields is being used."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm2CntBlkOnlyOneField", + "FADT PM_TMR_BLK field has both the 32-bit " + "and the 64-bit field set."); + + if ((uint64_t)fadt->pm_tmr_blk == fadt->x_pm_tmr_blk.address) { + fwts_passed(fw, + "FADT 32- and 64-bit PM_TMR_BLK fields are " + "at least equal."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM_TMR_BLK " + "fields are being used, but only one should be " + "non-zero. However, they are at least equal so " + "the kernel will at least have a usable value."); + } else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm2CntBlkNotSet", + "FADT PM1A_CNT_BLK is a required field and must " + "have either a 32-bit or 64-bit address set."); + fwts_advice(fw, + "Both FADT 32- and 64-bit PM_TMR_BLK " + "fields are being used, but only one should be " + "non-zero. Since the fields value are not equal " + "the kernel cannot unambiguously determine which " + "value is the correct one."); + } +} + +static void acpi_table_check_fadt_pm1_evt_len(fwts_framework *fw) +{ + if (fadt->pm1_evt_len >= 4) + fwts_passed(fw, "FADT PM1_EVT_LEN >= 4."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTPm1EvtLenTooSmall", + "FADT PM1_EVT_LEN must be >= 4, but is %d.", + fadt->pm1_evt_len); + return; +} + +static void acpi_table_check_fadt_pm1_cnt_len(fwts_framework *fw) +{ + if (fadt->pm1_cnt_len >= 2) + fwts_passed(fw, "FADT PM1_CNT_LEN >= 2."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTPm1CntLenTooSmall", + "FADT PM1_CNT_LEN must be >= 2, but is %d.", + fadt->pm1_cnt_len); + return; +} + +static void acpi_table_check_fadt_pm2_cnt_len(fwts_framework *fw) +{ + if (fadt->pm2_cnt_blk == 0) { + if (fadt->pm2_cnt_len == 0) + fwts_passed(fw, "FADT PM2_CNT_LEN is zero and " + "PM2_CNT_BLK is not supported."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPm2CntLenInconsistent", + "FADT PM2_CNT_LEN must be zero because " + "PM2_CNT_BLK is not supported, but is %d.", + fadt->pm2_cnt_len); + return; + } + + if (fadt->pm2_cnt_len >= 1) + fwts_passed(fw, "FADT PM2_CNT_LEN >= 1."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTPm2CntLenTooSmall", + "FADT PM2_CNT_LEN must be >= 1, but is %d.", + fadt->pm2_cnt_len); + return; +} + +static void acpi_table_check_fadt_pm_tmr_len(fwts_framework *fw) +{ + if (fadt->pm_tmr_len == 0) { + if (fadt->pm_tmr_blk == 0) + fwts_passed(fw, "FADT PM_TMR_LEN is zero and " + "PM_TMR_BLK is not supported."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTPmTmrLenInconsistent", + "FADT PM_TMR_LEN must be zero because " + "PM_TMR_BLK is not supported, but is %d.", + fadt->pm_tmr_len); + return; + } + + if (fadt->pm_tmr_len == 4) + fwts_passed(fw, "FADT PM_TMR_LEN is 4."); + else { fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadPMTMRLEN", "FADT PM_TMR_LEN is %" PRIu8 @@ -958,55 +1274,6 @@ static void acpi_table_check_fadt_gpe( } }
-static void acpi_table_check_fadt_addr( - fwts_framework *fw, - const char *name, - const uint32_t addr32, - const fwts_acpi_gas *addr64, - bool *passed) -{ - /* Don't compare if addresses are zero */ - if ((addr32 == 0) || (addr64->address == 0)) - return; - if (addr32 == addr64->address) - return; - - *passed = false; - /* - * Since this can cause systems to misbehave - * if the kernel uses the incorrect address we - * make this LOG_LEVEL_HIGH - */ - fwts_failed(fw, LOG_LEVEL_HIGH, - "FADTPmAddr32Addr64Different", - "FADT %s (32 bit address) 0x%" PRIx32 " is different from " - "X_%s (64 bit address) 0x%" PRIx64 ".", - name, addr32, name, addr64->address); -} - -static void acpi_table_check_fadt_pm_addr( - fwts_framework *fw, - const fwts_acpi_table_fadt *fadt, - bool *passed) -{ - if (fadt->header.length < 148) { - /* No 64 bit PM addresses to sanity check */ - return; - } - acpi_table_check_fadt_addr(fw, "PM1a_EVT_BLK", fadt->pm1a_evt_blk, - &fadt->x_pm1a_evt_blk, passed); - acpi_table_check_fadt_addr(fw, "PM1b_EVT_BLK", fadt->pm1b_evt_blk, - &fadt->x_pm1b_evt_blk, passed); - acpi_table_check_fadt_addr(fw, "PM1a_CNT_BLK", fadt->pm1a_cnt_blk, - &fadt->x_pm1a_cnt_blk, passed); - acpi_table_check_fadt_addr(fw, "PM1b_CNT_BLK", fadt->pm1b_cnt_blk, - &fadt->x_pm1b_cnt_blk, passed); - acpi_table_check_fadt_addr(fw, "PM2_CNT_BLK", fadt->pm2_cnt_blk, - &fadt->x_pm2_cnt_blk, passed); - acpi_table_check_fadt_addr(fw, "PM_TMR_BLK", fadt->pm_tmr_blk, - &fadt->x_pm_tmr_blk, passed); -} - static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1031,9 +1298,17 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_acpi_disable(fw); acpi_table_check_fadt_s4bios_req(fw); acpi_table_check_fadt_pstate_cnt(fw); - acpi_table_check_fadt_pm_tmr(fw, fadt, &passed); + acpi_table_check_fadt_pm1a_evt_blk(fw); + acpi_table_check_fadt_pm1b_evt_blk(fw); + acpi_table_check_fadt_pm1a_cnt_blk(fw); + acpi_table_check_fadt_pm1b_cnt_blk(fw); + acpi_table_check_fadt_pm2_cnt_blk(fw); + acpi_table_check_fadt_pm_tmr_blk(fw); + acpi_table_check_fadt_pm1_evt_len(fw); + acpi_table_check_fadt_pm1_cnt_len(fw); + acpi_table_check_fadt_pm2_cnt_len(fw); + acpi_table_check_fadt_pm_tmr_len(fw); acpi_table_check_fadt_gpe(fw, fadt, &passed); - acpi_table_check_fadt_pm_addr(fw, fadt, &passed); fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base);
fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16,
On 09/02/16 01:32, Al Stone wrote:
There are several address fields for power management in the FADT: PM1A_EVT_BLK PM1B_EVT_BLK PM1A_CNT_BLK PM1B_CNT_BLK PM2_CNT_BLK PM_TMR_BLK PM1_EVT_LEN PM1_CNT_LEN PM2_CNT_LEN PM_TMR_LEN
The tests that existed before only touched a few of these, so extend the tests so that now all of them are checked for proper values. Most of the tests are very similar, hence they are grouped together into this one patch.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 397 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 336 insertions(+), 61 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 3c4cdda..3b5547f 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -884,20 +884,336 @@ static void acpi_table_check_fadt_pstate_cnt(fwts_framework *fw) return; } -static void acpi_table_check_fadt_pm_tmr(
- fwts_framework *fw,
- const fwts_acpi_table_fadt *fadt,
- bool *passed)
+static void acpi_table_check_fadt_pm1a_evt_blk(fwts_framework *fw) {
- if (fwts_acpi_is_reduced_hardware(fadt)) {
if (fadt->pm_tmr_len != 0)fwts_warning(fw, "FADT PM_TMR_LEN is not zero ""but should be in reduced hardware mode.");
- bool both_zero;
- bool both_nonzero;
- if (fadt->pm1a_evt_blk == 0 && fadt->x_pm1a_evt_blk.address == 0) {
both_zero = true;fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1aEvtBlkNotSet","FADT PM1A_EVT_BLK is a required field and must ""have either a 32-bit or 64-bit address set.");- } else {
both_zero = false;fwts_passed(fw,"FADT required PM1A_EVT_BLK field is non-zero");- }
- if (fadt->pm1a_evt_blk != 0 && fadt->x_pm1a_evt_blk.address != 0) {
both_nonzero = true;fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1aEvtBlkBothtSet","FADT PM1A_EVT_BLK has both a 32-bit and a ""64-bit address set; only one should be used.");- } else {
both_nonzero = false;if (!both_zero)fwts_passed(fw,"FADT one required PM1A_EVT_BLK field ""is non-zero");- }
- if (both_nonzero &&
((uint64_t)fadt->pm1a_evt_blk == fadt->x_pm1a_evt_blk.address)) {fwts_passed(fw,"FADT 32- and 64-bit PM1A_EVT_BLK fields are ""at least equal.");fwts_advice(fw,"Both FADT 32- and 64-bit PM1A_EVT_BLK ""fields are being used, but only one should be ""non-zero. However, they are at least equal so ""the kernel will at least have a usable value.");- } else {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1aEvtBlkNotSet","FADT PM1A_EVT_BLK is a required field and must ""have either a 32-bit or 64-bit address set.");if (!both_zero)fwts_advice(fw,"Both FADT 32- and 64-bit PM1A_EVT_BLK ""fields are being used, but only one ""should be non-zero. Since the fields ""value are not equal the kernel cannot ""unambiguously determine which value ""is the correct one.");- }
+}
+static void acpi_table_check_fadt_pm1b_evt_blk(fwts_framework *fw) +{
- if (fadt->pm1b_evt_blk == 0 && fadt->x_pm1b_evt_blk.address == 0) {
fwts_skipped(fw, "FADT PM1B_EVT_BLK not being used.");
- if (fadt->pm_tmr_len != 4) {
*passed = false;
- if ((fadt->pm1b_evt_blk != 0 && fadt->x_pm1b_evt_blk.address == 0) ||
(fadt->pm1b_evt_blk == 0 && fadt->x_pm1b_evt_blk.address != 0))fwts_passed(fw,"FADT only one of the 32-bit or 64-bit ""PM1B_EVT_BLK fields is being used.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1bEvtBlkOnlyOneField","FADT PM1B_EVT_BLK field has both the 32-bit ""and the 64-bit field set.");- if ((uint64_t)fadt->pm1b_evt_blk == fadt->x_pm1b_evt_blk.address) {
fwts_passed(fw,"FADT 32- and 64-bit PM1B_EVT_BLK fields are ""at least equal.");fwts_advice(fw,"Both FADT 32- and 64-bit PM1B_EVT_BLK ""fields are being used, but only one should be ""non-zero. However, they are at least equal so ""the kernel will at least have a usable value.");- } else {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1bEvtBlkNotSet","FADT PM1A_EVT_BLK is a required field and must ""have either a 32-bit or 64-bit address set.");fwts_advice(fw,"Both FADT 32- and 64-bit PM1B_EVT_BLK ""fields are being used, but only one should be ""non-zero. Since the fields value are not equal ""the kernel cannot unambiguously determine which ""value is the correct one.");- }
+}
+static void acpi_table_check_fadt_pm1a_cnt_blk(fwts_framework *fw) +{
- if (fadt->pm1a_cnt_blk != 0 || fadt->x_pm1a_cnt_blk.address != 0)
fwts_passed(fw,"FADT required PM1A_CNT_BLK field is non-zero");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1aCntBlkNotSet","FADT PM1A_CNT_BLK is a required field and must ""have either a 32-bit or 64-bit address set.");- if ((fadt->pm1a_cnt_blk != 0 && fadt->x_pm1a_cnt_blk.address == 0) ||
(fadt->pm1a_cnt_blk == 0 && fadt->x_pm1a_cnt_blk.address != 0))fwts_passed(fw,"FADT only one of the 32-bit or 64-bit ""PM1A_CNT_BLK fields is being used.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1aCntBlkOnlyOneField","FADT PM1A_CNT_BLK field has both the 32-bit ""and the 64-bit field set.");- if ((uint64_t)fadt->pm1a_cnt_blk == fadt->x_pm1a_cnt_blk.address) {
fwts_passed(fw,"FADT 32- and 64-bit PM1A_CNT_BLK fields are ""at least equal.");fwts_advice(fw,"Both FADT 32- and 64-bit PM1A_CNT_BLK ""fields are being used, but only one should be ""non-zero. However, they are at least equal so ""the kernel will at least have a usable value.");- } else {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1aCntBlkNotSet","FADT PM1A_CNT_BLK is a required field and must ""have either a 32-bit or 64-bit address set.");fwts_advice(fw,"Both FADT 32- and 64-bit PM1A_CNT_BLK ""fields are being used, but only one should be ""non-zero. Since the fields value are not equal ""the kernel cannot unambiguously determine which ""value is the correct one.");- }
+}
+static void acpi_table_check_fadt_pm1b_cnt_blk(fwts_framework *fw) +{
- if (fadt->pm1b_cnt_blk == 0 && fadt->x_pm1b_cnt_blk.address == 0) {
fwts_skipped(fw, "FADT PM1B_CNT_BLK not being used.");return;- }
- if ((fadt->pm1b_cnt_blk != 0 && fadt->x_pm1b_cnt_blk.address == 0) ||
(fadt->pm1b_cnt_blk == 0 && fadt->x_pm1b_cnt_blk.address != 0))fwts_passed(fw,"FADT only one of the 32-bit or 64-bit ""PM1B_CNT_BLK fields is being used.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1bCntBlkOnlyOneField","FADT PM1B_CNT_BLK field has both the 32-bit ""and the 64-bit field set.");- if ((uint64_t)fadt->pm1b_cnt_blk == fadt->x_pm1b_cnt_blk.address) {
fwts_passed(fw,"FADT 32- and 64-bit PM1B_CNT_BLK fields are ""at least equal.");fwts_advice(fw,"Both FADT 32- and 64-bit PM1B_CNT_BLK ""fields are being used, but only one should be ""non-zero. However, they are at least equal so ""the kernel will at least have a usable value.");- } else {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm1bCntBlkNotSet","FADT PM1A_CNT_BLK is a required field and must ""have either a 32-bit or 64-bit address set.");fwts_advice(fw,"Both FADT 32- and 64-bit PM1B_CNT_BLK ""fields are being used, but only one should be ""non-zero. Since the fields value are not equal ""the kernel cannot unambiguously determine which ""value is the correct one.");- }
+}
+static void acpi_table_check_fadt_pm2_cnt_blk(fwts_framework *fw) +{
- if (fadt->pm2_cnt_blk == 0 && fadt->x_pm2_cnt_blk.address == 0) {
fwts_skipped(fw, "FADT PM2_CNT_BLK not being used.");return;- }
- if ((fadt->pm2_cnt_blk != 0 && fadt->x_pm2_cnt_blk.address == 0) ||
(fadt->pm2_cnt_blk == 0 && fadt->x_pm2_cnt_blk.address != 0))fwts_passed(fw,"FADT only one of the 32-bit or 64-bit ""PM2_CNT_BLK fields is being used.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm2CntBlkOnlyOneField","FADT PM2_CNT_BLK field has both the 32-bit ""and the 64-bit field set.");- if ((uint64_t)fadt->pm2_cnt_blk == fadt->x_pm2_cnt_blk.address) {
fwts_passed(fw,"FADT 32- and 64-bit PM2_CNT_BLK fields are ""at least equal.");fwts_advice(fw,"Both FADT 32- and 64-bit PM2_CNT_BLK ""fields are being used, but only one should be ""non-zero. However, they are at least equal so ""the kernel will at least have a usable value.");- } else {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm2CntBlkNotSet","FADT PM2_CNT_BLK is a required field and must ""have either a 32-bit or 64-bit address set.");fwts_advice(fw,"Both FADT 32- and 64-bit PM2_CNT_BLK ""fields are being used, but only one should be ""non-zero. Since the fields value are not equal ""the kernel cannot unambiguously determine which ""value is the correct one.");- }
+}
+static void acpi_table_check_fadt_pm_tmr_blk(fwts_framework *fw) +{
- if (fadt->pm_tmr_blk == 0 && fadt->x_pm_tmr_blk.address == 0) {
fwts_skipped(fw, "FADT PM_TMR_BLK not being used.");return;- }
- if ((fadt->pm_tmr_blk != 0 && fadt->x_pm_tmr_blk.address == 0) ||
(fadt->pm_tmr_blk == 0 && fadt->x_pm_tmr_blk.address != 0))fwts_passed(fw,"FADT only one of the 32-bit or 64-bit ""PM_TMR_BLK fields is being used.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm2CntBlkOnlyOneField","FADT PM_TMR_BLK field has both the 32-bit ""and the 64-bit field set.");- if ((uint64_t)fadt->pm_tmr_blk == fadt->x_pm_tmr_blk.address) {
fwts_passed(fw,"FADT 32- and 64-bit PM_TMR_BLK fields are ""at least equal.");fwts_advice(fw,"Both FADT 32- and 64-bit PM_TMR_BLK ""fields are being used, but only one should be ""non-zero. However, they are at least equal so ""the kernel will at least have a usable value.");- } else {
fwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm2CntBlkNotSet","FADT PM1A_CNT_BLK is a required field and must ""have either a 32-bit or 64-bit address set.");fwts_advice(fw,"Both FADT 32- and 64-bit PM_TMR_BLK ""fields are being used, but only one should be ""non-zero. Since the fields value are not equal ""the kernel cannot unambiguously determine which ""value is the correct one.");- }
+}
+static void acpi_table_check_fadt_pm1_evt_len(fwts_framework *fw) +{
- if (fadt->pm1_evt_len >= 4)
fwts_passed(fw, "FADT PM1_EVT_LEN >= 4.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTPm1EvtLenTooSmall","FADT PM1_EVT_LEN must be >= 4, but is %d.",fadt->pm1_evt_len);- return;
+}
+static void acpi_table_check_fadt_pm1_cnt_len(fwts_framework *fw) +{
- if (fadt->pm1_cnt_len >= 2)
fwts_passed(fw, "FADT PM1_CNT_LEN >= 2.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTPm1CntLenTooSmall","FADT PM1_CNT_LEN must be >= 2, but is %d.",fadt->pm1_cnt_len);- return;
+}
+static void acpi_table_check_fadt_pm2_cnt_len(fwts_framework *fw) +{
- if (fadt->pm2_cnt_blk == 0) {
if (fadt->pm2_cnt_len == 0)fwts_passed(fw, "FADT PM2_CNT_LEN is zero and ""PM2_CNT_BLK is not supported.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPm2CntLenInconsistent","FADT PM2_CNT_LEN must be zero because ""PM2_CNT_BLK is not supported, but is %d.",fadt->pm2_cnt_len);return;- }
- if (fadt->pm2_cnt_len >= 1)
fwts_passed(fw, "FADT PM2_CNT_LEN >= 1.");- else
fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTPm2CntLenTooSmall","FADT PM2_CNT_LEN must be >= 1, but is %d.",fadt->pm2_cnt_len);- return;
+}
+static void acpi_table_check_fadt_pm_tmr_len(fwts_framework *fw) +{
- if (fadt->pm_tmr_len == 0) {
if (fadt->pm_tmr_blk == 0)fwts_passed(fw, "FADT PM_TMR_LEN is zero and ""PM_TMR_BLK is not supported.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTPmTmrLenInconsistent","FADT PM_TMR_LEN must be zero because ""PM_TMR_BLK is not supported, but is %d.",fadt->pm_tmr_len);return;- }
- if (fadt->pm_tmr_len == 4)
fwts_passed(fw, "FADT PM_TMR_LEN is 4.");- else { fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadPMTMRLEN", "FADT PM_TMR_LEN is %" PRIu8
@@ -958,55 +1274,6 @@ static void acpi_table_check_fadt_gpe( } } -static void acpi_table_check_fadt_addr(
- fwts_framework *fw,
- const char *name,
- const uint32_t addr32,
- const fwts_acpi_gas *addr64,
- bool *passed)
-{
- /* Don't compare if addresses are zero */
- if ((addr32 == 0) || (addr64->address == 0))
return;- if (addr32 == addr64->address)
return;- *passed = false;
- /*
* Since this can cause systems to misbehave* if the kernel uses the incorrect address we* make this LOG_LEVEL_HIGH*/- fwts_failed(fw, LOG_LEVEL_HIGH,
"FADTPmAddr32Addr64Different","FADT %s (32 bit address) 0x%" PRIx32 " is different from ""X_%s (64 bit address) 0x%" PRIx64 ".",name, addr32, name, addr64->address);-}
-static void acpi_table_check_fadt_pm_addr(
- fwts_framework *fw,
- const fwts_acpi_table_fadt *fadt,
- bool *passed)
-{
- if (fadt->header.length < 148) {
/* No 64 bit PM addresses to sanity check */return;- }
- acpi_table_check_fadt_addr(fw, "PM1a_EVT_BLK", fadt->pm1a_evt_blk,
&fadt->x_pm1a_evt_blk, passed);- acpi_table_check_fadt_addr(fw, "PM1b_EVT_BLK", fadt->pm1b_evt_blk,
&fadt->x_pm1b_evt_blk, passed);- acpi_table_check_fadt_addr(fw, "PM1a_CNT_BLK", fadt->pm1a_cnt_blk,
&fadt->x_pm1a_cnt_blk, passed);- acpi_table_check_fadt_addr(fw, "PM1b_CNT_BLK", fadt->pm1b_cnt_blk,
&fadt->x_pm1b_cnt_blk, passed);- acpi_table_check_fadt_addr(fw, "PM2_CNT_BLK", fadt->pm2_cnt_blk,
&fadt->x_pm2_cnt_blk, passed);- acpi_table_check_fadt_addr(fw, "PM_TMR_BLK", fadt->pm_tmr_blk,
&fadt->x_pm_tmr_blk, passed);-}
static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1031,9 +1298,17 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_acpi_disable(fw); acpi_table_check_fadt_s4bios_req(fw); acpi_table_check_fadt_pstate_cnt(fw);
acpi_table_check_fadt_pm_tmr(fw, fadt, &passed);
acpi_table_check_fadt_pm1a_evt_blk(fw);acpi_table_check_fadt_pm1b_evt_blk(fw);acpi_table_check_fadt_pm1a_cnt_blk(fw);acpi_table_check_fadt_pm1b_cnt_blk(fw);acpi_table_check_fadt_pm2_cnt_blk(fw);acpi_table_check_fadt_pm_tmr_blk(fw);acpi_table_check_fadt_pm1_evt_len(fw);acpi_table_check_fadt_pm1_cnt_len(fw);acpi_table_check_fadt_pm2_cnt_len(fw);acpi_table_check_fadt_pm_tmr_len(fw);
acpi_table_check_fadt_pm_addr(fw, fadt, &passed);fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16,
Acked-by: Colin Ian King colin.king@canonical.com
Other parts of the FADT tests check for proper operation of some of the address fields, like the PM or GPE blocks. The original GPE test was rewritten here to check the fields in more detail.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 97 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 37 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 3b5547f..b349954 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1227,50 +1227,72 @@ static void acpi_table_check_fadt_pm_tmr_len(fwts_framework *fw) } }
-static void acpi_table_check_fadt_gpe( - fwts_framework *fw, - const fwts_acpi_table_fadt *fadt, - bool *passed) +static void acpi_table_check_fadt_gpe0_blk_len(fwts_framework *fw) { - if (fwts_acpi_is_reduced_hardware(fadt)) { - if (fadt->gpe0_blk_len != 0) { - fwts_warning(fw, "FADT GPE0_BLK_LEN is not zero " - "but should be in reduced hardware mode."); - } - if (fadt->gpe1_blk_len != 0) { - fwts_warning(fw, "FADT GPE1_BLK_LEN is not zero but " - "should be in reduced hardware mode."); - } - return; - } - if (fadt->gpe0_blk_len & 1) { - *passed = false; fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FADTBadGPEBLKLEN", - "FADT GPE0_BLK_LEN is %" PRIu8 - ", should a multiple of 2.", - fadt->gpe0_blk_len); + "FADTBadGPE0BLKLEN", + "FADT GPE0_BLK_LEN is %" PRIu8 + ", should a multiple of 2.", + fadt->gpe0_blk_len); fwts_advice(fw, - "The FADT GPE_BLK_LEN should be a multiple of 2. " - "Because it isn't, the ACPI driver will not map in " - "the GPE0 region. This could mean that General " - "Purpose Events will not function correctly (for " - "example lid or ac-power events)."); + "The FADT GPE0_BLK_LEN should be a multiple of 2. " + "Because it isn't, the ACPI driver will not map in " + "the GPE0 region. This could mean that General " + "Purpose Events will not function correctly (for " + "example lid or AC-power events)."); + } else { + if (fadt->gpe0_blk_len) + fwts_passed(fw, "FADT GPE0_BLK_LEN non-zero and a " + "non-negative multiple of 2: %" PRIu8 ".", + fadt->gpe0_blk_len); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTZeroGPE0BlkLen", + "FADT GPE0_BLK_LEN is zero, but must be " + "set to a non-negative multiple of 2."); + + } +} + +static void acpi_table_check_fadt_gpe1_blk_len(fwts_framework *fw) +{ + if (fadt->gpe1_blk_len == 0) { + if (fadt->gpe1_blk == 0) + fwts_passed(fw, "FADT GPE1_BLK_LEN is zero and " + "GPE1_BLK is not supported."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTGPE1BlkLenInconsistent", + "FADT GPE1_BLK_LEN must be zero because " + "GPE1_BLK is not supported, but is %d.", + fadt->gpe1_blk_len); + return; } + if (fadt->gpe1_blk_len & 1) { - *passed = false; fwts_failed(fw, LOG_LEVEL_MEDIUM, - "FADTBadGPE1BLKLEN", - "FADT GPE1_BLK_LEN is %" PRIu8 - ", should a multiple of 2.", - fadt->gpe1_blk_len); + "FADTBadGPE1BLKLEN", + "FADT GPE1_BLK_LEN is %" PRIu8 + ", should a multiple of 2.", + fadt->gpe1_blk_len); fwts_advice(fw, - "The FADT GPE1_BLK_LEN should be a multiple of 2. " - "Because it isn't, the ACPI driver will not map in " - "the GPE1 region. This could mean that General " - "Purpose Events will not function correctly (for " - "example lid or ac-power events)."); + "The FADT GPE1_BLK_LEN should be a multiple of 2. " + "Because it isn't, the ACPI driver will not map in " + "the GPE1 region. This could mean that General " + "Purpose Events will not function correctly (for " + "example lid or AC-power events)."); + } else { + if (fadt->gpe1_blk_len) + fwts_passed(fw, "FADT GPE1_BLK_LEN non-zero and a " + "non-negative multiple of 2: %" PRIu8 ".", + fadt->gpe1_blk_len); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "FADTZeroGPE1BlkLen", + "FADT GPE1_BLK_LEN is zero, but must be " + "set to a non-negative multiple of 2."); + } }
@@ -1308,7 +1330,8 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_pm1_cnt_len(fw); acpi_table_check_fadt_pm2_cnt_len(fw); acpi_table_check_fadt_pm_tmr_len(fw); - acpi_table_check_fadt_gpe(fw, fadt, &passed); + acpi_table_check_fadt_gpe0_blk_len(fw); + acpi_table_check_fadt_gpe1_blk_len(fw); fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base);
fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16,
On 09/02/16 01:32, Al Stone wrote:
Other parts of the FADT tests check for proper operation of some of the address fields, like the PM or GPE blocks. The original GPE test was rewritten here to check the fields in more detail.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 97 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 37 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 3b5547f..b349954 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1227,50 +1227,72 @@ static void acpi_table_check_fadt_pm_tmr_len(fwts_framework *fw) } } -static void acpi_table_check_fadt_gpe(
- fwts_framework *fw,
- const fwts_acpi_table_fadt *fadt,
- bool *passed)
+static void acpi_table_check_fadt_gpe0_blk_len(fwts_framework *fw) {
- if (fwts_acpi_is_reduced_hardware(fadt)) {
if (fadt->gpe0_blk_len != 0) {fwts_warning(fw, "FADT GPE0_BLK_LEN is not zero ""but should be in reduced hardware mode.");}if (fadt->gpe1_blk_len != 0) {fwts_warning(fw, "FADT GPE1_BLK_LEN is not zero but ""should be in reduced hardware mode.");}return;- }
- if (fadt->gpe0_blk_len & 1) {
*passed = false;"FADTBadGPEBLKLEN","FADT GPE0_BLK_LEN is %" PRIu8", should a multiple of 2.",fadt->gpe0_blk_len);
"FADTBadGPE0BLKLEN","FADT GPE0_BLK_LEN is %" PRIu8", should a multiple of 2.",fadt->gpe0_blk_len);
"The FADT GPE_BLK_LEN should be a multiple of 2. ""Because it isn't, the ACPI driver will not map in ""the GPE0 region. This could mean that General ""Purpose Events will not function correctly (for ""example lid or ac-power events).");
"The FADT GPE0_BLK_LEN should be a multiple of 2. ""Because it isn't, the ACPI driver will not map in ""the GPE0 region. This could mean that General ""Purpose Events will not function correctly (for ""example lid or AC-power events).");- } else {
if (fadt->gpe0_blk_len)fwts_passed(fw, "FADT GPE0_BLK_LEN non-zero and a ""non-negative multiple of 2: %" PRIu8 ".",fadt->gpe0_blk_len);elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTZeroGPE0BlkLen","FADT GPE0_BLK_LEN is zero, but must be ""set to a non-negative multiple of 2.");- }
+}
+static void acpi_table_check_fadt_gpe1_blk_len(fwts_framework *fw) +{
- if (fadt->gpe1_blk_len == 0) {
if (fadt->gpe1_blk == 0)fwts_passed(fw, "FADT GPE1_BLK_LEN is zero and ""GPE1_BLK is not supported.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTGPE1BlkLenInconsistent","FADT GPE1_BLK_LEN must be zero because ""GPE1_BLK is not supported, but is %d.",fadt->gpe1_blk_len);return;- if (fadt->gpe1_blk_len & 1) {
*passed = false;"FADTBadGPE1BLKLEN","FADT GPE1_BLK_LEN is %" PRIu8", should a multiple of 2.",fadt->gpe1_blk_len);
"FADTBadGPE1BLKLEN","FADT GPE1_BLK_LEN is %" PRIu8", should a multiple of 2.",fadt->gpe1_blk_len);
"The FADT GPE1_BLK_LEN should be a multiple of 2. ""Because it isn't, the ACPI driver will not map in ""the GPE1 region. This could mean that General ""Purpose Events will not function correctly (for ""example lid or ac-power events).");
"The FADT GPE1_BLK_LEN should be a multiple of 2. ""Because it isn't, the ACPI driver will not map in ""the GPE1 region. This could mean that General ""Purpose Events will not function correctly (for ""example lid or AC-power events).");- } else {
if (fadt->gpe1_blk_len)fwts_passed(fw, "FADT GPE1_BLK_LEN non-zero and a ""non-negative multiple of 2: %" PRIu8 ".",fadt->gpe1_blk_len);elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"FADTZeroGPE1BlkLen","FADT GPE1_BLK_LEN is zero, but must be ""set to a non-negative multiple of 2.");- }
} @@ -1308,7 +1330,8 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_pm1_cnt_len(fw); acpi_table_check_fadt_pm2_cnt_len(fw); acpi_table_check_fadt_pm_tmr_len(fw);
acpi_table_check_fadt_gpe(fw, fadt, &passed);
acpi_table_check_fadt_gpe0_blk_len(fw);acpi_table_check_fadt_gpe1_blk_len(fw);fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16,
Acked-by: Colin Ian King colin.king@canonical.com
Add in a new compliance test to check the C-State control (CST_CNT) field for the proper values.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index b349954..f932dea 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1296,6 +1296,28 @@ static void acpi_table_check_fadt_gpe1_blk_len(fwts_framework *fw) } }
+static void acpi_table_check_fadt_cst_cnt(fwts_framework *fw) +{ + if (fadt->cst_cnt == 0) { + if (fadt->smi_cmd == 0) + fwts_passed(fw, + "FADT SMI CST_CNT command is zero, " + "which is allowed since SMM is not " + "supported."); + } else { + if (fadt->smi_cmd == 0) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SMMHasExtraCSTCntCmd", + "FADT SMI CST_CNT command is " + "non-zero, but SMM is not supported."); + else + fwts_passed(fw, "FADT SMI CST_CNT command is " + "non-zero, and SMM is supported."); + } + + return; +} + static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1333,6 +1355,7 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_gpe0_blk_len(fw); acpi_table_check_fadt_gpe1_blk_len(fw); fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base); + acpi_table_check_fadt_cst_cnt(fw);
fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16, fadt->flush_size);
On 09/02/16 01:33, Al Stone wrote:
Add in a new compliance test to check the C-State control (CST_CNT) field for the proper values.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index b349954..f932dea 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1296,6 +1296,28 @@ static void acpi_table_check_fadt_gpe1_blk_len(fwts_framework *fw) } } +static void acpi_table_check_fadt_cst_cnt(fwts_framework *fw) +{
- if (fadt->cst_cnt == 0) {
if (fadt->smi_cmd == 0)fwts_passed(fw,"FADT SMI CST_CNT command is zero, ""which is allowed since SMM is not ""supported.");- } else {
if (fadt->smi_cmd == 0)fwts_failed(fw, LOG_LEVEL_MEDIUM,"SMMHasExtraCSTCntCmd","FADT SMI CST_CNT command is ""non-zero, but SMM is not supported.");elsefwts_passed(fw, "FADT SMI CST_CNT command is ""non-zero, and SMM is supported.");- }
- return;
+}
static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1333,6 +1355,7 @@ static int fadt_test1(fwts_framework *fw) acpi_table_check_fadt_gpe0_blk_len(fw); acpi_table_check_fadt_gpe1_blk_len(fw); fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base);
acpi_table_check_fadt_cst_cnt(fw);fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16, fadt->flush_size);
Acked-by: Colin Ian King colin.king@canonical.com
Add in new compliance checks for the P_LVL2_LAT and P_LVL3_LAT fields. Both of these require knowing the value of an x86 P_BLK; this in turn requires examination of any Processor() declarations in the ACPI name space, which in turn requires the initialization of the ACPI interpreter.
It is probable that these fields are seldom used; processor speeds and feeds are typically controlled via very different mechanisms these days. These tests are therefore for completeness sake and it may be difficult to find ACPI tables using these fields.
Note that these tests allow us to remove commented out versions of simpler tests of these fields.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 173 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 148 insertions(+), 25 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index f932dea..a2ed70c 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -19,6 +19,7 @@ * */ #include "fwts.h" +#include "fwts_acpi_object_eval.h"
#include <stdio.h> #include <stdlib.h> @@ -1318,6 +1319,139 @@ static void acpi_table_check_fadt_cst_cnt(fwts_framework *fw) return; }
+static uint64_t fadt_find_p_blk(fwts_framework *fw) +{ + uint64_t pblk; + fwts_list *objects; + + pblk = 0; + objects = fwts_acpi_object_get_names(); + if (objects) { + fwts_list_link *obj; + + fwts_list_foreach(obj, objects) { + char *name = fwts_list_data(char*, obj); + ACPI_OBJECT pr = { 0 }; + ACPI_BUFFER buf = { sizeof(ACPI_OBJECT), &pr }; + ACPI_HANDLE handle; + ACPI_OBJECT_TYPE type; + ACPI_STATUS status; + + status = AcpiGetHandle(NULL, name, &handle); + if (ACPI_FAILURE(status)) { + fwts_warning(fw, "Failed to get handle for " + "object %s.", name); + continue; + } + status = AcpiGetType(handle, &type); + if (ACPI_FAILURE(status)) { + fwts_warning(fw, "Failed to get type for " + "object %s.", name); + continue; + } + + /* + * If a CPU is not defined as a Processor object, + * we don't care here. Per section 8.1.2 and 8.1.3, + * defining a CPU with a Device object implies that + * a _CST method must be defined, and whatever is + * in the _CST overrides the P_BLK and P_LVL*_LAT + * values. Since we're only trying to validate the + * P_LVL*_LAT values, and they're only used if the + * CPUs are defined as Processor objects, we can + * ignore any CPUs defined as Device objects. + */ + if (type == ACPI_TYPE_PROCESSOR) { + fwts_log_info(fw, "Found processor %s.", name); + status = AcpiEvaluateObject(handle, NULL, NULL, + &buf); + if (ACPI_FAILURE(status)) + fwts_warning(fw, "Could not evaluate " + "Processor %s", name); + else { + if (pr.Processor.PblkAddress) + pblk = pr.Processor.PblkAddress; + } + } + } + } + + fwts_log_info(fw, "Using P_BLK address of 0x%" PRIx64, pblk); + return pblk; +} + +static void acpi_table_check_fadt_p_lvl2_lat(fwts_framework *fw, uint64_t pblk) +{ + if (pblk) { + if (fadt->p_lvl2_lat <= 100) + fwts_passed(fw, + "FADT P_LVL2_LAT is within proper range " + "at %" PRIu16, fadt->p_lvl2_lat); + else + fwts_warning(fw, + "FADT P_LVL2_LAT is > 100 (%" PRIu16 ") " + "but a P_BLK is defined. This implies " + "a C2 state is not supported, but there " + "is a P_BLK register block defined which " + "implies there might be a C2 state that " + "works. There is not enough information " + "to determine if this is expected or not.", + fadt->p_lvl2_lat); + } else { + if (fadt->p_lvl2_lat <= 100) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "PLvl2LatDefinedButNotUsable", + "FADT P_LVL2_LAT is <= 100 (%" PRIu16 ") " + "which implies a C2 state is supported " + "but there is no P_BLK register block " + "defined to enable the C2 transition.", + fadt->p_lvl2_lat); + else + fwts_passed(fw, + "FADT P_LVL2_LAT is > 100 (%" PRIu16 ") " + "and no P_BLK is defined.", + fadt->p_lvl2_lat); + } + + return; +} + +static void acpi_table_check_fadt_p_lvl3_lat(fwts_framework *fw, uint64_t pblk) +{ + if (pblk) { + if (fadt->p_lvl3_lat <= 1000) + fwts_passed(fw, + "FADT P_LVL3_LAT is within proper range " + "at %" PRIu16, fadt->p_lvl3_lat); + else + fwts_warning(fw, + "FADT P_LVL3_LAT is > 1000 (%" PRIu16 ") " + "but a P_BLK is defined. This implies " + "a C3 state is not supported, but there " + "is a P_BLK register block defined which " + "implies there might be a C3 state that " + "works. There is not enough information " + "to determine if this is expected or not.", + fadt->p_lvl3_lat); + } else { + if (fadt->p_lvl3_lat <= 1000) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "PLvl3LatDefinedButNotUsable", + "FADT P_LVL3_LAT is <= 1000 (%" PRIu16 ") " + "which implies a C3 state is supported " + "but there is no P_BLK register block " + "defined to enable the C3 transition.", + fadt->p_lvl3_lat); + else + fwts_passed(fw, + "FADT P_LVL3_LAT is > 100 (%" PRIu16 ") " + "and no P_BLK is defined.", + fadt->p_lvl3_lat); + } + + return; +} + static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1357,6 +1491,20 @@ static int fadt_test1(fwts_framework *fw) fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base); acpi_table_check_fadt_cst_cnt(fw);
+ if (fwts_acpi_init(fw) == FWTS_OK) { + uint64_t pblk = fadt_find_p_blk(fw); + + acpi_table_check_fadt_p_lvl2_lat(fw, pblk); + acpi_table_check_fadt_p_lvl3_lat(fw, pblk); + fwts_acpi_deinit(fw); + } else { + fwts_log_error(fw, "Cannot initialize ACPI namespace."); + fwts_log_info(fw, + "Without a namespace, cannot test " + "values of P_LVL2_LAT or P_LVL3_LAT " + "for correctness."); + } + fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16, fadt->flush_size); fwts_log_info(fw, "FADT FLUSH_STRIDE is %" PRIu16, @@ -1371,31 +1519,6 @@ static int fadt_test1(fwts_framework *fw) }
/* - * Bug LP: #833644 - * - * Remove these tests, really need to put more intelligence into it - * perhaps in the cstates test rather than here. For the moment we - * shall remove this warning as it's giving users false alarms - * See: https://bugs.launchpad.net/ubuntu/+source/fwts/+bug/833644 - */ - /* - if (fadt->p_lvl2_lat > 100) { - fwts_warning(fw, "FADT P_LVL2_LAT is %" PRIi16 ", a value > 100 indicates a " - "system not to support a C2 state.", fadt->p_lvl2_lat); - fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C2 latency in microseconds. The ACPI specification " - "states that a value > 100 indicates that C2 is not supported and hence the " - "ACPI processor idle routine will not use C2 power states."); - } - if (fadt->p_lvl3_lat > 1000) { - fwts_warning(fw, "FADT P_LVL3_LAT is %" PRIu16 ", a value > 1000 indicates a " - "system not to support a C3 state.", fadt->p_lvl3_lat); - fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C3 latency in microseconds. The ACPI specification " - "states that a value > 1000 indicates that C3 is not supported and hence the " - "ACPI processor idle routine will not use C3 power states."); - } - */ - - /* * Cannot really test the Hypervisor Vendor Identity since * the value is provided by the hypervisor to the OS (as a * sign that the ACPI tables have been fabricated), if it
On 09/02/16 01:33, Al Stone wrote:
Add in new compliance checks for the P_LVL2_LAT and P_LVL3_LAT fields. Both of these require knowing the value of an x86 P_BLK; this in turn requires examination of any Processor() declarations in the ACPI name space, which in turn requires the initialization of the ACPI interpreter.
It is probable that these fields are seldom used; processor speeds and feeds are typically controlled via very different mechanisms these days. These tests are therefore for completeness sake and it may be difficult to find ACPI tables using these fields.
Note that these tests allow us to remove commented out versions of simpler tests of these fields.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 173 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 148 insertions(+), 25 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index f932dea..a2ed70c 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -19,6 +19,7 @@
*/ #include "fwts.h" +#include "fwts_acpi_object_eval.h" #include <stdio.h> #include <stdlib.h> @@ -1318,6 +1319,139 @@ static void acpi_table_check_fadt_cst_cnt(fwts_framework *fw) return; } +static uint64_t fadt_find_p_blk(fwts_framework *fw) +{
- uint64_t pblk;
- fwts_list *objects;
- pblk = 0;
- objects = fwts_acpi_object_get_names();
- if (objects) {
fwts_list_link *obj;fwts_list_foreach(obj, objects) {char *name = fwts_list_data(char*, obj);ACPI_OBJECT pr = { 0 };ACPI_BUFFER buf = { sizeof(ACPI_OBJECT), &pr };ACPI_HANDLE handle;ACPI_OBJECT_TYPE type;ACPI_STATUS status;status = AcpiGetHandle(NULL, name, &handle);if (ACPI_FAILURE(status)) {fwts_warning(fw, "Failed to get handle for ""object %s.", name);continue;}status = AcpiGetType(handle, &type);if (ACPI_FAILURE(status)) {fwts_warning(fw, "Failed to get type for ""object %s.", name);continue;}/** If a CPU is not defined as a Processor object,* we don't care here. Per section 8.1.2 and 8.1.3,* defining a CPU with a Device object implies that* a _CST method must be defined, and whatever is* in the _CST overrides the P_BLK and P_LVL*_LAT* values. Since we're only trying to validate the* P_LVL*_LAT values, and they're only used if the* CPUs are defined as Processor objects, we can* ignore any CPUs defined as Device objects.*/if (type == ACPI_TYPE_PROCESSOR) {fwts_log_info(fw, "Found processor %s.", name);status = AcpiEvaluateObject(handle, NULL, NULL,&buf);if (ACPI_FAILURE(status))fwts_warning(fw, "Could not evaluate ""Processor %s", name);else {if (pr.Processor.PblkAddress)pblk = pr.Processor.PblkAddress;}}}- }
- fwts_log_info(fw, "Using P_BLK address of 0x%" PRIx64, pblk);
- return pblk;
+}
+static void acpi_table_check_fadt_p_lvl2_lat(fwts_framework *fw, uint64_t pblk) +{
- if (pblk) {
if (fadt->p_lvl2_lat <= 100)fwts_passed(fw,"FADT P_LVL2_LAT is within proper range ""at %" PRIu16, fadt->p_lvl2_lat);elsefwts_warning(fw,"FADT P_LVL2_LAT is > 100 (%" PRIu16 ") ""but a P_BLK is defined. This implies ""a C2 state is not supported, but there ""is a P_BLK register block defined which ""implies there might be a C2 state that ""works. There is not enough information ""to determine if this is expected or not.",fadt->p_lvl2_lat);- } else {
if (fadt->p_lvl2_lat <= 100)fwts_failed(fw, LOG_LEVEL_MEDIUM,"PLvl2LatDefinedButNotUsable","FADT P_LVL2_LAT is <= 100 (%" PRIu16 ") ""which implies a C2 state is supported ""but there is no P_BLK register block ""defined to enable the C2 transition.",fadt->p_lvl2_lat);elsefwts_passed(fw,"FADT P_LVL2_LAT is > 100 (%" PRIu16 ") ""and no P_BLK is defined.",fadt->p_lvl2_lat);- }
- return;
+}
+static void acpi_table_check_fadt_p_lvl3_lat(fwts_framework *fw, uint64_t pblk) +{
- if (pblk) {
if (fadt->p_lvl3_lat <= 1000)fwts_passed(fw,"FADT P_LVL3_LAT is within proper range ""at %" PRIu16, fadt->p_lvl3_lat);elsefwts_warning(fw,"FADT P_LVL3_LAT is > 1000 (%" PRIu16 ") ""but a P_BLK is defined. This implies ""a C3 state is not supported, but there ""is a P_BLK register block defined which ""implies there might be a C3 state that ""works. There is not enough information ""to determine if this is expected or not.",fadt->p_lvl3_lat);- } else {
if (fadt->p_lvl3_lat <= 1000)fwts_failed(fw, LOG_LEVEL_MEDIUM,"PLvl3LatDefinedButNotUsable","FADT P_LVL3_LAT is <= 1000 (%" PRIu16 ") ""which implies a C3 state is supported ""but there is no P_BLK register block ""defined to enable the C3 transition.",fadt->p_lvl3_lat);elsefwts_passed(fw,"FADT P_LVL3_LAT is > 100 (%" PRIu16 ") ""and no P_BLK is defined.",fadt->p_lvl3_lat);- }
- return;
+}
static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1357,6 +1491,20 @@ static int fadt_test1(fwts_framework *fw) fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base); acpi_table_check_fadt_cst_cnt(fw);
if (fwts_acpi_init(fw) == FWTS_OK) {uint64_t pblk = fadt_find_p_blk(fw);acpi_table_check_fadt_p_lvl2_lat(fw, pblk);acpi_table_check_fadt_p_lvl3_lat(fw, pblk);fwts_acpi_deinit(fw);} else {fwts_log_error(fw, "Cannot initialize ACPI namespace.");fwts_log_info(fw,"Without a namespace, cannot test ""values of P_LVL2_LAT or P_LVL3_LAT ""for correctness.");}- fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16, fadt->flush_size); fwts_log_info(fw, "FADT FLUSH_STRIDE is %" PRIu16,
@@ -1371,31 +1519,6 @@ static int fadt_test1(fwts_framework *fw) } /*
* Bug LP: #833644** Remove these tests, really need to put more intelligence into it* perhaps in the cstates test rather than here. For the moment we* shall remove this warning as it's giving users false alarms* See: https://bugs.launchpad.net/ubuntu/+source/fwts/+bug/833644*/- /*
- if (fadt->p_lvl2_lat > 100) {
fwts_warning(fw, "FADT P_LVL2_LAT is %" PRIi16 ", a value > 100 indicates a ""system not to support a C2 state.", fadt->p_lvl2_lat);fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C2 latency in microseconds. The ACPI specification ""states that a value > 100 indicates that C2 is not supported and hence the ""ACPI processor idle routine will not use C2 power states.");- }
- if (fadt->p_lvl3_lat > 1000) {
fwts_warning(fw, "FADT P_LVL3_LAT is %" PRIu16 ", a value > 1000 indicates a ""system not to support a C3 state.", fadt->p_lvl3_lat);fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C3 latency in microseconds. The ACPI specification ""states that a value > 1000 indicates that C3 is not supported and hence the ""ACPI processor idle routine will not use C3 power states.");- }
- */
- /*
- Cannot really test the Hypervisor Vendor Identity since
- the value is provided by the hypervisor to the OS (as a
- sign that the ACPI tables have been fabricated), if it
I'm interested to see how this fares on a wide range of ACPI tables, we may need to revisit this later, but let's go with it and see what we find in the field.
Acked-by: Colin Ian King colin.king@canonical.com
On 02/09/2016 05:31 AM, Colin Ian King wrote:
On 09/02/16 01:33, Al Stone wrote:
Add in new compliance checks for the P_LVL2_LAT and P_LVL3_LAT fields. Both of these require knowing the value of an x86 P_BLK; this in turn requires examination of any Processor() declarations in the ACPI name space, which in turn requires the initialization of the ACPI interpreter.
It is probable that these fields are seldom used; processor speeds and feeds are typically controlled via very different mechanisms these days. These tests are therefore for completeness sake and it may be difficult to find ACPI tables using these fields.
Note that these tests allow us to remove commented out versions of simpler tests of these fields.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 173 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 148 insertions(+), 25 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index f932dea..a2ed70c 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -19,6 +19,7 @@
*/ #include "fwts.h" +#include "fwts_acpi_object_eval.h" #include <stdio.h> #include <stdlib.h> @@ -1318,6 +1319,139 @@ static void acpi_table_check_fadt_cst_cnt(fwts_framework *fw) return; } +static uint64_t fadt_find_p_blk(fwts_framework *fw) +{
- uint64_t pblk;
- fwts_list *objects;
- pblk = 0;
- objects = fwts_acpi_object_get_names();
- if (objects) {
fwts_list_link *obj;fwts_list_foreach(obj, objects) {char *name = fwts_list_data(char*, obj);ACPI_OBJECT pr = { 0 };ACPI_BUFFER buf = { sizeof(ACPI_OBJECT), &pr };ACPI_HANDLE handle;ACPI_OBJECT_TYPE type;ACPI_STATUS status;status = AcpiGetHandle(NULL, name, &handle);if (ACPI_FAILURE(status)) {fwts_warning(fw, "Failed to get handle for ""object %s.", name);continue;}status = AcpiGetType(handle, &type);if (ACPI_FAILURE(status)) {fwts_warning(fw, "Failed to get type for ""object %s.", name);continue;}/** If a CPU is not defined as a Processor object,* we don't care here. Per section 8.1.2 and 8.1.3,* defining a CPU with a Device object implies that* a _CST method must be defined, and whatever is* in the _CST overrides the P_BLK and P_LVL*_LAT* values. Since we're only trying to validate the* P_LVL*_LAT values, and they're only used if the* CPUs are defined as Processor objects, we can* ignore any CPUs defined as Device objects.*/if (type == ACPI_TYPE_PROCESSOR) {fwts_log_info(fw, "Found processor %s.", name);status = AcpiEvaluateObject(handle, NULL, NULL,&buf);if (ACPI_FAILURE(status))fwts_warning(fw, "Could not evaluate ""Processor %s", name);else {if (pr.Processor.PblkAddress)pblk = pr.Processor.PblkAddress;}}}- }
- fwts_log_info(fw, "Using P_BLK address of 0x%" PRIx64, pblk);
- return pblk;
+}
+static void acpi_table_check_fadt_p_lvl2_lat(fwts_framework *fw, uint64_t pblk) +{
- if (pblk) {
if (fadt->p_lvl2_lat <= 100)fwts_passed(fw,"FADT P_LVL2_LAT is within proper range ""at %" PRIu16, fadt->p_lvl2_lat);elsefwts_warning(fw,"FADT P_LVL2_LAT is > 100 (%" PRIu16 ") ""but a P_BLK is defined. This implies ""a C2 state is not supported, but there ""is a P_BLK register block defined which ""implies there might be a C2 state that ""works. There is not enough information ""to determine if this is expected or not.",fadt->p_lvl2_lat);- } else {
if (fadt->p_lvl2_lat <= 100)fwts_failed(fw, LOG_LEVEL_MEDIUM,"PLvl2LatDefinedButNotUsable","FADT P_LVL2_LAT is <= 100 (%" PRIu16 ") ""which implies a C2 state is supported ""but there is no P_BLK register block ""defined to enable the C2 transition.",fadt->p_lvl2_lat);elsefwts_passed(fw,"FADT P_LVL2_LAT is > 100 (%" PRIu16 ") ""and no P_BLK is defined.",fadt->p_lvl2_lat);- }
- return;
+}
+static void acpi_table_check_fadt_p_lvl3_lat(fwts_framework *fw, uint64_t pblk) +{
- if (pblk) {
if (fadt->p_lvl3_lat <= 1000)fwts_passed(fw,"FADT P_LVL3_LAT is within proper range ""at %" PRIu16, fadt->p_lvl3_lat);elsefwts_warning(fw,"FADT P_LVL3_LAT is > 1000 (%" PRIu16 ") ""but a P_BLK is defined. This implies ""a C3 state is not supported, but there ""is a P_BLK register block defined which ""implies there might be a C3 state that ""works. There is not enough information ""to determine if this is expected or not.",fadt->p_lvl3_lat);- } else {
if (fadt->p_lvl3_lat <= 1000)fwts_failed(fw, LOG_LEVEL_MEDIUM,"PLvl3LatDefinedButNotUsable","FADT P_LVL3_LAT is <= 1000 (%" PRIu16 ") ""which implies a C3 state is supported ""but there is no P_BLK register block ""defined to enable the C3 transition.",fadt->p_lvl3_lat);elsefwts_passed(fw,"FADT P_LVL3_LAT is > 100 (%" PRIu16 ") ""and no P_BLK is defined.",fadt->p_lvl3_lat);- }
- return;
+}
static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1357,6 +1491,20 @@ static int fadt_test1(fwts_framework *fw) fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base); acpi_table_check_fadt_cst_cnt(fw);
if (fwts_acpi_init(fw) == FWTS_OK) {uint64_t pblk = fadt_find_p_blk(fw);acpi_table_check_fadt_p_lvl2_lat(fw, pblk);acpi_table_check_fadt_p_lvl3_lat(fw, pblk);fwts_acpi_deinit(fw);} else {fwts_log_error(fw, "Cannot initialize ACPI namespace.");fwts_log_info(fw,"Without a namespace, cannot test ""values of P_LVL2_LAT or P_LVL3_LAT ""for correctness.");}- fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16, fadt->flush_size); fwts_log_info(fw, "FADT FLUSH_STRIDE is %" PRIu16,
@@ -1371,31 +1519,6 @@ static int fadt_test1(fwts_framework *fw) } /*
* Bug LP: #833644** Remove these tests, really need to put more intelligence into it* perhaps in the cstates test rather than here. For the moment we* shall remove this warning as it's giving users false alarms* See: https://bugs.launchpad.net/ubuntu/+source/fwts/+bug/833644*/- /*
- if (fadt->p_lvl2_lat > 100) {
fwts_warning(fw, "FADT P_LVL2_LAT is %" PRIi16 ", a value > 100 indicates a ""system not to support a C2 state.", fadt->p_lvl2_lat);fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C2 latency in microseconds. The ACPI specification ""states that a value > 100 indicates that C2 is not supported and hence the ""ACPI processor idle routine will not use C2 power states.");- }
- if (fadt->p_lvl3_lat > 1000) {
fwts_warning(fw, "FADT P_LVL3_LAT is %" PRIu16 ", a value > 1000 indicates a ""system not to support a C3 state.", fadt->p_lvl3_lat);fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C3 latency in microseconds. The ACPI specification ""states that a value > 1000 indicates that C3 is not supported and hence the ""ACPI processor idle routine will not use C3 power states.");- }
- */
- /*
- Cannot really test the Hypervisor Vendor Identity since
- the value is provided by the hypervisor to the OS (as a
- sign that the ACPI tables have been fabricated), if it
I'm interested to see how this fares on a wide range of ACPI tables, we may need to revisit this later, but let's go with it and see what we find in the field.
Acked-by: Colin Ian King colin.king@canonical.com
Likewise; I'm really curious to see how this pans out. My gut feel says it'll work fine but really old tables may have some odd things in them, and there may be other quirks that are historical but not at all documented.
When in reduced hardware mode, these fields may or may not be used. If they are, there are rules to check to make the values are reasonable. It is also possible that these fields are simple null and not used at all.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index a2ed70c..05205cb 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1452,6 +1452,66 @@ static void acpi_table_check_fadt_p_lvl3_lat(fwts_framework *fw, uint64_t pblk) return; }
+static void acpi_table_check_fadt_sleep_control_reg(fwts_framework *fw) +{ + if (fwts_acpi_is_reduced_hardware(fadt)) { + if (fadt->sleep_control_reg.address == 0) + fwts_passed(fw, "FADT SLEEP_CONTROL_REG not in use."); + else { + if (fadt->sleep_control_reg.register_bit_width == 8 && + fadt->sleep_control_reg.register_bit_offset == 0) + fwts_passed(fw, "FADT SLEEP_CONTROL_REG is " + "in use and well-defined."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SleepControlRegHasBadGAS", + "FADT SLEEP_CONTROL_REG is " + "in use but register width or " + "offset is incorrect."); + } + } else { + if (fadt->sleep_control_reg.address == 0) + fwts_passed(fw, "FADT SLEEP_CONTROL_REG is null and " + "not available when not in reduced " + "hardware mode."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SleepControlRegNotAllowed", + "FADT SLEEP_CONTROL_REG is defined but " + "will be ignored reduced hardware mode."); + } +} + +static void acpi_table_check_fadt_sleep_status_reg(fwts_framework *fw) +{ + if (fwts_acpi_is_reduced_hardware(fadt)) { + if (fadt->sleep_status_reg.address == 0) + fwts_passed(fw, "FADT SLEEP_STATUS_REG not in use."); + else { + if (fadt->sleep_status_reg.register_bit_width == 8 && + fadt->sleep_status_reg.register_bit_offset == 0) + fwts_passed(fw, "FADT SLEEP_STATUS_REG is " + "in use and well-defined."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SleepStatusRegHasBadGAS", + "FADT SLEEP_STATUS_REG is " + "in use but register width or " + "offset is incorrect."); + } + } else { + if (fadt->sleep_status_reg.address == 0) + fwts_passed(fw, "FADT SLEEP_STATUS_REG is null and " + "not available when not in reduced " + "hardware mode."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SleepStatusRegNotAllowed", + "FADT SLEEP_STATUS_REG is defined but " + "will be ignored reduced hardware mode."); + } +} + static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1518,6 +1578,9 @@ static int fadt_test1(fwts_framework *fw) fwts_log_info(fw, "FADT CENTURY is %" PRIu8, fadt->century); }
+ acpi_table_check_fadt_sleep_control_reg(fw); + acpi_table_check_fadt_sleep_status_reg(fw); + /* * Cannot really test the Hypervisor Vendor Identity since * the value is provided by the hypervisor to the OS (as a
On 09/02/16 01:33, Al Stone wrote:
When in reduced hardware mode, these fields may or may not be used. If they are, there are rules to check to make the values are reasonable. It is also possible that these fields are simple null and not used at all.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index a2ed70c..05205cb 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1452,6 +1452,66 @@ static void acpi_table_check_fadt_p_lvl3_lat(fwts_framework *fw, uint64_t pblk) return; } +static void acpi_table_check_fadt_sleep_control_reg(fwts_framework *fw) +{
- if (fwts_acpi_is_reduced_hardware(fadt)) {
if (fadt->sleep_control_reg.address == 0)fwts_passed(fw, "FADT SLEEP_CONTROL_REG not in use.");else {if (fadt->sleep_control_reg.register_bit_width == 8 &&fadt->sleep_control_reg.register_bit_offset == 0)fwts_passed(fw, "FADT SLEEP_CONTROL_REG is ""in use and well-defined.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"SleepControlRegHasBadGAS","FADT SLEEP_CONTROL_REG is ""in use but register width or ""offset is incorrect.");}- } else {
if (fadt->sleep_control_reg.address == 0)fwts_passed(fw, "FADT SLEEP_CONTROL_REG is null and ""not available when not in reduced ""hardware mode.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"SleepControlRegNotAllowed","FADT SLEEP_CONTROL_REG is defined but ""will be ignored reduced hardware mode.");- }
+}
+static void acpi_table_check_fadt_sleep_status_reg(fwts_framework *fw) +{
- if (fwts_acpi_is_reduced_hardware(fadt)) {
if (fadt->sleep_status_reg.address == 0)fwts_passed(fw, "FADT SLEEP_STATUS_REG not in use.");else {if (fadt->sleep_status_reg.register_bit_width == 8 &&fadt->sleep_status_reg.register_bit_offset == 0)fwts_passed(fw, "FADT SLEEP_STATUS_REG is ""in use and well-defined.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"SleepStatusRegHasBadGAS","FADT SLEEP_STATUS_REG is ""in use but register width or ""offset is incorrect.");}- } else {
if (fadt->sleep_status_reg.address == 0)fwts_passed(fw, "FADT SLEEP_STATUS_REG is null and ""not available when not in reduced ""hardware mode.");elsefwts_failed(fw, LOG_LEVEL_MEDIUM,"SleepStatusRegNotAllowed","FADT SLEEP_STATUS_REG is defined but ""will be ignored reduced hardware mode.");- }
+}
static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1518,6 +1578,9 @@ static int fadt_test1(fwts_framework *fw) fwts_log_info(fw, "FADT CENTURY is %" PRIu8, fadt->century); }
- acpi_table_check_fadt_sleep_control_reg(fw);
- acpi_table_check_fadt_sleep_status_reg(fw);
- /*
- Cannot really test the Hypervisor Vendor Identity since
- the value is provided by the hypervisor to the OS (as a
Acked-by: Colin Ian King colin.king@canonical.com
Now that the tests have been resequenced, added to, and generally overhauled, clean up some variables in test1 that are no longer useful.
Signed-off-by: Al Stone al.stone@linaro.org --- src/acpi/fadt/fadt.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 05205cb..fbc71fd 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1514,8 +1514,6 @@ static void acpi_table_check_fadt_sleep_status_reg(fwts_framework *fw)
static int fadt_test1(fwts_framework *fw) { - bool passed = true; - acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw); acpi_table_check_fadt_reserved(fw); @@ -1589,8 +1587,6 @@ static int fadt_test1(fwts_framework *fw) */ fwts_log_info(fw, "FADT Hypervisor Vendor Identity is %" PRIu64, fadt->hypervisor_id); - if (passed) - fwts_passed(fw, "No issues found in FADT table.");
return FWTS_OK; }
On 09/02/16 01:33, Al Stone wrote:
Now that the tests have been resequenced, added to, and generally overhauled, clean up some variables in test1 that are no longer useful.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 05205cb..fbc71fd 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1514,8 +1514,6 @@ static void acpi_table_check_fadt_sleep_status_reg(fwts_framework *fw) static int fadt_test1(fwts_framework *fw) {
- bool passed = true;
- acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw); acpi_table_check_fadt_reserved(fw);
@@ -1589,8 +1587,6 @@ static int fadt_test1(fwts_framework *fw) */ fwts_log_info(fw, "FADT Hypervisor Vendor Identity is %" PRIu64, fadt->hypervisor_id);
- if (passed)
fwts_passed(fw, "No issues found in FADT table.");return FWTS_OK; }
Acked-by: Colin Ian King colin.king@canonical.com
Thanks Al for all these improvements. Are there any fwts-test patches to come later?
Colin
On 02/09/2016 05:34 AM, Colin Ian King wrote:
On 09/02/16 01:33, Al Stone wrote:
Now that the tests have been resequenced, added to, and generally overhauled, clean up some variables in test1 that are no longer useful.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 05205cb..fbc71fd 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1514,8 +1514,6 @@ static void acpi_table_check_fadt_sleep_status_reg(fwts_framework *fw) static int fadt_test1(fwts_framework *fw) {
- bool passed = true;
- acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw); acpi_table_check_fadt_reserved(fw);
@@ -1589,8 +1587,6 @@ static int fadt_test1(fwts_framework *fw) */ fwts_log_info(fw, "FADT Hypervisor Vendor Identity is %" PRIu64, fadt->hypervisor_id);
- if (passed)
fwts_passed(fw, "No issues found in FADT table.");return FWTS_OK; }
Acked-by: Colin Ian King colin.king@canonical.com
Thanks Al for all these improvements. Are there any fwts-test patches to come later?
Colin
Thanks for all the review. This turned out a lot bigger than I thought it might so I appreciate the patience involved.
I did run make check but I did not get any regression test failures that I had not already seen and reported or fixed (there's a previous series called "Update several regression tests" that still needs to be ACKd and pulled in, btw). I'll double check that, of course, and send anything I find along.
Or, were you expecting new sections in fwts-test? I hadn't really thought that about it, if that's what's being asked....is there a rule of thumb the project follows that applies here?
On 09/02/16 23:30, Al Stone wrote:
On 02/09/2016 05:34 AM, Colin Ian King wrote:
On 09/02/16 01:33, Al Stone wrote:
Now that the tests have been resequenced, added to, and generally overhauled, clean up some variables in test1 that are no longer useful.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 05205cb..fbc71fd 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1514,8 +1514,6 @@ static void acpi_table_check_fadt_sleep_status_reg(fwts_framework *fw) static int fadt_test1(fwts_framework *fw) {
- bool passed = true;
- acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw); acpi_table_check_fadt_reserved(fw);
@@ -1589,8 +1587,6 @@ static int fadt_test1(fwts_framework *fw) */ fwts_log_info(fw, "FADT Hypervisor Vendor Identity is %" PRIu64, fadt->hypervisor_id);
- if (passed)
fwts_passed(fw, "No issues found in FADT table.");return FWTS_OK; }
Acked-by: Colin Ian King colin.king@canonical.com
Thanks Al for all these improvements. Are there any fwts-test patches to come later?
Colin
Thanks for all the review. This turned out a lot bigger than I thought it might so I appreciate the patience involved.
I did run make check but I did not get any regression test failures that I had not already seen and reported or fixed (there's a previous series called "Update several regression tests" that still needs to be ACKd and pulled in, btw). I'll double check that, of course, and send anything I find along.
Or, were you expecting new sections in fwts-test? I hadn't really thought that about it, if that's what's being asked....is there a rule of thumb the project follows that applies here?
No worries about extra tests for now. Let's see how this patch set shakes down on a range of firmware over the next few release cycles.
Colin
On 02/12/2016 03:09 AM, Colin Ian King wrote:
On 09/02/16 23:30, Al Stone wrote:
On 02/09/2016 05:34 AM, Colin Ian King wrote:
On 09/02/16 01:33, Al Stone wrote:
Now that the tests have been resequenced, added to, and generally overhauled, clean up some variables in test1 that are no longer useful.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 05205cb..fbc71fd 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1514,8 +1514,6 @@ static void acpi_table_check_fadt_sleep_status_reg(fwts_framework *fw) static int fadt_test1(fwts_framework *fw) {
- bool passed = true;
- acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw); acpi_table_check_fadt_reserved(fw);
@@ -1589,8 +1587,6 @@ static int fadt_test1(fwts_framework *fw) */ fwts_log_info(fw, "FADT Hypervisor Vendor Identity is %" PRIu64, fadt->hypervisor_id);
- if (passed)
fwts_passed(fw, "No issues found in FADT table.");return FWTS_OK; }
Acked-by: Colin Ian King colin.king@canonical.com
Thanks Al for all these improvements. Are there any fwts-test patches to come later?
Colin
Thanks for all the review. This turned out a lot bigger than I thought it might so I appreciate the patience involved.
I did run make check but I did not get any regression test failures that I had not already seen and reported or fixed (there's a previous series called "Update several regression tests" that still needs to be ACKd and pulled in, btw). I'll double check that, of course, and send anything I find along.
Or, were you expecting new sections in fwts-test? I hadn't really thought that about it, if that's what's being asked....is there a rule of thumb the project follows that applies here?
No worries about extra tests for now. Let's see how this patch set shakes down on a range of firmware over the next few release cycles.
Colin
Okey dokey. Shall I send a v2 of this set or are we copacetic?
On 02/09/2016 05:34 AM, Colin Ian King wrote:
On 09/02/16 01:33, Al Stone wrote:
Now that the tests have been resequenced, added to, and generally overhauled, clean up some variables in test1 that are no longer useful.
Signed-off-by: Al Stone al.stone@linaro.org
src/acpi/fadt/fadt.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 05205cb..fbc71fd 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -1514,8 +1514,6 @@ static void acpi_table_check_fadt_sleep_status_reg(fwts_framework *fw) static int fadt_test1(fwts_framework *fw) {
- bool passed = true;
- acpi_table_check_fadt_firmware_ctrl(fw); acpi_table_check_fadt_dsdt(fw); acpi_table_check_fadt_reserved(fw);
@@ -1589,8 +1587,6 @@ static int fadt_test1(fwts_framework *fw) */ fwts_log_info(fw, "FADT Hypervisor Vendor Identity is %" PRIu64, fadt->hypervisor_id);
- if (passed)
fwts_passed(fw, "No issues found in FADT table.");return FWTS_OK; }
Acked-by: Colin Ian King colin.king@canonical.com
Thanks Al for all these improvements. Are there any fwts-test patches to come later?
Colin
Argh. My bad. I must have missed these the first time through. So, attached are the regression test cleanups. Sorry about that....
-
Al Stone -
Colin Ian King -
ivanhu