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