ACK: [PATCH 1/2] acpi: add fwts_acpi_space_id_check to check GAS adrress space ids
Colin Ian King
colin.king at canonical.com
Thu Jan 7 09:37:34 UTC 2021
On 07/01/2021 00:34, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
> src/acpi/ecdt/ecdt.c | 25 ++-------
> src/acpi/einj/einj.c | 12 +----
> src/acpi/mchi/mchi.c | 16 ++----
> src/acpi/pcct/pcct.c | 27 ++++------
> src/acpi/spmi/spmi.c | 20 ++------
> src/acpi/tcpa/tcpa.c | 26 ++++------
> src/lib/include/fwts_acpi_tables.h | 1 +
> src/lib/src/fwts_acpi_tables.c | 82 ++++++++++++++++++++++++++++++
> 8 files changed, 117 insertions(+), 92 deletions(-)
>
> diff --git a/src/acpi/ecdt/ecdt.c b/src/acpi/ecdt/ecdt.c
> index 23e2ae93..0cf81893 100644
> --- a/src/acpi/ecdt/ecdt.c
> +++ b/src/acpi/ecdt/ecdt.c
> @@ -41,18 +41,9 @@ static int ecdt_test1(fwts_framework *fw)
> uint32_t min_length;
> int i;
>
> -
> /* Must be I/O Address Space or a Memory Space */
> - if ((ecdt->ec_control.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO) &&
> - (ecdt->ec_control.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY)) {
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "ECDTECControlInvalidAddrSpaceID",
> - "ECDT EC_CONTROL address space ID is 0x%2.2" PRIx8
> - "which is not a System I/O Space ID or a System "
> - "Memory Space ID",
> - ecdt->ec_control.address_space_id);
> - passed = false;
> - }
> + fwts_acpi_space_id_check(fw, "ECDT", "EC_CONTROL", &passed, ecdt->ec_control.address_space_id, 2,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY, FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO);
> /* Must be correct Access Size */
> if (ecdt->ec_control.access_width > 4) {
> fwts_failed(fw, LOG_LEVEL_MEDIUM,
> @@ -63,16 +54,8 @@ static int ecdt_test1(fwts_framework *fw)
> passed = false;
> }
> /* Must be I/O Address Space or a Memory Space */
> - if ((ecdt->ec_data.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO) &&
> - (ecdt->ec_data.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY)) {
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "ECDTECControlInvalidAddrSpaceID",
> - "ECDT EC_DATA address space ID is 0x%2.2" PRIx8
> - "which is not a System I/O Space ID or a System "
> - "Memory Space ID",
> - ecdt->ec_data.address_space_id);
> - passed = false;
> - }
> + fwts_acpi_space_id_check(fw, "ECDT", "EC_DATA", &passed, ecdt->ec_control.address_space_id, 2,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY, FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO);
> /* Must be correct Access Size */
> if (ecdt->ec_data.access_width > 4) {
> fwts_failed(fw, LOG_LEVEL_MEDIUM,
> diff --git a/src/acpi/einj/einj.c b/src/acpi/einj/einj.c
> index e6f8a7e9..de303f65 100644
> --- a/src/acpi/einj/einj.c
> +++ b/src/acpi/einj/einj.c
> @@ -104,16 +104,8 @@ static int einj_test1(fwts_framework *fw)
> }
>
> fwts_acpi_reserved_zero_check(fw, "EINJ", "Reserved", entry->reserved, sizeof(entry->reserved), &passed);
> -
> - if (gas.address_space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> - gas.address_space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "EINJBadSpaceId",
> - "EINJ Address_Space_ID must be within 0 or "
> - "1, got 0x%" PRIx8 " instead",
> - gas.address_space_id);
> - passed = false;
> - }
> + fwts_acpi_space_id_check(fw, "EINJ", "Register Region", &passed, gas.address_space_id, 2,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY, FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO);
>
> fwts_log_nl(fw);
> }
> diff --git a/src/acpi/mchi/mchi.c b/src/acpi/mchi/mchi.c
> index 446c5722..7fe59ded 100644
> --- a/src/acpi/mchi/mchi.c
> +++ b/src/acpi/mchi/mchi.c
> @@ -123,19 +123,13 @@ static int mchi_test1(fwts_framework *fw)
> mchi->global_system_interrupt);
> }
>
> - if ((mchi->base_address.address_space_id != 0x00) &&
> - (mchi->base_address.address_space_id != 0x01) &&
> - (mchi->base_address.address_space_id != 0x04)) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "MCHIAddrSpaceIdInvalid",
> - "MCHI Address Space ID is 0x%2.2" PRIx8 " and should be "
> - "0x00 (System Memory), 0x01 (System I/O) or 0x04 (SMBus)",
> - mchi->base_address.address_space_id);
> - }
> + fwts_acpi_space_id_check(fw, "MCHI", "Base Address", &passed, mchi->base_address.address_space_id, 3,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO,
> + FWTS_GAS_ADDR_SPACE_ID_SMBUS);
>
> /* SMBus specific checks */
> - if (mchi->base_address.address_space_id == 0x04) {
> + if (mchi->base_address.address_space_id == FWTS_GAS_ADDR_SPACE_ID_SMBUS) {
> if ((mchi->pci_device_flag & 0x01) == 1) {
> passed = false;
> fwts_failed(fw, LOG_LEVEL_HIGH,
> diff --git a/src/acpi/pcct/pcct.c b/src/acpi/pcct/pcct.c
> index a255bb30..e2e40164 100644
> --- a/src/acpi/pcct/pcct.c
> +++ b/src/acpi/pcct/pcct.c
> @@ -41,21 +41,19 @@ static bool subspace_length_equal(fwts_framework *fw, uint8_t type, uint8_t type
>
> static void gas_messages(fwts_framework *fw, uint8_t type, fwts_acpi_gas *gas, bool *passed)
> {
> + char label[20];
> +
> fwts_log_info_verbatim(fw, " Address Space ID 0x%2.2" PRIx8, gas->address_space_id);
> fwts_log_info_verbatim(fw, " Register Bit Width 0x%2.2" PRIx8, gas->register_bit_width);
> fwts_log_info_verbatim(fw, " Register Bit Offset 0x%2.2" PRIx8, gas->register_bit_offset);
> fwts_log_info_verbatim(fw, " Access Size 0x%2.2" PRIx8, gas->access_width);
> fwts_log_info_verbatim(fw, " Address 0x%16.16" PRIx64, gas->address);
>
> - if ((gas->address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO) &&
> - (gas->address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY) &&
> - (gas->address_space_id != FWTS_GAS_ADDR_SPACE_ID_FFH)) {
> - *passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "PCCTSubspaceInvalidAddrSpaceID",
> - "PCCT Subspace Type %" PRId8 " has space ID = %" PRId8
> - " which is not System I/O, Memory or FFH", type, gas->address_space_id);
> - }
> + snprintf(label, 20, "Subspace Type % " PRId8, type);
> + fwts_acpi_space_id_check(fw, "PCCT", label, passed, gas->address_space_id, 3,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO,
> + FWTS_GAS_ADDR_SPACE_ID_FFH);
> }
>
> static void memory_length(fwts_framework *fw, uint8_t type, uint64_t memory_range, uint64_t min_length, bool *passed)
> @@ -108,15 +106,8 @@ static void generic_comm_test(fwts_framework *fw, fwts_acpi_table_pcct_subspace_
> fwts_log_info_verbatim(fw, " Max Periodic Access Rate: 0x%8.8" PRIx32, entry->max_periodic_access_rate);
> fwts_log_info_verbatim(fw, " Min Request Turnaround Time: 0x%8.8" PRIx32, entry->min_request_turnaround_time);
>
> - if ((gas->address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO) &&
> - (gas->address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY)) {
> - *passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "PCCTSubspaceInvalidAddrSpaceID",
> - "PCCT Subspace Type 0 has space ID = 0x%2.2" PRIx8
> - " which is not System I/O or Memory",
> - gas->address_space_id);
> - }
> + fwts_acpi_space_id_check(fw, "PCCT", "Subspace Type 0", passed, gas->address_space_id, 2,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY, FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO);
> }
>
> static void hw_reduced_comm_test_type1(fwts_framework *fw, fwts_acpi_table_pcct_subspace_type_1 *entry, bool *passed)
> diff --git a/src/acpi/spmi/spmi.c b/src/acpi/spmi/spmi.c
> index 4eed3518..8971ad21 100644
> --- a/src/acpi/spmi/spmi.c
> +++ b/src/acpi/spmi/spmi.c
> @@ -125,21 +125,11 @@ static int spmi_test1(fwts_framework *fw)
> }
>
> /* Base address must be one of 3 types, System Memory, System I/O or SMBUS */
> - if ((spmi->base_address.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY) &&
> - (spmi->base_address.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO) &&
> - (spmi->base_address.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SMBUS)) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "SPMIBaseAddressSpaceID",
> - "SPMI Base Address Space ID is 0x%2.2" PRIx8 " but should be one of "
> - "0x%2.2" PRIx8 " (System Memory), "
> - "0x%2.2" PRIx8 " (System I/O) or "
> - "0x%2.2" PRIx8 " (SMBUS)",
> - spmi->base_address.address_space_id,
> - FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY,
> - FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO,
> - FWTS_GAS_ADDR_SPACE_ID_SMBUS);
> - }
> + fwts_acpi_space_id_check(fw, "SPMI", "Base Address", &passed, spmi->base_address.address_space_id, 3,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO,
> + FWTS_GAS_ADDR_SPACE_ID_SMBUS);
> +
> /*
> * For SSIF: The Address_Space_ID is SMBUS and the address field of the GAS
> * holds the 7-bit slave address of the BMC on the host SMBus
> diff --git a/src/acpi/tcpa/tcpa.c b/src/acpi/tcpa/tcpa.c
> index 27107f09..a5b0d512 100644
> --- a/src/acpi/tcpa/tcpa.c
> +++ b/src/acpi/tcpa/tcpa.c
> @@ -100,23 +100,15 @@ static int tcpa_server_test(fwts_framework *fw, fwts_acpi_table_tcpa *tcpa)
> }
> }
>
> - if (tcpa->server.base_addr.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY &&
> - tcpa->server.base_addr.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "TCPABadAddressID",
> - "TCPA base address ID must be 1 or zero, got "
> - "0x%2.2" PRIx8 " instead", tcpa->server.base_addr.address_space_id);
> - }
> -
> - if (tcpa->server.config_addr.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY &&
> - tcpa->server.config_addr.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "TCPABadAddressID",
> - "TCPA configuration address ID must be 1 or zero, got "
> - "0x%2.2" PRIx8 " instead", tcpa->server.config_addr.address_space_id);
> - }
> + fwts_acpi_space_id_check(fw, "TCPA", "Base Address", &passed,
> + tcpa->server.base_addr.address_space_id, 2,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO);
> +
> + fwts_acpi_space_id_check(fw, "TCPA", "Configuration Address", &passed,
> + tcpa->server.config_addr.address_space_id, 2,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY,
> + FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO);
>
> return passed;
> }
> diff --git a/src/lib/include/fwts_acpi_tables.h b/src/lib/include/fwts_acpi_tables.h
> index 602fb571..fcfb9198 100644
> --- a/src/lib/include/fwts_acpi_tables.h
> +++ b/src/lib/include/fwts_acpi_tables.h
> @@ -70,6 +70,7 @@ void fwts_acpi_reserved_type_check(fwts_framework *fw, const char *table, uint8_
> bool fwts_acpi_table_length_check(fwts_framework *fw, const char *table, uint32_t length, uint32_t size);
> bool fwts_acpi_structure_length_check(fwts_framework *fw, const char *table, uint8_t subtable_type, uint32_t subtable_length, uint32_t size);
> void fwts_acpi_fixed_value_check(fwts_framework *fw, fwts_log_level level, const char *table, const char *field, uint8_t actual, uint8_t must_be, bool *passed);
> +void fwts_acpi_space_id_check(fwts_framework *fw, const char *table, const char *field, bool *passed, uint8_t actual, uint8_t num_type, ...);
>
> uint32_t fwts_get_acpi_version(fwts_framework *fw);
>
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index 2d0082b1..d88d31e4 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -1613,6 +1613,88 @@ void fwts_acpi_reserved_type_check(
> }
> }
>
> +/*
> + * Space Id defined for Generic Address Structure (GAS)
> + * See Table 5.1: Generic Address Structure (GAS) in ACPI spec
> + * Also see fwts_acpi.h
> + */
> +static const char *gas_space_id_names[] = {
> + "System Memory (0x0)",
> + "System I/O (0x1)",
> + "PCI Configuration (0x2)",
> + "Embedded Controller (0x3)",
> + "SMBus (0x4)",
> + "SystemCMOS (0x5)",
> + "PciBarTarget (0x6)",
> + "IPMI (0x7)",
> + "General PurposeIO (0x8)",
> + "GenericSerialBus (0x9)",
> + "Platform Communications Channel (0xa)",
> + "Functional Fixed Hardware (0x7f)",
> +};
> +
> +static const char *get_space_id_name(uint8_t id) {
> + if (id <= 0x0a)
> + return gas_space_id_names[id];
> + else if (id == 0x7f)
> + return gas_space_id_names[0x0b];
> + else
> + return NULL;
> +}
> +
> +/*
> + * fwts_acpi_space_id_check()
> + * check whether gas space id matches
> + */
> +void fwts_acpi_space_id_check(
> + fwts_framework *fw,
> + const char *table,
> + const char *field,
> + bool *passed,
> + uint8_t actual,
> + uint8_t num_type,
> + ...)
> +{
> + bool matched = false;
> + char must_be_id[255];
> + uint16_t length = 0;
> + uint8_t i, must_be;
> + char label[25];
> + va_list ap;
> +
> + strncpy(label, table, 4); /* ACPI table name is 4 char long */
> + strncpy(label + 4, "BadAddressSpaceId", sizeof(label) - 4);
> + memset(must_be_id, 0, strlen(must_be_id));
> +
> + va_start(ap, num_type);
> + for (i = 0; i < num_type; i++) {
> + must_be = va_arg(ap, int);
> + if (actual == must_be) {
> + matched = true;
> + break;
> + }
> +
> + length += strlen(get_space_id_name(must_be));
> + if (length > 255)
> + continue;
> +
> + strncat(must_be_id, get_space_id_name(must_be), strlen(get_space_id_name(must_be)));
> + if (i < num_type - 2)
> + strncat(must_be_id, ", ", 2);
> + else if (i == num_type - 2)
> + strncat(must_be_id, " or ", 4);
> + }
> +
> + if (!matched) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, label,
> + "%4.4s %s Space ID must be one of %s, got %s instead.",
> + table, field, must_be_id, get_space_id_name(actual));
> + *passed = false;
> + }
> +
> + va_end(ap);
> +}
> +
> /*
> * fwts_acpi_table_length_check()
> * verify whether table length is sane
>
Acked-by: Colin Ian King <colin.king at canonical.com>
More information about the fwts-devel
mailing list