[PATCH] acpi: pcct: add ACPI PCCT test
Colin Ian King
colin.king at canonical.com
Fri Jul 15 09:13:33 UTC 2016
On 11/07/16 03:45, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
> src/Makefile.am | 1 +
> src/acpi/pcct/pcct.c | 277 ++++++++++++++++++++++++++++++++++++++++++++
> src/lib/include/fwts_acpi.h | 35 +++++-
> 3 files changed, 312 insertions(+), 1 deletion(-)
> create mode 100644 src/acpi/pcct/pcct.c
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 100eaa3..11c65b2 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -75,6 +75,7 @@ fwts_SOURCES = main.c \
> acpi/method/method.c \
> acpi/osilinux/osilinux.c \
> acpi/pcc/pcc.c \
> + acpi/pcct/pcct.c \
> acpi/powerbutton/powerbutton.c \
> acpi/plddump/plddump.c \
> acpi/rsdp/rsdp.c \
> diff --git a/src/acpi/pcct/pcct.c b/src/acpi/pcct/pcct.c
> new file mode 100644
> index 0000000..eb5f181
> --- /dev/null
> +++ b/src/acpi/pcct/pcct.c
> @@ -0,0 +1,277 @@
> +/*
> + * Copyright (C) 2016 Canonical
> + *
> + * This program is free software; you can redistribute it and/or
^ leading whitespace needs removing
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include "fwts.h"
> +
> +#if defined(FWTS_HAS_ACPI)
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <inttypes.h>
> +#include <stdbool.h>
> +
> +static fwts_acpi_table_info *table;
> +
> +static int pcct_init(fwts_framework *fw)
> +{
> + if (fwts_acpi_find_table(fw, "PCCT", 0, &table) != FWTS_OK) {
> + fwts_log_error(fw, "Cannot load ACPI table");
> + return FWTS_ERROR;
> + }
> + if (table == NULL) {
> + fwts_log_error(fw, "ACPI PCCT table does not exist, skipping test");
> + return FWTS_ERROR;
> + }
> +
> + return FWTS_OK;
> +}
> +
> +static int pcct_test1(fwts_framework *fw)
> +{
> + fwts_acpi_table_pcct *pcct = (fwts_acpi_table_pcct*) table->data;
> + fwts_acpi_table_pcct_subspace_header *pcct_sub;
> + size_t offset;
> + bool passed = true;
> +
> + fwts_log_info_verbatim(fw, "PCC Table:");
> + fwts_log_info_verbatim(fw, " Flags: 0x%8.8" PRIx32, pcct->flags);
> + fwts_log_info_verbatim(fw, " Reserved: 0x%16.16" PRIx64, pcct->reserved);
> + fwts_log_nl(fw);
> +
> + if ((pcct->flags & ~0x01) != 0) {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTReservedNonZero",
> + "PCCT flags field's bit 1..31 be zero, got "
> + "0x%8.8" PRIx32 " instead", pcct->flags);
> + }
> +
> + if (pcct->reserved != 0) {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_LOW,
> + "PCCTReservedNonZero",
> + "PCCT reserved field must be zero, got "
> + "0x%8.8" PRIx64 " instead", pcct->reserved);
> + }
> +
> + offset = sizeof(fwts_acpi_table_pcct);
> + pcct_sub = (fwts_acpi_table_pcct_subspace_header *) (table->data + offset);
> +
> + while (offset < table->length) {
> + uint8_t doorbell_int_flags = 0;
> + uint64_t addr_length = 8;
> +
> + fwts_log_info_verbatim(fw, " PCC Subspace Structure:");
> + fwts_log_info_verbatim(fw, " Type: 0x%2.2" PRIx8, pcct_sub->type);
> + fwts_log_info_verbatim(fw, " Length: 0x%2.2" PRIx8, pcct_sub->length);
> +
> + if (pcct_sub->type == 0) {
I'd prefer a switch statement on pcct_sub->type rather than a chain of
if statements. But that's just a style choice.
> + fwts_acpi_table_pcct_subspace_type_0 *subspace = (fwts_acpi_table_pcct_subspace_type_0 *) pcct_sub;
> + fwts_acpi_gas gas = subspace->doorbell_register;
Copying gas may be problematic because it could offset into subspace and
the subspace structure could be smaller than expected because of a
corrupt table. Rather than a copy, just dereference it, e.g.
fwts_acpi_gas *gas = &subspace->doorbell_register;
> + uint64_t reserved;
> +
> + if (pcct_sub->length != 62) {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTBadSubpaceLength",
> + "PCCT Subspace Type 0 must have length = 62, got "
> + "0x%2.2" PRIx8 " instead", pcct_sub->length);
> + }
If the length is not 62 should we be validating the rest of the
structure, especially if the length is shorter than 62 bytes? I'd just
skip over the field checking if the length is too short.
> +
> + reserved = subspace->reserved[0] + subspace->reserved[1] + subspace->reserved[2] + subspace->reserved[3] + subspace->reserved[4] + subspace->reserved[5];
should reserved be:
reserved = subspace->reserved[0] |
(subspace->reserved[1] << 8) |
(subspace->reserved[2] << 16) |
(subspace->reserved[3] << 24) |
(subspace->reserved[4] << 32) |
(subspace->reserved[5] << 40);
> + addr_length = subspace->length;
> +
> + fwts_log_info_verbatim(fw, " Reserved: 0x%16.16" PRIx64, reserved);
> + fwts_log_info_verbatim(fw, " Base Address: 0x%16.16" PRIx64, subspace->base_address);
> + fwts_log_info_verbatim(fw, " Length: 0x%16.16" PRIx64, subspace->length);
> + fwts_log_info_verbatim(fw, " Doorbell Register:");
> + 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);
..and reference gas fields as gas->address_space_id etc (see *gas notes
earlier)
The alignment of the field names is good on the GAS fields, but the 0x
hex fields should line up with all other hex fields, these look like
they are 2 chars too far to the right.
> + fwts_log_info_verbatim(fw, " Doorbell Preserve: 0x%16.16" PRIx64, subspace->doorbell_preserve);
> + fwts_log_info_verbatim(fw, " Doorbell Write: 0x%16.16" PRIx64, subspace->doorbell_write);
> + fwts_log_info_verbatim(fw, " Nominal Latency: 0x%8.8" PRIx32, subspace->nominal_latency);
> + fwts_log_info_verbatim(fw, " Max Periodic Access Rate: 0x%8.8" PRIx32, subspace->max_periodic_access_rate);
> + fwts_log_info_verbatim(fw, " Min Request Turnaround Time: 0x%8.8" PRIx32, subspace->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 Space or System Memory Space",
> + gas.address_space_id);
> + }
> +
> + } else if (pcct_sub->type == 1) {
> + fwts_acpi_table_pcct_subspace_type_1 *subspace = (fwts_acpi_table_pcct_subspace_type_1 *) pcct_sub;
> + fwts_acpi_gas gas = subspace->doorbell_register;
again, same as above about dereferencing gas rather than copying it.
> +
> + if (pcct_sub->length != 62) {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTBadSubpaceLength",
> + "PCCT Subspace Type 1 must have length = 62, got "
> + "0x%2.2" PRIx8 " instead", pcct_sub->length);
> + }
again, watch out for short sub table lengths, skip the dumping of it if
less than 62 bytes
> +
> + addr_length = subspace->length;
> + doorbell_int_flags = subspace->doorbell_interrupt_flags;
> +
> + fwts_log_info_verbatim(fw, " Doorbell Interrupt: 0x%8.8" PRIx32, subspace->doorbell_interrupt);
> + fwts_log_info_verbatim(fw, " Doorbell Interrupt Flags: 0x%2.2" PRIx8, subspace->doorbell_interrupt_flags);
> + fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, subspace->reserved);
> + fwts_log_info_verbatim(fw, " Base Address: 0x%16.16" PRIx64, subspace->base_address);
> + fwts_log_info_verbatim(fw, " Length: 0x%16.16" PRIx64, subspace->length);
> + fwts_log_info_verbatim(fw, " Doorbell Register:");
> + 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);
The alignment of the field names is good on the GAS fields, but the 0x
hex fields should line up with all other hex fields, these look like
they are 2 chars too far to the right.
> + fwts_log_info_verbatim(fw, " Doorbell Preserve: 0x%16.16" PRIx64, subspace->doorbell_preserve);
> + fwts_log_info_verbatim(fw, " Doorbell Write: 0x%16.16" PRIx64, subspace->doorbell_write);
> + fwts_log_info_verbatim(fw, " Nominal Latency: 0x%8.8" PRIx32, subspace->nominal_latency);
> + fwts_log_info_verbatim(fw, " Max Periodic Access Rate: 0x%8.8" PRIx32, subspace->max_periodic_access_rate);
> + fwts_log_info_verbatim(fw, " Min Request Turnaround Time: 0x%8.8" PRIx32, subspace->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) &&
> + (gas.address_space_id != FWTS_GAS_ADDR_SPACE_ID_FFH)) {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTSubspaceInvalidAddrSpaceID",
> + "PCCT Subspace Type 1 has space ID = 0x%2.2" PRIx8
> + " which is not System I/O Space or System Memory Space",
> + gas.address_space_id);
> + }
> +
> + } else if (pcct_sub->type == 2) {
> + fwts_acpi_table_pcct_subspace_type_2 *subspace = (fwts_acpi_table_pcct_subspace_type_2 *) pcct_sub;
> + fwts_acpi_gas gas = subspace->doorbell_register;
> + fwts_acpi_gas gas2 = subspace->doorbell_ack_register;
again, same as above about dereferencing gas and gas2 rather than
copying it.
> +
> + if (pcct_sub->length != 90) {
again, watch out for short sub table lengths, skip the dumping of it if
less than 90 bytes
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTBadSubpaceLength",
> + "PCCT Subspace Type 2 must have length = 90, got "
> + "0x%2.2" PRIx8 " instead", pcct_sub->length);
> + }
> +
> + addr_length = subspace->length;
> + doorbell_int_flags = subspace->doorbell_interrupt_flags;
> +
> + fwts_log_info_verbatim(fw, " Doorbell Interrupt: 0x%8.8" PRIx32, subspace->doorbell_interrupt);
> + fwts_log_info_verbatim(fw, " Doorbell Interrupt Flags: 0x%2.2" PRIx8, subspace->doorbell_interrupt_flags);
> + fwts_log_info_verbatim(fw, " Reserved: 0x%2.2" PRIx8, subspace->reserved);
> + fwts_log_info_verbatim(fw, " Base Address: 0x%16.16" PRIx64, subspace->base_address);
> + fwts_log_info_verbatim(fw, " Length: 0x%16.16" PRIx64, subspace->length);
> + fwts_log_info_verbatim(fw, " Doorbell Register:");
> + 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);
The alignment of the field names is good on the GAS fields, but the 0x
hex fields should line up with all other hex fields, these look like
they are 2 chars too far to the right.
> + fwts_log_info_verbatim(fw, " Doorbell Preserve: 0x%16.16" PRIx64, subspace->doorbell_preserve);
> + fwts_log_info_verbatim(fw, " Doorbell Write: 0x%16.16" PRIx64, subspace->doorbell_write);
> + fwts_log_info_verbatim(fw, " Nominal Latency: 0x%8.8" PRIx32, subspace->nominal_latency);
> + fwts_log_info_verbatim(fw, " Max Periodic Access Rate: 0x%8.8" PRIx32, subspace->max_periodic_access_rate);
> + fwts_log_info_verbatim(fw, " Min Request Turnaround Time: 0x%8.8" PRIx32, subspace->min_request_turnaround_time);
> + fwts_log_info_verbatim(fw, " Doorbell Ack Register:");
> + fwts_log_info_verbatim(fw, " Address Space ID 0x%2.2" PRIx8, gas2.address_space_id);
> + fwts_log_info_verbatim(fw, " Register Bit Width 0x%2.2" PRIx8, gas2.register_bit_width);
> + fwts_log_info_verbatim(fw, " Register Bit Offset 0x%2.2" PRIx8, gas2.register_bit_offset);
> + fwts_log_info_verbatim(fw, " Access Size 0x%2.2" PRIx8, gas2.access_width);
> + fwts_log_info_verbatim(fw, " Address 0x%16.16" PRIx64, gas2.address);
The alignment of the field names is good on the GAS fields, but the 0x
hex fields should line up with all other hex fields, these look like
they are 2 chars too far to the right.
> + fwts_log_info_verbatim(fw, " Doorbell Ack Preserve: 0x%16.16" PRIx64, subspace->doorbell_ack_preserve);
> + fwts_log_info_verbatim(fw, " Doorbell Ack Write: 0x%16.16" PRIx64, subspace->doorbell_ack_write);
> +
> + 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 2 has space ID = 0x%2.2" PRIx8
> + " which is not System I/O Space or System Memory Space",
> + gas.address_space_id);
> + }
> +
> + if ((gas2.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_IO) &&
> + (gas2.address_space_id != FWTS_GAS_ADDR_SPACE_ID_SYSTEM_MEMORY) &&
> + (gas2.address_space_id != FWTS_GAS_ADDR_SPACE_ID_FFH)) {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTSubspaceInvalidAddrSpaceID",
> + "PCCT Subspace Type 2 has space ID = 0x%2.2" PRIx8
> + " which is not System I/O Space or System Memory Space",
> + gas.address_space_id);
> + }
> +
> + } else {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTBadSubType",
> + "PCCT Subspace Structure supports type 0..2, got "
> + "0x%2.2" PRIx8 " instead", pcct_sub->type);
> + }
> +
> + if (addr_length <= 8) {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTBadSubtypeMemoryLength",
> + "PCCT Subspace Structure must have memory length > 8, got "
> + "0x%16.16" PRIx64 " instead", addr_length);
> + }
> +
> + if (doorbell_int_flags & ~0x3) {
> + passed = false;
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "PCCTBadSubtypeDoorbellFlags",
> + "PCCT Subspace Doorbell Interrupt's bit 2..7 be zero, got "
> + "0x%2.2" PRIx8 " instead", doorbell_int_flags);
> + }
> +
> + fwts_log_nl(fw);
> +
> + offset += pcct_sub->length;
> + pcct_sub = (fwts_acpi_table_pcct_subspace_header *) (table->data + offset);
> + }
> +
> + if (passed)
> + fwts_passed(fw, "No issues found in PCC table.");
> +
> + return FWTS_OK;
> +}
> +
> +static fwts_framework_minor_test pcct_tests[] = {
> + { pcct_test1, "Validate PCC table." },
> + { NULL, NULL }
> +};
> +
> +static fwts_framework_ops pcct_ops = {
> + .description = "Platform Communications Channel test.",
> + .init = pcct_init,
> + .minor_tests = pcct_tests
> +};
> +
> +FWTS_REGISTER("pcct", &pcct_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI)
> +
> +#endif
> diff --git a/src/lib/include/fwts_acpi.h b/src/lib/include/fwts_acpi.h
> index a3a5701..864fff6 100644
> --- a/src/lib/include/fwts_acpi.h
> +++ b/src/lib/include/fwts_acpi.h
> @@ -995,7 +995,7 @@ typedef struct {
> typedef struct {
> fwts_acpi_table_header header;
> uint32_t flags;
> - uint8_t reserved[8];
> + uint64_t reserved;
> } __attribute__ ((packed)) fwts_acpi_table_pcct;
>
> typedef struct {
> @@ -1016,6 +1016,39 @@ typedef struct {
> uint16_t min_request_turnaround_time;
> } __attribute__ ((packed)) fwts_acpi_table_pcct_subspace_type_0;
>
> +typedef struct {
> + fwts_acpi_table_pcct_subspace_header header;
> + uint32_t doorbell_interrupt;
> + uint8_t doorbell_interrupt_flags;
> + uint8_t reserved;
> + uint64_t base_address;
> + uint64_t length;
> + fwts_acpi_gas doorbell_register;
> + uint64_t doorbell_preserve;
> + uint64_t doorbell_write;
> + uint32_t nominal_latency;
> + uint32_t max_periodic_access_rate;
> + uint16_t min_request_turnaround_time;
> +} __attribute__ ((packed)) fwts_acpi_table_pcct_subspace_type_1;
> +
> +typedef struct {
> + fwts_acpi_table_pcct_subspace_header header;
> + uint32_t doorbell_interrupt;
> + uint8_t doorbell_interrupt_flags;
> + uint8_t reserved;
> + uint64_t base_address;
> + uint64_t length;
> + fwts_acpi_gas doorbell_register;
> + uint64_t doorbell_preserve;
> + uint64_t doorbell_write;
> + uint32_t nominal_latency;
> + uint32_t max_periodic_access_rate;
> + uint16_t min_request_turnaround_time;
> + fwts_acpi_gas doorbell_ack_register;
> + uint64_t doorbell_ack_preserve;
> + uint64_t doorbell_ack_write;
> +} __attribute__ ((packed)) fwts_acpi_table_pcct_subspace_type_2;
> +
> /*
> * ACPI SPCR (Serial Port Console Redirection Table)
> * http://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
>
More information about the fwts-devel
mailing list