ACK: [PATCH 1/2] acpi: replace checks for reserved bits by fwts_acpi_reserved_bits_check

Colin Ian King colin.king at canonical.com
Tue Sep 26 00:21:57 UTC 2017


On 26/09/17 01:15, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
>  src/acpi/bgrt/bgrt.c | 11 +++------
>  src/acpi/gtdt/gtdt.c | 54 +++++++++++++++----------------------------
>  src/acpi/hest/hest.c | 22 +++++++-----------
>  src/acpi/iort/iort.c | 65 +++++++++++++++-------------------------------------
>  src/acpi/lpit/lpit.c |  9 +-------
>  src/acpi/mchi/mchi.c | 22 +++++-------------
>  src/acpi/nfit/nfit.c |  9 +-------
>  src/acpi/pptt/pptt.c |  9 +-------
>  src/acpi/spmi/spmi.c | 19 +++------------
>  9 files changed, 59 insertions(+), 161 deletions(-)
> 
> diff --git a/src/acpi/bgrt/bgrt.c b/src/acpi/bgrt/bgrt.c
> index 25030ad..5b9b413 100644
> --- a/src/acpi/bgrt/bgrt.c
> +++ b/src/acpi/bgrt/bgrt.c
> @@ -66,14 +66,9 @@ static int bgrt_test1(fwts_framework *fw)
>  			" and not the expected value of 0x01",
>  			bgrt->version);
>  	}
> -	if (bgrt->status & ~0x7) {
> -		passed = false;
> -		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -			"BGRTStatusRersevedBits",
> -			"BGRT: Status field is 0x%" PRIx8
> -			", reserved bits [7:3] should be zero",
> -			bgrt->status);
> -	}
> +
> +	fwts_acpi_reserved_bits_check(fw, "BGRT", "BGRT Status", bgrt->status, sizeof(bgrt->status), 3, 7, &passed);
> +
>  	if (bgrt->image_type > 0x00) {
>  		passed = false;
>  		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> diff --git a/src/acpi/gtdt/gtdt.c b/src/acpi/gtdt/gtdt.c
> index 1559d31..f3a8044 100644
> --- a/src/acpi/gtdt/gtdt.c
> +++ b/src/acpi/gtdt/gtdt.c
> @@ -67,6 +67,7 @@ static int gtdt_test1(fwts_framework *fw)
>  		fwts_acpi_table_gtdt_block *block;
>  		fwts_acpi_table_gtdt_block_timer *block_timer;
>  		fwts_acpi_table_gtdt_watchdog *watchdog;
> +		char field[80];
>  
>  		switch (*ptr) {
>  		case 0x00:
> @@ -145,33 +146,18 @@ static int gtdt_test1(fwts_framework *fw)
>  						block_timer->reserved[1],
>  						block_timer->reserved[2]);
>  				}
> -				if (block_timer->phys_timer_flags & ~0x3) {
> -					passed = false;
> -					fwts_failed(fw, LOG_LEVEL_LOW,
> -						"GTDTFBlockPhysTimerFlagReservedNonZero",
> -						"GTDT block %" PRIu32 " physical timer "
> -						"flags reserved bits 2 to 31 are "
> -						"non-zero, instead got 0x%" PRIx32 ".",
> -						i, block_timer->phys_timer_flags);
> -				}
> -				if (block_timer->virt_timer_flags & ~0x3) {
> -					passed = false;
> -					fwts_failed(fw, LOG_LEVEL_LOW,
> -						"GTDTFBlockVirtTimerFlagReservedNonZero",
> -						"GTDT block %" PRIu32 " virtual timer "
> -						"flags reserved bits 2 to 31 are "
> -						"non-zero, instead got 0x%" PRIx32 ".",
> -						i, block_timer->virt_timer_flags);
> -				}
> -				if (block_timer->common_flags & ~0x3) {
> -					passed = false;
> -					fwts_failed(fw, LOG_LEVEL_LOW,
> -						"GTDTFBlockCommonFlagReservedNonZero",
> -						"GTDT block %" PRIu32 " common flags "
> -						"reserved bits 2 to 31 are "
> -						"non-zero, instead got 0x%" PRIx32 ".",
> -						i, block_timer->common_flags);
> -				}
> +
> +				snprintf(field, sizeof(field), "block %" PRIu32 " physical timer flags", i);
> +				fwts_acpi_reserved_bits_check(fw, "GTDT", field, block_timer->phys_timer_flags,
> +					sizeof(block_timer->phys_timer_flags), 2, 31, &passed);
> +
> +				snprintf(field, sizeof(field), "block %" PRIu32 " virtual timer flags", i);
> +				fwts_acpi_reserved_bits_check(fw, "GTDT", field, block_timer->virt_timer_flags,
> +					sizeof(block_timer->virt_timer_flags), 2, 31, &passed);
> +
> +				snprintf(field, sizeof(field), "block %" PRIu32 " common flags", i);
> +				fwts_acpi_reserved_bits_check(fw, "GTDT", field, block_timer->common_flags,
> +					sizeof(block_timer->common_flags), 2, 31, &passed);
>  			}
>  			ptr += block->length;
>  			break;
> @@ -203,15 +189,11 @@ static int gtdt_test1(fwts_framework *fw)
>  					" reserved is non-zero, got 0x%" PRIx8,
>  					i, watchdog->reserved);
>  			}
> -			if (watchdog->watchdog_timer_flags & ~0x7) {
> -				passed = false;
> -				fwts_failed(fw, LOG_LEVEL_LOW,
> -					"GTDTWatchDogTimerFlagReservedNonZero",
> -					"GTDT SBSA generic watchdog timer %" PRIu32
> -					" flags reserved bits 3 to 31 are non-zero, "
> -					"instead got 0x%" PRIx8 ".",
> -					i, watchdog->watchdog_timer_flags);
> -			}
> +
> +			snprintf(field, sizeof(field), "SBSA generic watchdog timer %" PRIu32 " flags", i);
> +			fwts_acpi_reserved_bits_check(fw, "GTDT", field, watchdog->watchdog_timer_flags,
> +				sizeof(watchdog->watchdog_timer_flags), 3, 31, &passed);
> +
>  			ptr += watchdog->length;
>  			break;
>  		default:
> diff --git a/src/acpi/hest/hest.c b/src/acpi/hest/hest.c
> index 86a5312..289bd18 100644
> --- a/src/acpi/hest/hest.c
> +++ b/src/acpi/hest/hest.c
> @@ -658,13 +658,10 @@ static void hest_check_generic_error_source(
>  			"expecting value 0 to 11",
>  			source->notification.type);
>  	}
> -	if (source->notification.configuration_write_enable & ~0x3f) {
> -		*passed = false;
> -		fwts_failed(fw, LOG_LEVEL_LOW,
> -			"HESTIA32ConfigWriteEnabledReservedNonZero",
> -			"HEST Configuration Write Enabled Reserved bits [6:31] "
> -			"are non-zero.");
> -	}
> +
> +	fwts_acpi_reserved_bits_check(fw, "HEST", "HEST Configuration Write Enabled",
> +		source->notification.configuration_write_enable,
> +		sizeof(source->notification.configuration_write_enable), 6, 31, passed);
>  
>  	*length -= sizeof(fwts_acpi_table_hest_generic_hardware_error_source);
>  	*data += sizeof(fwts_acpi_table_hest_generic_hardware_error_source);
> @@ -776,13 +773,10 @@ static void hest_check_generic_error_source_v2(
>  			"expecting value 0 to 11",
>  			source->notification.type);
>  	}
> -	if (source->notification.configuration_write_enable & ~0x3f) {
> -		*passed = false;
> -		fwts_failed(fw, LOG_LEVEL_LOW,
> -			"HESTIA32ConfigWriteEnabledReservedNonZero",
> -			"HEST Configuration Write Enabled Reserved bits [6:31] "
> -			"are non-zero.");
> -	}
> +
> +	fwts_acpi_reserved_bits_check(fw, "HEST", "HEST Configuration Write Enabled",
> +		source->notification.configuration_write_enable,
> +		sizeof(source->notification.configuration_write_enable), 6, 31, passed);
>  
>  	*length -= sizeof(fwts_acpi_table_hest_generic_hardware_error_source_v2);
>  	*data += sizeof(fwts_acpi_table_hest_generic_hardware_error_source_v2);
> diff --git a/src/acpi/iort/iort.c b/src/acpi/iort/iort.c
> index b7046ea..9c20610 100644
> --- a/src/acpi/iort/iort.c
> +++ b/src/acpi/iort/iort.c
> @@ -148,28 +148,6 @@ static void iort_id_mappings_dump(
>  }
>  
>  /*
> - *  iort_id_mapping_check()
> - *	sanity check ID mapping
> - */
> -static void iort_id_mapping_check(
> -	fwts_framework *fw,
> -	uint32_t i,
> -	fwts_acpi_table_iort_id_mapping *id_mapping,
> -	bool *passed)
> -{
> -	if (id_mapping->flags & ~1) {
> -		*passed = false;
> -		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -			"IORTIdMappingReservedNonZero",
> -			"IORT ID Mapping %" PRIu32 " Reserved Flags field reserved "
> -			"bits [31:1] should be all zero, got 0x%" PRIx32
> -			" instead",
> -			i, id_mapping->flags);
> -	}
> -	/* Should also check output_reference is out of range or not */
> -}
> -
> -/*
>   *  iort_id_mappings_check()
>   *	snaity check array of ID mappings
>   */
> @@ -182,6 +160,7 @@ static void iort_id_mappings_check(
>  	uint32_t i;
>  	fwts_acpi_table_iort_node *node = (fwts_acpi_table_iort_node *)data;
>  	fwts_acpi_table_iort_id_mapping *id_mapping;
> +	char field[80];
>  
>  	id_mapping = (fwts_acpi_table_iort_id_mapping *)(data + node->id_array_offset);
>  
> @@ -195,7 +174,9 @@ static void iort_id_mappings_check(
>  				"or the IORT table size or the node is too small.", i);
>  			break;
>  		}
> -		iort_id_mapping_check(fw, i, id_mapping, passed);
> +
> +		snprintf(field, sizeof(field), "ID Mapping %" PRIu32 " flags", i);
> +		fwts_acpi_reserved_bits_check(fw, "IORT", field, id_mapping->flags, sizeof(id_mapping->flags), 1, 31, passed);
>  	}
>  }
>  
> @@ -381,6 +362,7 @@ static void iort_memory_access_properties_check(
>  	bool *passed)
>  {
>  	uint8_t cca, cpm, dacs;
> +	char field[80];
>  
>  	if (properties->cache_coherent > 1) {
>  		*passed = false;
> @@ -392,30 +374,19 @@ static void iort_memory_access_properties_check(
>  			"not coherent).",
>  			name, properties->cache_coherent);
>  	}
> -	if (properties->allocation_hints & ~0xf) {
> -		*passed = false;
> -		fwts_failed(fw, LOG_LEVEL_HIGH,
> -			"IORTAllocHintsReservedNonZero",
> -			"IORT %s Allocation Hints reserved bits "
> -			"[7:4] are reserved and should be zero.",
> -			name);
> -	}
> -	if (properties->reserved) {
> -		*passed = false;
> -		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -			"IORTNodeReservedNonZero",
> -			"IORT %s Node Reserved is 0x%" PRIx32
> -			" and is reserved and should be zero.",
> -			name, properties->reserved);
> -	}
> -	if (properties->memory_access_flags & ~0x3) {
> -		*passed = false;
> -		fwts_failed(fw, LOG_LEVEL_HIGH,
> -			"IORTMemoryAccessFlagsReservedNonZero",
> -			"IORT %s Memory Access Flags is 0x%" PRIx8
> -			" and all the reserved bits [7:2] should be zero but some are not.",
> -			name, properties->memory_access_flags);
> -	}
> +
> +	snprintf(field, sizeof(field), "%s Allocation Hints", name);
> +	fwts_acpi_reserved_bits_check(fw, "IORT", field, properties->allocation_hints,
> +		sizeof(properties->allocation_hints), 4, 7, passed);
> +
> +	snprintf(field, sizeof(field), "%s Reserved", name);
> +	fwts_acpi_reserved_zero_check(fw, "IORT", field, properties->reserved,
> +		sizeof(properties->reserved), passed);
> +
> +	snprintf(field, sizeof(field), "%s  Memory Access Flags", name);
> +	fwts_acpi_reserved_bits_check(fw, "IORT", field, properties->memory_access_flags,
> +		sizeof(properties->memory_access_flags), 2, 7, passed);
> +
>  	cca = properties->cache_coherent & 1;
>  	cpm = properties->memory_access_flags & 1;
>  	dacs = (properties->memory_access_flags >> 1) & 1;
> diff --git a/src/acpi/lpit/lpit.c b/src/acpi/lpit/lpit.c
> index 2273ee4..eb4e3af 100644
> --- a/src/acpi/lpit/lpit.c
> +++ b/src/acpi/lpit/lpit.c
> @@ -95,14 +95,7 @@ static void lpit_check_type_0(
>  	fwts_log_nl(fw);
>  
>  	fwts_acpi_reserved_zero_check(fw, "LPIT", "Native C-state based LPI structure reserved", lpi->reserved, sizeof(lpi->reserved), passed);
> -
> -	if (lpi->flags & ~3) {
> -		*passed = false;
> -		fwts_failed(fw, LOG_LEVEL_LOW,
> -			"LPITNativeCStateLpitFlagsReserved",
> -			"Some of the Native C-state based LPI structure flags "
> -			"bits [31:2] are set, they are expected to be zero");
> -	}
> +	fwts_acpi_reserved_bits_check(fw, "LPIT", "LPI structure flags", lpi->flags, sizeof(lpi->flags), 2, 31, passed);
>  
>  	/* 2.2.1.2, if FFH, then it is a MSR, check GAS fields */
>  	if (((lpi->flags & 2) == 0) &&
> diff --git a/src/acpi/mchi/mchi.c b/src/acpi/mchi/mchi.c
> index 6244b93..8ddff2c 100644
> --- a/src/acpi/mchi/mchi.c
> +++ b/src/acpi/mchi/mchi.c
> @@ -116,14 +116,9 @@ static int mchi_test1(fwts_framework *fw)
>  			"allowed values are 0x00 (Unspecifier), 0x01 (MCTP), 0x02 (IPMI) or "
>  			"255 (OEM defined)", mchi->protocol_identifier);
>  	}
> -	if (mchi->interrupt_type & ~0x03) {
> -		passed = false;
> -		fwts_failed(fw, LOG_LEVEL_HIGH,
> -			"MCHIInterruptTypeReservedNonZero",
> -			"MCHI Interrupt Type 0x%2.2" PRIx8 " has some reserved "
> -			"bits [7:2] set, these should be all zero.",
> -			mchi->interrupt_type);
> -	}
> +
> +	fwts_acpi_reserved_bits_check(fw, "MCHI", "Interrupt Type", mchi->interrupt_type, sizeof(mchi->interrupt_type), 2, 7, &passed);
> +
>  	if (((mchi->interrupt_type & 0x01) == 0) &&
>  	    (mchi->gpe != 0)) {
>  		passed = false;
> @@ -134,14 +129,9 @@ static int mchi_test1(fwts_framework *fw)
>  			"(SCI triggered through GPE non-supported)",
>  			mchi->gpe);
>  	}
> -	if (mchi->pci_device_flag & ~0x01) {
> -		passed = false;
> -		fwts_failed(fw, LOG_LEVEL_HIGH,
> -			"MCHIPciDeviceFlagReservedNonZero",
> -			"MCHI PCI Device Flag 0x%2.2" PRIx8 " has some reserved "
> -			"bits [7:1] set, these should be all zero.",
> -			mchi->pci_device_flag);
> -	}
> +
> +	fwts_acpi_reserved_bits_check(fw, "MCHI", "PCI Device Flag", mchi->pci_device_flag, sizeof(mchi->pci_device_flag), 1, 7, &passed);
> +
>  	if (((mchi->interrupt_type & 0x02) == 0) &&
>  	    (mchi->global_system_interrupt != 0)) {
>  		passed = false;
> diff --git a/src/acpi/nfit/nfit.c b/src/acpi/nfit/nfit.c
> index fc69798..e1a4815 100644
> --- a/src/acpi/nfit/nfit.c
> +++ b/src/acpi/nfit/nfit.c
> @@ -284,14 +284,7 @@ static int nfit_test1(fwts_framework *fw)
>  			if (nfit_struct->reserved != 0)
>  				reserved_passed = nfit_struct->reserved;
>  
> -			if (nfit_struct->valid_fields & ~0x01) {
> -				passed = false;
> -				fwts_failed(fw, LOG_LEVEL_HIGH,
> -					"NFITBadValidField",
> -					"NFIT Valid Field's Bits[7..1] must be zero, got "
> -					"0x%2.2" PRIx8 " instead", nfit_struct->valid_fields);
> -			}
> -
> +			fwts_acpi_reserved_bits_check(fw, "NFIT", "Valid", nfit_struct->valid_fields, sizeof(nfit_struct->valid_fields), 1, 7, &passed);
>  			fwts_acpi_reserved_bits_check(fw, "NFIT", "NVDIMM Control Region Flags", nfit_struct->flags, sizeof(nfit_struct->flags), 1, 15, &passed);
>  
>  		} else if (entry->type == FWTS_ACPI_NFIT_TYPE_DATA_REGION) {
> diff --git a/src/acpi/pptt/pptt.c b/src/acpi/pptt/pptt.c
> index 50b7491..25f5d9a 100644
> --- a/src/acpi/pptt/pptt.c
> +++ b/src/acpi/pptt/pptt.c
> @@ -85,14 +85,7 @@ static void pptt_cache_test(fwts_framework *fw, const fwts_acpi_table_pptt_cache
>  
>  	fwts_acpi_reserved_zero_check(fw, "PPTT", "Reserved", entry->reserved, sizeof(entry->reserved), passed);
>  	fwts_acpi_reserved_bits_check(fw, "PPTT", "Flags", entry->flags, sizeof(entry->flags), 7, 31, passed);
> -
> -	if (entry->attributes & ~0x1f) {
> -		*passed = false;
> -		fwts_failed(fw, LOG_LEVEL_HIGH,
> -			"PPTTBadAttributes",
> -			"PPTT attributes's Bits[7..5] must be zero, got "
> -			"0x%2.2" PRIx8 " instead", entry->attributes);
> -	}
> +	fwts_acpi_reserved_bits_check(fw, "PPTT", "Attributes", entry->attributes, sizeof(entry->attributes), 5, 7, passed);
>  }
>  
>  static void pptt_id_test(fwts_framework *fw, const fwts_acpi_table_pptt_id *entry, bool *passed)
> diff --git a/src/acpi/spmi/spmi.c b/src/acpi/spmi/spmi.c
> index 23b9b22..86acb11 100644
> --- a/src/acpi/spmi/spmi.c
> +++ b/src/acpi/spmi/spmi.c
> @@ -123,14 +123,7 @@ static int spmi_test1(fwts_framework *fw)
>  			"0x%2.2" PRIx8 " instead", spmi->reserved1);
>  	}
>  
> -	/* Only bottom 2 bits should be set */
> -	if (spmi->interrupt_type & ~0x03) {
> -		passed = false;
> -		fwts_failed(fw, LOG_LEVEL_LOW,
> -			"SPMIInterruptTypeReservedNonZero",
> -			"SPMI Interrupt Type reserved bits [7:2] must be 0, got "
> -			"0x%2.2" PRIx8 " instead", spmi->interrupt_type);
> -	}
> +	fwts_acpi_reserved_bits_check(fw, "SPMI", "Interrupt type", spmi->interrupt_type, sizeof(spmi->interrupt_type), 2, 7, &passed);
>  
>  	/* Check for zero GPE on specific condition of interrupt type */
>  	if (((spmi->interrupt_type & 1) == 0) &&
> @@ -149,14 +142,8 @@ static int spmi_test1(fwts_framework *fw)
>  			"SPMI reserved field (offset 42) must be 0x00, got "
>  			"0x%2.2" PRIx8 " instead", spmi->reserved2);
>  	}
> -	/* Only bottom 1 bit should be set */
> -	if (spmi->pci_device_flag & ~0x01) {
> -		passed = false;
> -		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -			"SPMIPciDeviceFlag",
> -			"SPMI PCI Device Flag reserved bits [7:1] must be 0, got "
> -			"0x%2.2" PRIx8 " instead", spmi->pci_device_flag);
> -	}
> +
> +	fwts_acpi_reserved_bits_check(fw, "SPMI", "PCI device flag", spmi->pci_device_flag, sizeof(spmi->pci_device_flag), 1, 7, &passed);
>  
>  	if (((spmi->interrupt_type & 2) == 0) &&
>  	    (spmi->global_system_interrupt != 0)) {
> 
Acked-by: Colin Ian King <colin.king at canonical.com>



More information about the fwts-devel mailing list