diff mbox series

[1/2] acpi: add fwts_acpi_space_id_check to check GAS adrress space ids

Message ID 20210107003406.676264-1-alex.hung@canonical.com
State Accepted
Headers show
Series [1/2] acpi: add fwts_acpi_space_id_check to check GAS adrress space ids | expand

Commit Message

Alex Hung Jan. 7, 2021, 12:34 a.m. UTC
Signed-off-by: Alex Hung <alex.hung@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(-)

Comments

Colin Ian King Jan. 7, 2021, 9:37 a.m. UTC | #1
On 07/01/2021 00:34, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung@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@canonical.com>
Ivan Hu Jan. 11, 2021, 6:45 a.m. UTC | #2
On 1/7/21 8:34 AM, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung@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: Ivan Hu <ivan.hu@canonical.com>
diff mbox series

Patch

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