[PATCH 1/2] acpi: add support for FPDT table (LP: #1476954)
ivanhu
ivan.hu at canonical.com
Thu Jul 23 14:39:57 UTC 2015
On 2015年07月23日 18:17, Colin Ian King wrote:
> On 23/07/15 08:16, Ivan Hu wrote:
>> Firmware Performance Data Table (FPDT) table defined after ACPI spec v5.0, 5.2.23
>>
>> Signed-off-by: Ivan Hu <ivan.hu at canonical.com>
>> ---
>> src/Makefile.am | 1 +
>> src/acpi/fpdt/fpdt.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 214 insertions(+)
>> create mode 100644 src/acpi/fpdt/fpdt.c
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 3e7fc3c..5d5a5ca 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -50,6 +50,7 @@ fwts_SOURCES = main.c \
>> acpi/facs/facs.c \
>> acpi/fadt/fadt.c \
>> acpi/fan/fan.c \
>> + acpi/fpdt/fpdt.c \
>> acpi/gpedump/gpedump.c \
>> acpi/gtdt/gtdt.c \
>> acpi/hest/hest.c \
>> diff --git a/src/acpi/fpdt/fpdt.c b/src/acpi/fpdt/fpdt.c
>> new file mode 100644
>> index 0000000..f5a0118
>> --- /dev/null
>> +++ b/src/acpi/fpdt/fpdt.c
>> @@ -0,0 +1,213 @@
>> +/*
>> + * Copyright (C) 2015 Canonical
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
>> + *
>> + */
>> +#include "fwts.h"
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <inttypes.h>
>> +#include <string.h>
>> +#include <ctype.h>
>> +
>> +fwts_acpi_table_info *table;
>> +
>> +static int fpdt_init(fwts_framework *fw)
>> +{
>> +
>> + if (fwts_acpi_find_table(fw, "FPDT", 0, &table) != FWTS_OK) {
>> + fwts_log_error(fw, "Cannot read ACPI tables.");
>> + return FWTS_ERROR;
>> + }
>> + if (table == NULL || (table && table->length == 0)) {
>> + fwts_log_error(fw, "ACPI FPDT table does not exist, skipping test");
>> + return FWTS_SKIP;
>> + }
>> +
>> + return FWTS_OK;
>> +}
>> +
>> +static void fpdt_rec_header_dump(
>> + fwts_framework *fw,
>> + const char *type_name,
>> + fwts_acpi_table_fpdt_header *header)
>> +{
>> + fwts_log_info_verbatum(fw, " %s:", type_name);
>> + fwts_log_info_verbatum(fw, " Perf Rec Type: 0x%4.4" PRIx16, header->type);
>> + fwts_log_info_verbatum(fw, " Rec Length: 0x%2.2" PRIx8, header->length);
>> + fwts_log_info_verbatum(fw, " Revision: 0x%2.2" PRIx8, header->revision);
>> +}
>> +
>> +static void fpdt_dump_raw_data(
>> + fwts_framework *fw,
>> + const uint8_t *data,
>> + const size_t length,
>> + const size_t offset)
>> +{
>> + size_t n;
>> +
>> + fwts_log_info_verbatum(fw, " Data:");
>> + for (n = 0; n < length; n+=16) {
>> + int left = length - n;
>> + char buffer[128];
>> + fwts_dump_raw_data(buffer, sizeof(buffer), data + n, n + offset, left > 16 ? 16 : left);
>> + fwts_log_info_verbatum(fw, "%s", buffer);
>> + }
>> +}
>> +
>> +static int fpdt_test1(fwts_framework *fw)
>> +{
>> + uint8_t *data = (uint8_t *)table->data, *ptr = data;
>> + fwts_acpi_table_header *header= (fwts_acpi_table_header *)table->data;
>> + bool passed = true;
>> + const size_t fpdt_hdr_len = sizeof(fwts_acpi_table_fpdt_header);
>> +
>> + /* Size sanity, got enough table to get initial header */
>> + if (table->length < (sizeof(fwts_acpi_table_header) + sizeof(fwts_acpi_table_fpdt_header))) {
>> + passed = false;
>> + fwts_failed(fw, LOG_LEVEL_HIGH,
>> + "FPDTTooShort",
>> + "FPDT table too short, must be at least %zu bytes, "
>> + "instead got %zu bytes",
>> + sizeof(fwts_acpi_table_header) + sizeof(fwts_acpi_table_fpdt_header),
>> + table->length);
>> + goto done;
>> + }
>> +
>> + if (header->revision != 1) {
>> + passed = false;
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> + "FPDTBabRevision",
> FPDTBabRevision should be FPDTBadRevsion
>> + "FPDT revision is incorrect, expecting 0x01,"
>> + " instead got 0x%2.2" PRIx8,
>> + header->revision);
>> + }
>> +
>> + ptr += sizeof(fwts_acpi_table_header);
>> + while (ptr < data + table->length) {
> what happens if table->length is sizeof(fwts_acpi_table_header) + 1? I
> believe we fall off the end of the table when we access fpdt->type. So....
have checked the
table->length > (sizeof(fwts_acpi_table_header) + sizeof(fwts_acpi_table_fpdt_header) above,
table->length shouldn't equal sizeof(fwts_acpi_table_header) + 1 here
>> + fwts_acpi_table_fpdt_header *fpdt = (fwts_acpi_table_fpdt_header *)ptr;
>> + fwts_acpi_table_fpdt_basic_boot_perf_ptr *fbbpr = (fwts_acpi_table_fpdt_basic_boot_perf_ptr *)ptr;
>> + fwts_acpi_table_fpdt_s3_perf_ptr *s3ptpr = (fwts_acpi_table_fpdt_s3_perf_ptr *)ptr;
>
> ...here we need to see if (ptr + sizeof(fwts_acpi_table_fpdt_header) <=
> data + table->length) to see if the fpdt header is fully available
> before we access fpdt->type.
>
>> + switch (fpdt->type) {
>> + case 0x0000: /* Firmware Basic Boot Performance Pointer Record */
>> + if (fbbpr->fpdt.length != sizeof(fwts_acpi_table_fpdt_basic_boot_perf_ptr)) {
>> + passed = false;
>> + fwts_failed(fw, LOG_LEVEL_HIGH,
>> + "FPDTFWBootPerfPrtRecBadLength",
>> + "FPDT Firmware Basic Boot Performance Pointer Record is %" PRIu32
>> + " bytes and should be %zu bytes in size",
>> + fbbpr->fpdt.length, sizeof(fwts_acpi_table_fpdt_basic_boot_perf_ptr));
>> + } else {
>> + fpdt_rec_header_dump(fw, " Firmware Basic Boot Performance Pointer Record", fpdt);
>> + fwts_log_info_verbatum(fw, " Reserved: 0x%8.8" PRIx32, fbbpr->reserved);
>> + fwts_log_info_verbatum(fw, " FBPT Pointer: 0x%16.16" PRIx64, fbbpr->fbpt_addr);
>> +
>> + /*
>> + * For the moment, only dump the 64-bit processor-relative physical address
>> + * of the Firmware Basic Boot Performance Table, should also get and check
>> + * the table from the address
>> + */
> It may be worth printing an info message saying we're not checking FBPT
> for now.
>
>> + }
>> +
>> + if (fbbpr->fpdt.revision != 1) {
>> + passed = false;
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> + "FPDTInvalidReversion",
>> + "FPDT: Reversion field is 0x%" PRIx8
> "Reversion" should be "Revision"
>
>> + " and not the expected value of 0x01",
>> + fbbpr->fpdt.revision);
>> + }
>> + /* Spec doesn't mention Reserved field should be 0 or not, skip checking the reserved field */
>> + break;
>> + case 0x0001: /* S3 Performance Table Pointer Record */
>> + if (s3ptpr->fpdt.length != sizeof(fwts_acpi_table_fpdt_s3_perf_ptr)) {
>> + passed = false;
>> + fwts_failed(fw, LOG_LEVEL_HIGH,
>> + "FPDTS3PerfPrtRecBadLength",
>> + "FPDT S3 Performance Table Pointer Record is %" PRIu32
>> + " bytes and should be %zu bytes in size",
>> + s3ptpr->fpdt.length, sizeof(fwts_acpi_table_fpdt_s3_perf_ptr));
>> + } else {
>> + fpdt_rec_header_dump(fw, "S3 Performance Table Pointer Record", fpdt);
>> + fwts_log_info_verbatum(fw, " Reserved: 0x%8.8" PRIx32, s3ptpr->reserved);
>> + fwts_log_info_verbatum(fw, " S3PT Pointer: 0x%16.16" PRIx64, s3ptpr->s3pt_addr);
>> +
>> + /*
>> + * For the moment, only dump 64-bit processor-relative physical
>> + * address of the S3 Performance Table, should also get and check
>> + * the table from the address
>> + */
> It may be worth printing an info message saying we're not checking S3PT
> for now. e.g.
> fwts_log_info(fw, "Note: currently fwts does not check S3PT validity
> and the associated data");
>
>
> we can come back and mmap and check this region on another
> iteration/release of fwts.
>
>> + }
>> +
>> + if (s3ptpr->fpdt.revision != 1) {
>> + passed = false;
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> + "FPDTInvalidReversion",
> FPDTInvalidReversion should be FPDTInvalidRevision
>
>> + "FPDT: Reversion field is 0x%" PRIx8
>> + " and not the expected value of 0x01",
>> + s3ptpr->fpdt.revision);
>> + }
>> + /* Spec doesn't mention Reserved field should be 0 or not, skip checking the reserved field */
>> + break;
>> + case 0x0002 ... 0x0fff:
>> + fpdt_rec_header_dump(fw, "Reserved for ACPI specification usage", fpdt);
> I'd recommend replacing "usage" with "use", same for occurrences below too
>
>> + fpdt_dump_raw_data(fw, ptr + fpdt_hdr_len, fpdt->length - fpdt_hdr_len, ptr + fpdt_hdr_len - data);
>> + break;
>> + case 0x1000 ... 0x1fff:
>> + fpdt_rec_header_dump(fw, "Reserved for Platform Vendor usage", fpdt);
>> + fpdt_dump_raw_data(fw, ptr + fpdt_hdr_len, fpdt->length - fpdt_hdr_len, ptr + fpdt_hdr_len - data);
>> + break;
>> + case 0x2000 ... 0x2fff:
>> + fpdt_rec_header_dump(fw, "Reserved for Hardware Vendor usage", fpdt);
>> + fpdt_dump_raw_data(fw, ptr + fpdt_hdr_len, fpdt->length - fpdt_hdr_len, ptr + fpdt_hdr_len - data);
>> + break;
>> + case 0x3000 ... 0x3fff:
>> + fpdt_rec_header_dump(fw, "Reserved for BIOS Vendor usage", fpdt);
>> + fpdt_dump_raw_data(fw, ptr + fpdt_hdr_len, fpdt->length - fpdt_hdr_len, ptr + fpdt_hdr_len - data);
>> + break;
>> + default: /* For future use */
>> + passed = false;
>> + fwts_failed(fw, LOG_LEVEL_HIGH,
>> + "FPDTPerformanceRecordTypeInvalid",
>> + "FPDT Performance Record Type is 0x%4.4" PRIx16
>> + " which is a reserved type for future use only",
>> + fpdt->type);
>> + break;
>> + }
>> + ptr += fpdt->length;
>> + }
>> +
>> +done:
>> + if (passed)
>> + fwts_passed(fw, "No issues found in FPDT table.");
>> +
>> + return FWTS_OK;
>> +}
>> +
>> +static fwts_framework_minor_test fpdt_tests[] = {
>> + { fpdt_test1, "FPDT Firmware Performance Data Table test." },
>> + { NULL, NULL }
>> +};
>> +
>> +static fwts_framework_ops fpdt_ops = {
>> + .description = "FPDT Firmware Performance Data Table test.",
>> + .init = fpdt_init,
>> + .minor_tests = fpdt_tests
>> +};
>> +
>> +FWTS_REGISTER("fpdt", &fpdt_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI)
>>
For the others,
Thanks for spotting these typo and the suggestion, will send out the v2
patch for fixing these later.
More information about the fwts-devel
mailing list