ACK: [PATCH 1/2] acpi: check reserved fields by fwts_acpi_reserved_zero_check
ivanhu
ivan.hu at canonical.com
Mon Dec 21 07:53:25 UTC 2020
On 12/19/20 11:49 AM, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
> src/acpi/cpep/cpep.c | 16 +++++++--------
> src/acpi/dbg2/dbg2.c | 2 ++
> src/acpi/dbgp/dbgp.c | 15 +++++++++-----
> src/acpi/einj/einj.c | 4 ++--
> src/acpi/facs/facs.c | 16 ++++++---------
> src/acpi/fpdt/fpdt.c | 4 ++++
> src/acpi/hest/hest.c | 40 ++++++++++++++++++++-----------------
> src/acpi/rsdp/rsdp.c | 9 ++++-----
> src/acpi/spmi/spmi.c | 8 +-------
> src/lib/include/fwts_acpi.h | 2 +-
> 10 files changed, 59 insertions(+), 57 deletions(-)
>
> diff --git a/src/acpi/cpep/cpep.c b/src/acpi/cpep/cpep.c
> index ebd2beb3..42b09a14 100644
> --- a/src/acpi/cpep/cpep.c
> +++ b/src/acpi/cpep/cpep.c
> @@ -49,6 +49,7 @@ static int cpep_test1(fwts_framework *fw)
> {
> fwts_acpi_table_cpep *cpep = (fwts_acpi_table_cpep*)table->data;
> bool passed = true;
> + uint64_t reserved;
> uint32_t i, n;
>
> fwts_log_info_verbatim(fw, "CPEP Corrected Platform Error Polling Table:");
> @@ -58,15 +59,12 @@ static int cpep_test1(fwts_framework *fw)
> cpep->reserved[0], cpep->reserved[1], cpep->reserved[2], cpep->reserved[3],
> cpep->reserved[4], cpep->reserved[4], cpep->reserved[6], cpep->reserved[7]);
>
> - if (cpep->reserved[0] | cpep->reserved[1] |
> - cpep->reserved[2] | cpep->reserved[3] |
> - cpep->reserved[4] | cpep->reserved[5] |
> - cpep->reserved[6] | cpep->reserved[7]) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "CPEPNonZeroReserved",
> - "CPEP: Reserved field contains a non-zero value");
> - }
> + reserved = cpep->reserved[0] + ((uint64_t) cpep->reserved[1] << 8) +
> + ((uint64_t) cpep->reserved[2] << 16) + ((uint64_t) cpep->reserved[3] << 24) +
> + ((uint64_t) cpep->reserved[4] << 32) + ((uint64_t) cpep->reserved[5] << 40) +
> + ((uint64_t) cpep->reserved[6] << 48) + ((uint64_t) cpep->reserved[7] << 56);
> + fwts_acpi_reserved_zero_check(fw, "CPEP", "Reserved", reserved, sizeof(reserved), &passed);
> +
> n = (table->length - sizeof(fwts_acpi_table_cpep)) /
> sizeof(fwts_acpi_cpep_processor_info);
>
> diff --git a/src/acpi/dbg2/dbg2.c b/src/acpi/dbg2/dbg2.c
> index d904620d..d0e72fe1 100644
> --- a/src/acpi/dbg2/dbg2.c
> +++ b/src/acpi/dbg2/dbg2.c
> @@ -285,6 +285,8 @@ static int dbg2_test1(fwts_framework *fw)
> info->port_subtype);
> }
>
> + fwts_acpi_reserved_zero_check(fw, "DBG2", "Info Structure Reserved", info->reserved, sizeof(info->reserved), &passed);
> +
> length_ok = true;
> dbg2_check_offset(fw, table->length, offset + info->length,
> "DBG2 Info Structure Namespace Length", &length_ok);
> diff --git a/src/acpi/dbgp/dbgp.c b/src/acpi/dbgp/dbgp.c
> index fffa4762..1c38e161 100644
> --- a/src/acpi/dbgp/dbgp.c
> +++ b/src/acpi/dbgp/dbgp.c
> @@ -49,9 +49,10 @@ static int dbgp_init(fwts_framework *fw)
> */
> static int dbgp_test1(fwts_framework *fw)
> {
> - bool passed = true;
> - char *interface_type;
> fwts_acpi_table_dbgp *dbgp = (fwts_acpi_table_dbgp *)table->data;
> + char *interface_type;
> + bool passed = true;
> + uint32_t reserved;
>
> if (!fwts_acpi_table_length_check(fw, "DBGP", table->length, sizeof(fwts_acpi_table_dbgp))) {
> passed = false;
> @@ -70,12 +71,13 @@ static int dbgp_test1(fwts_framework *fw)
> break;
> }
>
> + reserved = dbgp->reserved[0] + ((uint32_t) dbgp->reserved[1] << 8) +
> + ((uint32_t) dbgp->reserved[2] << 16);
> +
> fwts_log_info_verbatim(fw, "DBGP Table:");
> fwts_log_info_verbatim(fw, " Interface Type 0x%2.2" PRIx8 " (%s)",
> dbgp->interface_type, interface_type);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, dbgp->reserved1[0]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, dbgp->reserved1[1]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, dbgp->reserved1[2]);
> + fwts_log_info_verbatim(fw, " Reserved: 0x%8.8" PRIx32, reserved);
> fwts_log_info_verbatim(fw, " Base Address:");
> fwts_log_info_verbatim(fw, " Address Space ID: 0x%2.2" PRIx8, dbgp->base_address.address_space_id);
> fwts_log_info_verbatim(fw, " Register Bit Width 0x%2.2" PRIx8, dbgp->base_address.register_bit_width);
> @@ -93,6 +95,9 @@ static int dbgp_test1(fwts_framework *fw)
> " 0x00 (Full 16550) or 0x01 (16550 subset)",
> dbgp->interface_type);
> }
> +
> + fwts_acpi_reserved_zero_check(fw, "DBGP", "Reserved", reserved, sizeof(reserved), &passed);
> +
> if (dbgp->base_address.register_bit_width == 0) {
> passed = false;
> fwts_failed(fw, LOG_LEVEL_HIGH,
> diff --git a/src/acpi/einj/einj.c b/src/acpi/einj/einj.c
> index 02dd4e15..e915c110 100644
> --- a/src/acpi/einj/einj.c
> +++ b/src/acpi/einj/einj.c
> @@ -50,8 +50,8 @@ static int einj_test1(fwts_framework *fw)
> uint32_t reserved, i;
> bool passed = true;
>
> - reserved = einj->reserved[0] + (einj->reserved[1] << 4) +
> - (einj->reserved[2] << 8);
> + reserved = einj->reserved[0] + ((uint32_t) einj->reserved[1] << 8) +
> + ((uint32_t) einj->reserved[2] << 16);
>
> fwts_log_info_verbatim(fw, "EINJ Error Injection Table:");
> fwts_log_info_verbatim(fw, " Injection Header Size: 0x%8.8" PRIx32,
> diff --git a/src/acpi/facs/facs.c b/src/acpi/facs/facs.c
> index 7dcffc3f..340771fd 100644
> --- a/src/acpi/facs/facs.c
> +++ b/src/acpi/facs/facs.c
> @@ -49,6 +49,7 @@ static int facs_test1(fwts_framework *fw)
> {
> fwts_acpi_table_facs *facs = (fwts_acpi_table_facs*)table->data;
> bool passed = true;
> + uint32_t reserved;
> int i;
>
> if (table->length < 64) {
> @@ -61,6 +62,9 @@ static int facs_test1(fwts_framework *fw)
> goto done;
> }
>
> + reserved = facs->reserved[0] + ((uint32_t) facs->reserved[1] << 8) +
> + ((uint32_t) facs->reserved[2] << 16);
> +
> fwts_log_info_verbatim(fw, "FACS Firmware ACPI Control Structure:");
> fwts_log_info_verbatim(fw, " Signature: '%4.4s'", facs->signature);
> fwts_log_info_verbatim(fw, " Length: 0x%8.8" PRIx32, facs->length);
> @@ -70,8 +74,7 @@ static int facs_test1(fwts_framework *fw)
> fwts_log_info_verbatim(fw, " Flags: 0x%8.8" PRIx32, facs->flags);
> fwts_log_info_verbatim(fw, " X-Firmware Waking Vector: 0x%16.16" PRIx64, facs->x_firmware_waking_vector);
> fwts_log_info_verbatim(fw, " Version: 0x%2.2" PRIx8, facs->version);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8 " 0x%2.2" PRIx8 " 0x%2.2" PRIx8,
> - facs->reserved[0], facs->reserved[1], facs->reserved[2]);
> + fwts_log_info_verbatim(fw, " Reserved: 0x%8.8" PRIx32, reserved);
> fwts_log_info_verbatim(fw, " OSPM Flags: 0x%8.8" PRIx32, facs->ospm_flags);
> for (i = 0; i < 24; i+= 4) {
> fwts_log_info_verbatim(fw, " Reserved: "
> @@ -122,15 +125,8 @@ static int facs_test1(fwts_framework *fw)
> " and should be at least 64",
> facs->length);
> }
> - if (facs->reserved[0] |
> - facs->reserved[1] |
> - facs->reserved[2]) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_LOW,
> - "FACSInvalidReserved1",
> - "FACS: 1st Reserved field is non-zero");
> - }
>
> + fwts_acpi_reserved_zero_check(fw, "FACS", "Reserved", reserved, sizeof(reserved), &passed);
> fwts_acpi_reserved_bits_check(fw, "FACS", "Flags", facs->flags, sizeof(facs->flags), 2, 31, &passed);
> fwts_acpi_reserved_bits_check(fw, "FACS", "OSPM Flags", facs->ospm_flags, sizeof(facs->ospm_flags), 1, 31, &passed);
>
> diff --git a/src/acpi/fpdt/fpdt.c b/src/acpi/fpdt/fpdt.c
> index a96341b2..f5d0457b 100644
> --- a/src/acpi/fpdt/fpdt.c
> +++ b/src/acpi/fpdt/fpdt.c
> @@ -124,6 +124,8 @@ static int fpdt_test1(fwts_framework *fw)
> fwts_log_info_verbatim(fw, " Reserved: 0x%8.8" PRIx32, fbbpr->reserved);
> fwts_log_info_verbatim(fw, " FBPT Pointer: 0x%16.16" PRIx64, fbbpr->fbpt_addr);
>
> + fwts_acpi_reserved_zero_check(fw, "FPDT", "Reserved", fbbpr->reserved, sizeof(fbbpr->reserved), &passed);
> +
> /*
> * For the moment, only dump the 64-bit processor-relative physical address
> * of the Firmware Basic Boot Performance Table, should also get and check
> @@ -155,6 +157,8 @@ static int fpdt_test1(fwts_framework *fw)
> fwts_log_info_verbatim(fw, " Reserved: 0x%8.8" PRIx32, s3ptpr->reserved);
> fwts_log_info_verbatim(fw, " S3PT Pointer: 0x%16.16" PRIx64, s3ptpr->s3pt_addr);
>
> + fwts_acpi_reserved_zero_check(fw, "FPDT", "Reserved", s3ptpr->reserved, sizeof(s3ptpr->reserved), &passed);
> +
> /*
> * For the moment, only dump 64-bit processor-relative physical
> * address of the S3 Performance Table, should also get and check
> diff --git a/src/acpi/hest/hest.c b/src/acpi/hest/hest.c
> index 96314136..fdc46446 100644
> --- a/src/acpi/hest/hest.c
> +++ b/src/acpi/hest/hest.c
> @@ -53,6 +53,7 @@ static void hest_check_ia32_arch_machine_check_exception(
> bool *passed)
> {
> ssize_t i, total_size;
> + uint64_t reserved2;
>
> fwts_acpi_table_hest_ia32_machine_check_exception *exception =
> (fwts_acpi_table_hest_ia32_machine_check_exception *)*data;
> @@ -79,6 +80,11 @@ static void hest_check_ia32_arch_machine_check_exception(
> return;
> }
>
> + reserved2 = exception->reserved2[0] + ((uint64_t) exception->reserved2[1] << 8) +
> + ((uint64_t) exception->reserved2[2] << 16) + ((uint64_t) exception->reserved2[3] << 24) +
> + ((uint64_t) exception->reserved2[4] << 32) + ((uint64_t) exception->reserved2[5] << 40) +
> + ((uint64_t) exception->reserved2[6] << 48);
> +
> fwts_log_info_verbatim(fw, "HEST IA-32 Architecture Machine Check Exception:");
> fwts_log_info_verbatim(fw, " Type: 0x%2.2" PRIx8, exception->type);
> fwts_log_info_verbatim(fw, " Source ID: 0x%4.4" PRIx16, exception->source_id);
> @@ -90,15 +96,11 @@ static void hest_check_ia32_arch_machine_check_exception(
> fwts_log_info_verbatim(fw, " Global Capability Data: 0x%16.16" PRIx64, exception->global_capability_init_data);
> fwts_log_info_verbatim(fw, " Global Control Data: 0x%16.16" PRIx64, exception->global_control_init_data);
> fwts_log_info_verbatim(fw, " Number of Hardware Banks: 0x%8.8" PRIx32, exception->number_of_hardware_banks);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, exception->reserved2[0]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, exception->reserved2[1]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, exception->reserved2[2]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, exception->reserved2[3]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, exception->reserved2[4]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, exception->reserved2[5]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, exception->reserved2[6]);
> + fwts_log_info_verbatim(fw, " Reserved: 0x%16.16" PRIx64, reserved2);
> fwts_log_nl(fw);
>
> + fwts_acpi_reserved_zero_check(fw, "HEST", "MCE Reserved1", exception->reserved1, sizeof(exception->reserved1), passed);
> +
> if (exception->flags & ~0x5) {
> fwts_failed(fw, LOG_LEVEL_MEDIUM,
> "HESTIA32FlagsReserved",
> @@ -115,6 +117,8 @@ static void hest_check_ia32_arch_machine_check_exception(
> *passed = false;
> }
>
> + fwts_acpi_reserved_zero_check(fw, "HEST", "MCE Reserved2", reserved2, sizeof(reserved2), passed);
> +
> for (i = 0; i < exception->number_of_hardware_banks; i++) {
> fwts_acpi_table_hest_machine_check_bank *bank = &exception->bank[i];
>
> @@ -165,6 +169,7 @@ static void hest_check_ia32_arch_corrected_machine_check(
> bool *passed)
> {
> ssize_t i, total_size;
> + uint32_t reserved2;
>
> fwts_acpi_table_hest_ia32_corrected_machine_check *check =
> (fwts_acpi_table_hest_ia32_corrected_machine_check *)*data;
> @@ -191,6 +196,9 @@ static void hest_check_ia32_arch_corrected_machine_check(
> return;
> }
>
> + reserved2 = check->reserved2[0] + ((uint32_t) check->reserved2[1] << 8) +
> + ((uint32_t) check->reserved2[2] << 16);
> +
> fwts_log_info_verbatim(fw, "HEST IA-32 Architecture Machine Check:");
> fwts_log_info_verbatim(fw, " Type: 0x%2.2" PRIx8, check->type);
> fwts_log_info_verbatim(fw, " Source ID: 0x%4.4" PRIx16, check->source_id);
> @@ -217,11 +225,11 @@ static void hest_check_ia32_arch_corrected_machine_check(
> fwts_log_info_verbatim(fw, " Error: Thresh. Window: 0x%4.4" PRIx16,
> check->notification.error_threshold_window);
> fwts_log_info_verbatim(fw, " Number of Hardware Banks: 0x%8.8" PRIx32, check->number_of_hardware_banks);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, check->reserved2[0]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, check->reserved2[1]);
> - fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, check->reserved2[2]);
> + fwts_log_info_verbatim(fw, " Reserved: 0x%8.8" PRIx32, reserved2);
> fwts_log_nl(fw);
>
> + fwts_acpi_reserved_zero_check(fw, "HEST", "Machine Check Reserved1", check->reserved1, sizeof(check->reserved1), passed);
> +
> if (check->flags & ~0x5) {
> fwts_failed(fw, LOG_LEVEL_MEDIUM,
> "HESTIA32CorrectedMachineCheckFlagsReserved",
> @@ -248,6 +256,8 @@ static void hest_check_ia32_arch_corrected_machine_check(
> check->notification.type);
> }
>
> + fwts_acpi_reserved_zero_check(fw, "HEST", "Machine Check Reserved2", reserved2, sizeof(reserved2), passed);
> +
> for (i = 0; i < check->number_of_hardware_banks; i++) {
> fwts_acpi_table_hest_machine_check_bank *bank = &check->bank[i];
>
> @@ -323,14 +333,8 @@ static void hest_check_acpi_table_hest_nmi_error(
> fwts_log_info_verbatim(fw, " Max Raw Data Length: 0x%8.8" PRIx32, err->max_raw_data_length);
> fwts_log_nl(fw);
>
> - if (err->reserved1) {
> - *passed = false;
> - fwts_failed(fw, LOG_LEVEL_LOW,
> - "HESTInvalidRecordsToPreallocate",
> - "HEST IA-32 Architecture NMI Reserved field "
> - "at offset 4 must be zero, instead got 0x%" PRIx16,
> - err->reserved1);
> - }
> + fwts_acpi_reserved_zero_check(fw, "HEST", "NMI Reserved", err->reserved1, sizeof(err->reserved1), passed);
> +
> if (err->number_of_records_to_preallocate < 1) {
> *passed = false;
> fwts_failed(fw, LOG_LEVEL_HIGH,
> diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c
> index c7985cdf..3e1a1544 100644
> --- a/src/acpi/rsdp/rsdp.c
> +++ b/src/acpi/rsdp/rsdp.c
> @@ -205,11 +205,10 @@ static int rsdp_test1(fwts_framework *fw)
> value = 0;
> for (ptr = (uint8_t *)rsdp->reserved, i = 0; i < 3; i++)
> value += *ptr++;
> - if (value)
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "RSDPReserved",
> - "RSDP: reserved field is non-zero.");
> - else
> +
> + passed = true;
> + fwts_acpi_reserved_zero_check(fw, "RSDP", "Reserved", value, sizeof(value), &passed);
> + if (passed)
> fwts_passed(fw, "RSDP: the reserved field is zero.");
>
> return FWTS_OK;
> diff --git a/src/acpi/spmi/spmi.c b/src/acpi/spmi/spmi.c
> index 2dce7e06..5db4f670 100644
> --- a/src/acpi/spmi/spmi.c
> +++ b/src/acpi/spmi/spmi.c
> @@ -130,14 +130,8 @@ static int spmi_test1(fwts_framework *fw)
> "Type bit 0 (SCI triggered through GPE) is set to 0",
> spmi->gpe);
> }
> - if (spmi->reserved2 != 0) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_LOW,
> - "SPMIReserved1NonZero",
> - "SPMI reserved field (offset 42) must be 0x00, got "
> - "0x%2.2" PRIx8 " instead", spmi->reserved2);
> - }
>
> + fwts_acpi_reserved_zero_check(fw, "SPMI", "Reserved2", spmi->reserved2, sizeof(spmi->reserved2), &passed);
> 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) &&
> diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h
> index 3015364c..3de50040 100644
> --- a/src/lib/include/fwts_acpi.h
> +++ b/src/lib/include/fwts_acpi.h
> @@ -1585,7 +1585,7 @@ typedef struct {
> typedef struct {
> fwts_acpi_table_header header;
> uint8_t interface_type;
> - uint8_t reserved1[3];
> + uint8_t reserved[3];
> fwts_acpi_gas base_address;
> } __attribute__ ((packed)) fwts_acpi_table_dbgp;
>
>
Acked-by: Ivan Hu <ivan.hu at canonical.com>
More information about the fwts-devel
mailing list