[PATCH 1/3] acpi: add MSCT table sanity check
Colin Ian King
colin.king at canonical.com
Fri Feb 26 10:04:31 UTC 2016
On 26/02/16 08:37, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
> src/Makefile.am | 1 +
> src/acpi/msct/msct.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 109 insertions(+)
> create mode 100644 src/acpi/msct/msct.c
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 2de2585..dbef055 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -63,6 +63,7 @@ fwts_SOURCES = main.c \
> acpi/madt/madt.c \
> acpi/mcfg/mcfg.c \
> acpi/mchi/mchi.c \
> + acpi/msct/msct.c \
> acpi/msdm/msdm.c \
> acpi/method/method.c \
> acpi/osilinux/osilinux.c \
> diff --git a/src/acpi/msct/msct.c b/src/acpi/msct/msct.c
> new file mode 100644
> index 0000000..72b02a5
> --- /dev/null
> +++ b/src/acpi/msct/msct.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright (C) 2016 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.
> + *
> + */
> +#include "fwts.h"
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <inttypes.h>
> +#include <string.h>
> +#include <ctype.h>
> +#include "fwts_acpi_object_eval.h"
I guess fwts_acpi_object_eval.h is being included to pick up the ACPICA
table struct definitions. In the past, all new tables were added into
fwts into src/lib/include/fwts_acpi.h because:
1) I wasn't using ACPICA when fwts was first written
2) When ACPICA was being used later on I wanted to ensure we had a clean
separation from ACPICA.
I'm still trying to work out if 2) makes sense. I think if we do include
data structures from ACPICA then perhaps we should not include
fwts_acpi_object_eval.h but instead just add the following to fwts_acpica.h:
#pragma GCC diagnostic ignored "-Wunused-parameter"
#include "acpi.h"
#pragma GCC diagnostic error "-Wunused-parameter"
and then include that instead rather than using fwts_acpi_object_eval.h.
At least then the name of the include file indicates we're pulling in
some ACPICA related definitions.
> +
> +static fwts_acpi_table_info *table;
> +
> +static int msct_init(fwts_framework *fw)
> +{
> + if (fwts_acpi_find_table(fw, "MSCT", 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 MSCT table does not exist, skipping test");
> + return FWTS_SKIP;
> + }
> + return FWTS_OK;
> +}
> +
> +/*
> + * MSCT Maximum System Characteristics Table
> + */
> +static int msct_test1(fwts_framework *fw)
> +{
> + ACPI_TABLE_MSCT *msct = (ACPI_TABLE_MSCT *)table->data;
> + uint32_t numProximity = 0, OffsetProximity = 0;
> + uint32_t i;
> + bool passed = true;
> +
> + fwts_log_info_verbatum(fw, "MSCT Maximum System Characteristics Table:");
> + fwts_log_info_verbatum(fw, " Proximity Offset: 0x%8.8" PRIX32, msct->ProximityOffset);
> + fwts_log_info_verbatum(fw, " Max Proximity Domains: 0x%8.8" PRIX32, msct->MaxProximityDomains);
> + fwts_log_info_verbatum(fw, " Max Clock Domains: 0x%8.8" PRIX32, msct->MaxClockDomains);
> + fwts_log_info_verbatum(fw, " Max Physical Address: 0x%16.16" PRIX64, msct->MaxAddress);
The convention in fwts is to use lower case hex, so PRIx32, PRIx64 is
preferred here (and throughout this test).
> +
> + if (msct->ProximityOffset < 0x38) {
> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
> + "MSCTBadProximityOffset",
> + "Proximity Domain Information Structures need to be located at Offset 0x38 "
> + "or after, but Proximity Offsetis is 0x%" PRIX32, msct->ProximityOffset);
> + OffsetProximity = 0x38;
> + passed = false;
> + } else
> + OffsetProximity = msct->ProximityOffset;
What about the case where ProximityOffset + numProximity *
sizeof(ACPI_MSCT_PROXIMITY) > sizeof the table? We need to check for
overflows off the end of the table too.
> +
> + numProximity = (msct->Header.Length - OffsetProximity) / sizeof(ACPI_MSCT_PROXIMITY);
> +
> + if (numProximity > msct->MaxProximityDomains + 1) {
> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
> + "MSCTBadProimityDomains",
> + "MSCT's Max Proximity Domains is 0x%" PRIX32
> + ", but it has 0x%" PRIX32 " Proximity Domain Information Structures",
> + msct->MaxProximityDomains, numProximity);
> + passed = false;
> + }
> +
> + fwts_log_nl(fw);
How about:
proximity = (ACPI_MSCT_PROXIMITY *)((char *) msct + OffsetProximity;
> +
> + for (i = 0; i < numProximity; i++) {
And then:
for (i = 0; i < numProximity; i++, proximity++) {
> + ACPI_MSCT_PROXIMITY *proximity = (ACPI_MSCT_PROXIMITY *)((char *) msct + OffsetProximity + i * sizeof(ACPI_MSCT_PROXIMITY));
> + fwts_log_info_verbatum(fw, " Proximity Domain Information Structure %2.2" PRIu8, i);
> + fwts_log_info_verbatum(fw, " Revision: 0x%2.2" PRIX8, proximity->Revision);
> + fwts_log_info_verbatum(fw, " Length: 0x%2.2" PRIX8, proximity->Length);
> + fwts_log_info_verbatum(fw, " Proximity Domain Range (low): 0x%8.8" PRIX32, proximity->RangeStart);
> + fwts_log_info_verbatum(fw, " Proximity Domain Range (high): 0x%8.8" PRIX32, proximity->RangeEnd);
> + fwts_log_info_verbatum(fw, " Maximum Processor Capacity: 0x%8.8" PRIX32, proximity->ProcessorCapacity);
> + fwts_log_info_verbatum(fw, " Maximum Memory Capacity: 0x%16.16" PRIX64, proximity->MemoryCapacity);
> + fwts_log_nl(fw);
> + }
> +
> + if (passed)
> + fwts_passed(fw, "No issues found in MSCT table.");
> +
> + return FWTS_OK;
> +}
> +
> +static fwts_framework_minor_test msct_tests[] = {
> + { msct_test1, "MSCT Maximum System Characteristics Table test." },
> + { NULL, NULL }
> +};
> +
> +static fwts_framework_ops msct_ops = {
> + .description = "MSCT Maximum System Characteristics Table test.",
> + .init = msct_init,
> + .minor_tests = msct_tests
> +};
> +
> +FWTS_REGISTER("msct", &msct_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI)
>
Finally, if you can make this code 80 column wide then that would be
appreciated too.
Colin
More information about the fwts-devel
mailing list