[2/3] acpi: phat: add a test for PHAT table

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 src/Makefile.am             |   1 +
 src/acpi/phat/phat.c        | 153 ++++++++++++++++++++++++++++++++++++
 src/lib/include/fwts_acpi.h |  40 ++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 src/acpi/phat/phat.c

On 09/04/2021 03:27, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  src/Makefile.am             |   1 +
>  src/acpi/phat/phat.c        | 153 ++++++++++++++++++++++++++++++++++++
>  src/lib/include/fwts_acpi.h |  40 ++++++++++
>  3 files changed, 194 insertions(+)
>  create mode 100644 src/acpi/phat/phat.c
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index a26a197b..95293080 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -118,6 +118,7 @@ fwts_SOURCES = main.c 				\
>  	acpi/pdtt/pdtt.c			\
>  	acpi/powerbutton/powerbutton.c 		\
>  	acpi/plddump/plddump.c			\
> +	acpi/phat/phat.c 			\
>  	acpi/pmtt/pmtt.c 			\
>  	acpi/pptt/pptt.c 			\
>  	acpi/rasf/rasf.c			\
> diff --git a/src/acpi/phat/phat.c b/src/acpi/phat/phat.c
> new file mode 100644
> index 00000000..066d1313
> --- /dev/null
> +++ b/src/acpi/phat/phat.c
> @@ -0,0 +1,153 @@
> +/*
> + * Copyright (C) 2021 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"
> +
> +#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;
> +acpi_table_init(PHAT, &table)
> +
> +static void print_record_header(fwts_framework *fw, const fwts_acpi_table_phat_header *header)
> +{
> +	fwts_log_info_simp_int(fw, "    Type:                           ", header->type);
> +	fwts_log_info_simp_int(fw, "    Record Length:                  ", header->length);
> +	fwts_log_info_simp_int(fw, "    Revision:                       ", header->revision);
> +}
> +
> +static void phat_version_test(fwts_framework *fw, fwts_acpi_table_phat_version *entry, bool *passed)
> +{
> +	uint32_t reserved, i;
> +
> +	reserved = entry->reserved[0] + (entry->reserved[1] << 8) + (entry->reserved[2] << 16);
> +
> +	fwts_log_info_verbatim(fw, "  Firmware Version Data Record (Type 0):");
> +	print_record_header(fw, &entry->header);
> +	fwts_log_info_simp_int(fw, "    Reserved:                       ", reserved);
> +	fwts_log_info_simp_int(fw, "    Record Count:                   ", entry->element_count);
> +
> +	for (i = 0 ; i < entry->element_count; i++) {

What happens if element->count is incorrect? I think some check that we
don't fall off the end of the table with the element pointer below.

> +		char guid[37];

blank line here plase

> +		fwts_acpi_table_phat_version_elem *element =
> +			(fwts_acpi_table_phat_version_elem *) ((char *)entry + sizeof(fwts_acpi_table_phat_version));
> +
> +		fwts_guid_buf_to_str(element->component_id, guid, sizeof(guid));
> +
> +		fwts_log_info_verbatim(fw, "    Component ID:                   %s", guid);
> +		fwts_log_info_simp_int(fw, "    Version:                        ", element->version);
> +		fwts_log_info_simp_int(fw, "    Producer ID:                    ", element->producer_id);
> +	}
> +
> +	fwts_acpi_reserved_zero_check("PHAT", "Reserved", reserved, passed);
> +}
> +
> +static void phat_health_test(fwts_framework *fw, fwts_acpi_table_phat_health *entry, bool *passed)
> +{
> +	char *device_path;
> +	char guid[37];
> +
> +	fwts_guid_buf_to_str(entry->data_signature, guid, sizeof(guid));
> +	device_path = (char *)entry + sizeof(fwts_acpi_table_phat_health);
> +
> +	fwts_log_info_verbatim(fw, " Firmware Health Data Record (Type 1):");
> +	print_record_header(fw, &entry->header);
> +	fwts_log_info_simp_int(fw, "    Reserved:                       ", entry->reserved);
> +	fwts_log_info_simp_int(fw, "    AmHealthy:                      ", entry->healthy);
> +	fwts_log_info_verbatim(fw, "    Device Signature:               %s", guid);
> +	fwts_log_info_simp_int(fw, "    Device-specific Data Offset:    ", entry->data_offset);
> +	fwts_log_info_verbatim(fw, "    Device Path:                    %s", device_path);
> +
> +	if (entry->data_offset != 0) {
> +		uint16_t i;
> +

Same here for entry->header.length - we must ensure vendor_data does not
fall off the end of the table

> +		for (i = entry->data_offset; i < entry->header.length; i++) {
> +			uint8_t *vendor_data = (uint8_t *)entry + i;

blank line here please

> +			fwts_log_info_simp_int(fw, "    Vendor Data:                    ", *vendor_data);
> +		}
> +	}
> +
> +	fwts_acpi_reserved_zero_check("PHAT", "Reserved", entry->reserved, passed);
> +
> +	if (entry->data_offset > entry->header.length) {
> +		fwts_failed(fw, LOG_LEVEL_CRITICAL,
> +			"PHATOutOfRangeOffset",
> +			"PHAT Type 1's Data Offset is out of range");
> +		*passed = false;
> +	}
> +}
> +
> +static int phat_test1(fwts_framework *fw)
> +{
> +	fwts_acpi_table_phat_header *entry;
> +	bool passed = true;
> +	uint32_t offset;
> +
> +	fwts_log_info_verbatim(fw, "PHAT Platform Health Assessment Table:");
> +
> +	entry = (fwts_acpi_table_phat_header *) (table->data + sizeof(fwts_acpi_table_phat));
> +	offset = sizeof(fwts_acpi_table_phat);
> +	while (offset < table->length) {
> +

blank line can be removed

> +		if (fwts_acpi_structure_length_zero_check(fw, "PHAT", entry->length, offset)) {
> +			passed = false;
> +			break;
> +		}
> +
> +		if (entry->type == FWTS_ACPI_PHAT_VERSION) {
> +			phat_version_test(fw, (fwts_acpi_table_phat_version *) entry, &passed);
> +		} else if (entry->type == FWTS_ACPI_PHAT_HEALTH) {
> +			phat_health_test(fw, (fwts_acpi_table_phat_health *) entry, &passed);
> +		} else {
> +			fwts_acpi_structure_type_error(fw, "PHAT", FWTS_ACPI_PHAT_END - 1, entry->type);
> +			passed = false;
> +			break;
> +		}
> +
> +		offset += entry->length;

probably worth checking if offset falls off the end of the table before
calling this check

> +		if (fwts_acpi_structure_range_check(fw, "PHAT", table->length, offset)) {
> +			passed = false;
> +			break;
> +		}
> +
> +		entry = (fwts_acpi_table_phat_header *) (table->data + offset);
> +		fwts_log_nl(fw);
> +	}
> +
> +	if (passed)
> +		fwts_passed(fw, "No issues found in PHAT table.");
> +
> +	return FWTS_OK;
> +}
> +
> +static fwts_framework_minor_test phat_tests[] = {
> +	{ phat_test1, "Validate PHAT table." },
> +	{ NULL, NULL }
> +};
> +
> +static fwts_framework_ops phat_ops = {
> +	.description = "PHAT Platform Health Assessment Table test.",
> +	.init        = PHAT_init,
> +	.minor_tests = phat_tests
> +};
> +
> +FWTS_REGISTER("phat", &phat_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 d223d934..e9ccdb0c 100644
> --- a/src/lib/include/fwts_acpi.h
> +++ b/src/lib/include/fwts_acpi.h
> @@ -1512,6 +1512,46 @@ typedef struct {
>  	uint16_t	spin_rev;
>  } __attribute__ ((packed)) fwts_acpi_table_pptt_id;
>  
> +/*
> + * ACPI PHAT (Platform Health Assessment Table), 5.2.30
> + */
> +typedef struct {
> +	fwts_acpi_table_header	header;
> +} __attribute__ ((packed)) fwts_acpi_table_phat;
> +
> +typedef enum {
> +	FWTS_ACPI_PHAT_VERSION	= 0,
> +	FWTS_ACPI_PHAT_HEALTH	= 1,
> +	FWTS_ACPI_PHAT_END
> +} fwts_acpi_phat_type;
> +
> +typedef struct {
> +	uint16_t	type;
> +	uint16_t	length;
> +	uint8_t		revision;
> +} __attribute__ ((packed)) fwts_acpi_table_phat_header;
> +
> +/* 0: Firmware Version Data Record */
> +typedef struct {
> +	fwts_acpi_table_phat_header	header;
> +	uint8_t		reserved[3];
> +	uint32_t	element_count;
> +} __attribute__ ((packed)) fwts_acpi_table_phat_version;
> +
> +typedef struct {
> +	uint8_t		component_id[16];
> +	uint64_t	version;
> +	uint32_t	producer_id;
> +} __attribute__ ((packed)) fwts_acpi_table_phat_version_elem;
> +
> +/* 1: Firmware Health Data Record */
> +typedef struct {
> +	fwts_acpi_table_phat_header	header;
> +	uint16_t	reserved;
> +	uint8_t		healthy;
> +	uint8_t		data_signature[16];
> +	uint32_t	data_offset;
> +} __attribute__ ((packed)) fwts_acpi_table_phat_health;
>  
>  /*
>   * ACPI PCCT (Platform Communications Channel Table), 14.1
>

Thanks for the comments and I will fix them in V2 except one comment below.

On 2021-04-09 2:33 a.m., Colin Ian King wrote:
> On 09/04/2021 03:27, Alex Hung wrote:
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>  src/Makefile.am             |   1 +
>>  src/acpi/phat/phat.c        | 153 ++++++++++++++++++++++++++++++++++++
>>  src/lib/include/fwts_acpi.h |  40 ++++++++++
>>  3 files changed, 194 insertions(+)
>>  create mode 100644 src/acpi/phat/phat.c
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index a26a197b..95293080 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -118,6 +118,7 @@ fwts_SOURCES = main.c 				\
>>  	acpi/pdtt/pdtt.c			\
>>  	acpi/powerbutton/powerbutton.c 		\
>>  	acpi/plddump/plddump.c			\
>> +	acpi/phat/phat.c 			\
>>  	acpi/pmtt/pmtt.c 			\
>>  	acpi/pptt/pptt.c 			\
>>  	acpi/rasf/rasf.c			\
>> diff --git a/src/acpi/phat/phat.c b/src/acpi/phat/phat.c
>> new file mode 100644
>> index 00000000..066d1313
>> --- /dev/null
>> +++ b/src/acpi/phat/phat.c
>> @@ -0,0 +1,153 @@
>> +/*
>> + * Copyright (C) 2021 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"
>> +
>> +#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;
>> +acpi_table_init(PHAT, &table)
>> +
>> +static void print_record_header(fwts_framework *fw, const fwts_acpi_table_phat_header *header)
>> +{
>> +	fwts_log_info_simp_int(fw, "    Type:                           ", header->type);
>> +	fwts_log_info_simp_int(fw, "    Record Length:                  ", header->length);
>> +	fwts_log_info_simp_int(fw, "    Revision:                       ", header->revision);
>> +}
>> +
>> +static void phat_version_test(fwts_framework *fw, fwts_acpi_table_phat_version *entry, bool *passed)
>> +{
>> +	uint32_t reserved, i;
>> +
>> +	reserved = entry->reserved[0] + (entry->reserved[1] << 8) + (entry->reserved[2] << 16);
>> +
>> +	fwts_log_info_verbatim(fw, "  Firmware Version Data Record (Type 0):");
>> +	print_record_header(fw, &entry->header);
>> +	fwts_log_info_simp_int(fw, "    Reserved:                       ", reserved);
>> +	fwts_log_info_simp_int(fw, "    Record Count:                   ", entry->element_count);
>> +
>> +	for (i = 0 ; i < entry->element_count; i++) {
> 
> What happens if element->count is incorrect? I think some check that we
> don't fall off the end of the table with the element pointer below.
> 
>> +		char guid[37];
> 
> blank line here plase
> 
>> +		fwts_acpi_table_phat_version_elem *element =
>> +			(fwts_acpi_table_phat_version_elem *) ((char *)entry + sizeof(fwts_acpi_table_phat_version));
>> +
>> +		fwts_guid_buf_to_str(element->component_id, guid, sizeof(guid));
>> +
>> +		fwts_log_info_verbatim(fw, "    Component ID:                   %s", guid);
>> +		fwts_log_info_simp_int(fw, "    Version:                        ", element->version);
>> +		fwts_log_info_simp_int(fw, "    Producer ID:                    ", element->producer_id);
>> +	}
>> +
>> +	fwts_acpi_reserved_zero_check("PHAT", "Reserved", reserved, passed);
>> +}
>> +
>> +static void phat_health_test(fwts_framework *fw, fwts_acpi_table_phat_health *entry, bool *passed)
>> +{
>> +	char *device_path;
>> +	char guid[37];
>> +
>> +	fwts_guid_buf_to_str(entry->data_signature, guid, sizeof(guid));
>> +	device_path = (char *)entry + sizeof(fwts_acpi_table_phat_health);
>> +
>> +	fwts_log_info_verbatim(fw, " Firmware Health Data Record (Type 1):");
>> +	print_record_header(fw, &entry->header);
>> +	fwts_log_info_simp_int(fw, "    Reserved:                       ", entry->reserved);
>> +	fwts_log_info_simp_int(fw, "    AmHealthy:                      ", entry->healthy);
>> +	fwts_log_info_verbatim(fw, "    Device Signature:               %s", guid);
>> +	fwts_log_info_simp_int(fw, "    Device-specific Data Offset:    ", entry->data_offset);
>> +	fwts_log_info_verbatim(fw, "    Device Path:                    %s", device_path);
>> +
>> +	if (entry->data_offset != 0) {
>> +		uint16_t i;
>> +
> 
> Same here for entry->header.length - we must ensure vendor_data does not
> fall off the end of the table
> 
>> +		for (i = entry->data_offset; i < entry->header.length; i++) {
>> +			uint8_t *vendor_data = (uint8_t *)entry + i;
> 
> blank line here please
> 
>> +			fwts_log_info_simp_int(fw, "    Vendor Data:                    ", *vendor_data);
>> +		}
>> +	}
>> +
>> +	fwts_acpi_reserved_zero_check("PHAT", "Reserved", entry->reserved, passed);
>> +
>> +	if (entry->data_offset > entry->header.length) {
>> +		fwts_failed(fw, LOG_LEVEL_CRITICAL,
>> +			"PHATOutOfRangeOffset",
>> +			"PHAT Type 1's Data Offset is out of range");
>> +		*passed = false;
>> +	}
>> +}
>> +
>> +static int phat_test1(fwts_framework *fw)
>> +{
>> +	fwts_acpi_table_phat_header *entry;
>> +	bool passed = true;
>> +	uint32_t offset;
>> +
>> +	fwts_log_info_verbatim(fw, "PHAT Platform Health Assessment Table:");
>> +
>> +	entry = (fwts_acpi_table_phat_header *) (table->data + sizeof(fwts_acpi_table_phat));
>> +	offset = sizeof(fwts_acpi_table_phat);
>> +	while (offset < table->length) {
>> +
> 
> blank line can be removed
> 
>> +		if (fwts_acpi_structure_length_zero_check(fw, "PHAT", entry->length, offset)) {
>> +			passed = false;
>> +			break;
>> +		}
>> +
>> +		if (entry->type == FWTS_ACPI_PHAT_VERSION) {
>> +			phat_version_test(fw, (fwts_acpi_table_phat_version *) entry, &passed);
>> +		} else if (entry->type == FWTS_ACPI_PHAT_HEALTH) {
>> +			phat_health_test(fw, (fwts_acpi_table_phat_health *) entry, &passed);
>> +		} else {
>> +			fwts_acpi_structure_type_error(fw, "PHAT", FWTS_ACPI_PHAT_END - 1, entry->type);
>> +			passed = false;
>> +			break;
>> +		}
>> +
>> +		offset += entry->length;
> 
> probably worth checking if offset falls off the end of the table before
> calling this check

The fwts_acpi_structure_range_check checks whether offset out of table
length, but I made a mistake in its comments and I will fix it in v2.

> 
>> +		if (fwts_acpi_structure_range_check(fw, "PHAT", table->length, offset)) {
>> +			passed = false;
>> +			break;
>> +		}
>> +
>> +		entry = (fwts_acpi_table_phat_header *) (table->data + offset);
>> +		fwts_log_nl(fw);
>> +	}
>> +
>> +	if (passed)
>> +		fwts_passed(fw, "No issues found in PHAT table.");
>> +
>> +	return FWTS_OK;
>> +}
>> +
>> +static fwts_framework_minor_test phat_tests[] = {
>> +	{ phat_test1, "Validate PHAT table." },
>> +	{ NULL, NULL }
>> +};
>> +
>> +static fwts_framework_ops phat_ops = {
>> +	.description = "PHAT Platform Health Assessment Table test.",
>> +	.init        = PHAT_init,
>> +	.minor_tests = phat_tests
>> +};
>> +
>> +FWTS_REGISTER("phat", &phat_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 d223d934..e9ccdb0c 100644
>> --- a/src/lib/include/fwts_acpi.h
>> +++ b/src/lib/include/fwts_acpi.h
>> @@ -1512,6 +1512,46 @@ typedef struct {
>>  	uint16_t	spin_rev;
>>  } __attribute__ ((packed)) fwts_acpi_table_pptt_id;
>>  
>> +/*
>> + * ACPI PHAT (Platform Health Assessment Table), 5.2.30
>> + */
>> +typedef struct {
>> +	fwts_acpi_table_header	header;
>> +} __attribute__ ((packed)) fwts_acpi_table_phat;
>> +
>> +typedef enum {
>> +	FWTS_ACPI_PHAT_VERSION	= 0,
>> +	FWTS_ACPI_PHAT_HEALTH	= 1,
>> +	FWTS_ACPI_PHAT_END
>> +} fwts_acpi_phat_type;
>> +
>> +typedef struct {
>> +	uint16_t	type;
>> +	uint16_t	length;
>> +	uint8_t		revision;
>> +} __attribute__ ((packed)) fwts_acpi_table_phat_header;
>> +
>> +/* 0: Firmware Version Data Record */
>> +typedef struct {
>> +	fwts_acpi_table_phat_header	header;
>> +	uint8_t		reserved[3];
>> +	uint32_t	element_count;
>> +} __attribute__ ((packed)) fwts_acpi_table_phat_version;
>> +
>> +typedef struct {
>> +	uint8_t		component_id[16];
>> +	uint64_t	version;
>> +	uint32_t	producer_id;
>> +} __attribute__ ((packed)) fwts_acpi_table_phat_version_elem;
>> +
>> +/* 1: Firmware Health Data Record */
>> +typedef struct {
>> +	fwts_acpi_table_phat_header	header;
>> +	uint16_t	reserved;
>> +	uint8_t		healthy;
>> +	uint8_t		data_signature[16];
>> +	uint32_t	data_offset;
>> +} __attribute__ ((packed)) fwts_acpi_table_phat_health;
>>  
>>  /*
>>   * ACPI PCCT (Platform Communications Channel Table), 14.1
>>
> 
>