ACPI: PHAT: Add Platform Health Assessment Table support

ACPI Platform Health Assessment Table (PHAT) enables a platform to expose
an extensible set of platform health related telemetry. The telemetry is
exposed through Firmware Version and Firmware Health Data Records which
provide version data and health-related information of their associated
components respectively.

Additionally, the platform also provides Reset Reason Health Record in
the PHAT table highlighting the cause of last system reset or boot in case
of both expected and unexpected events. Vendor-specific data capturing the
underlying state of the system during reset can also be optionally provided
through the record.[1]

Add support to parse the PHAT table during system bootup and have its
information logged into the dmesg buffer.

[1] ACPI specification 6.5, section 5.2.31.5

Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
---
 .../admin-guide/kernel-parameters.txt         |   4 +
 drivers/acpi/Kconfig                          |   9 +
 drivers/acpi/Makefile                         |   1 +
 drivers/acpi/phat.c                           | 270 ++++++++++++++++++
 include/acpi/actbl2.h                         |  18 ++
 5 files changed, 302 insertions(+)
 create mode 100644 drivers/acpi/phat.c

On 8/10/2023 18:48, Avadhut Naik wrote:
> ACPI Platform Health Assessment Table (PHAT) enables a platform to expose
> an extensible set of platform health related telemetry. The telemetry is
> exposed through Firmware Version and Firmware Health Data Records which
> provide version data and health-related information of their associated
> components respectively.
> 
> Additionally, the platform also provides Reset Reason Health Record in
> the PHAT table highlighting the cause of last system reset or boot in case
> of both expected and unexpected events. Vendor-specific data capturing the
> underlying state of the system during reset can also be optionally provided
> through the record.[1]
> 
> Add support to parse the PHAT table during system bootup and have its
> information logged into the dmesg buffer.
> 
> [1] ACPI specification 6.5, section 5.2.31.5
> 
> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |   4 +
>  drivers/acpi/Kconfig                          |   9 +
>  drivers/acpi/Makefile                         |   1 +
>  drivers/acpi/phat.c                           | 270 ++++++++++++++++++
>  include/acpi/actbl2.h                         |  18 ++
>  5 files changed, 302 insertions(+)
>  create mode 100644 drivers/acpi/phat.c
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 722b6eca2e93..33b932302ece 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4490,6 +4490,10 @@
>  			allocator.  This parameter is primarily	for debugging
>  			and performance comparison.
>  
> +	phat_disable=	[ACPI]
	Slight Correction: This should just be "phat_disable".
Will handle this in the next revision along with the feedback
that may be provided for this patch.
> +			Disable PHAT table parsing and logging of Firmware
> +			Version and Health Data records.
> +
>  	pirq=		[SMP,APIC] Manual mp-table setup
>  			See Documentation/arch/x86/i386/IO-APIC.rst.
>  
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 00dd309b6682..06a7dd6e5a40 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -96,6 +96,15 @@ config ACPI_FPDT
>  	  This table provides information on the timing of the system
>  	  boot, S3 suspend and S3 resume firmware code paths.
>  
> +config ACPI_PHAT
> +	bool "ACPI Platform Health Assessment Table (PHAT) support"
> +	depends on X86_64 || ARM64
> +	help
> +	  Enable support for Platform Health Assessment Table (PHAT).
> +	  This table exposes an extensible set of platform health
> +	  related telemetry through Firmware Version and Firmware Health
> +	  Data Records.
> +
>  config ACPI_LPIT
>  	bool
>  	depends on X86_64
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 3fc5a0d54f6e..93a4ec57ba6d 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -69,6 +69,7 @@ acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
>  acpi-$(CONFIG_ACPI_PRMT)	+= prmt.o
>  acpi-$(CONFIG_ACPI_PCC)		+= acpi_pcc.o
>  acpi-$(CONFIG_ACPI_FFH)		+= acpi_ffh.o
> +acpi-$(CONFIG_ACPI_PHAT)	+= phat.o
>  
>  # Address translation
>  acpi-$(CONFIG_ACPI_ADXL)	+= acpi_adxl.o
> diff --git a/drivers/acpi/phat.c b/drivers/acpi/phat.c
> new file mode 100644
> index 000000000000..6006dd7615fa
> --- /dev/null
> +++ b/drivers/acpi/phat.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Platform Health Assessment Table (PHAT) support
> + *
> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
> + *
> + * Author: Avadhut Naik <avadhut.naik@amd.com>
> + *
> + * This file implements parsing of the Platform Health Assessment Table
> + * through which a platform can expose an extensible set of platform
> + * health related telemetry. The telemetry is exposed through Firmware
> + * Version Data Records and Firmware Health Data Records. Additionally,
> + * a platform, through system firmware, also exposes Reset Reason Health
> + * Record to inform the operating system of the cause of last system
> + * reset or boot.
> + *
> + * For more information on PHAT, please refer to ACPI specification
> + * version 6.5, section 5.2.31
> + */
> +
> +#include <linux/acpi.h>
> +
> +static int phat_disable __initdata;
> +static const char *prefix = "ACPI PHAT: ";
> +
> +/* Reset Reason Health Record GUID */
> +static const guid_t reset_guid =
> +	GUID_INIT(0x7a014ce2, 0xf263, 0x4b77,
> +		  0xb8, 0x8a, 0xe6, 0x33, 0x6b, 0x78, 0x2c, 0x14);
> +
> +static struct { u8 mask; const char *str; } const reset_sources[] = {
> +	{BIT(0), "Unknown source"},
> +	{BIT(1), "Hardware Source"},
> +	{BIT(2), "Firmware Source"},
> +	{BIT(3), "Software initiated reset"},
> +	{BIT(4), "Supervisor initiated reset"},
> +};
> +
> +static struct { u8 val; const char *str; } const reset_reasons[] = {
> +	{0, "UNKNOWN"},
> +	{1, "COLD BOOT"},
> +	{2, "COLD RESET"},
> +	{3, "WARM RESET"},
> +	{4, "UPDATE"},
> +	{32, "UNEXPECTED RESET"},
> +	{33, "FAULT"},
> +	{34, "TIMEOUT"},
> +	{35, "THERMAL"},
> +	{36, "POWER LOSS"},
> +	{37, "POWER BUTTON"},
> +};
> +
> +/*
> + * Print the last PHAT Version Element associated with a Firmware
> + * Version Data Record.
> + * Firmware Version Data Record consists of an array of PHAT Version
> + * Elements with each entry in the array representing a modification
> + * undertaken on a given platform component.
> + * In the event the array has multiple entries, minimize logs on the
> + * console and print only the last version element since it denotes
> + * the currently running instance of the component.
> + */
> +static int phat_version_data_parse(const char *pfx,
> +				   struct acpi_phat_version_data *version)
> +{
> +	char newpfx[64];
> +	u32 num_elems = version->element_count - 1;
> +	struct acpi_phat_version_element *element;
> +	int offset = sizeof(struct acpi_phat_version_data);
> +
> +	if (!version->element_count) {
> +		pr_info("%sNo PHAT Version Elements found.\n", prefix);
> +		return 0;
> +	}
> +
> +	offset += num_elems * sizeof(struct acpi_phat_version_element);
> +	element = (void *)version + offset;
> +
> +	pr_info("%sPHAT Version Element:\n", pfx);
> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> +	pr_info("%sComponent ID: %pUl\n", newpfx, element->guid);
> +	pr_info("%sVersion: 0x%llx\n", newpfx, element->version_value);
> +	snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> +	print_hex_dump(newpfx, "Producer ID: ", DUMP_PREFIX_NONE, 16, 4,
> +		       &element->producer_id, sizeof(element->producer_id), true);
> +
> +	return 0;
> +}
> +
> +/*
> + * Print the Reset Reason Health Record
> + */
> +static int phat_reset_reason_parse(const char *pfx,
> +				   struct acpi_phat_health_data *record)
> +{
> +	int idx;
> +	void *data;
> +	u32 data_len;
> +	char newpfx[64];
> +	struct acpi_phat_reset_reason *rr;
> +	struct acpi_phat_vendor_reset_data *vdata;
> +
> +	rr = (void *)record + record->device_specific_offset;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(reset_sources); idx++) {
> +		if (!rr->reset_source) {
> +			pr_info("%sUnknown Reset Source.\n", pfx);
> +			break;
> +		}
> +		if (rr->reset_source & reset_sources[idx].mask) {
> +			pr_info("%sReset Source: 0x%x\t%s\n", pfx, reset_sources[idx].mask,
> +				reset_sources[idx].str);
> +			/* According to ACPI v6.5 Table 5.168, Sub-Source is
> +			 * defined only for Software initiated reset.
> +			 */
> +			if (idx == 0x3 && rr->reset_sub_source)
> +				pr_info("%sReset Sub-Source: %s\n", pfx,
> +					rr->reset_sub_source == 0x1 ?
> +					"Operating System" : "Hypervisor");
> +			break;
> +		}
> +	}
> +
> +	for (idx = 0; idx < ARRAY_SIZE(reset_reasons); idx++) {
> +		if (rr->reset_reason == reset_reasons[idx].val) {
> +			pr_info("%sReset Reason: 0x%x\t%s\n", pfx, reset_reasons[idx].val,
> +				reset_reasons[idx].str);
> +			break;
> +		}
> +	}
> +
> +	if (!rr->vendor_count)
> +		return 0;
> +
> +	pr_info("%sReset Reason Vendor Data:\n", pfx);
> +	vdata = (void *)rr + sizeof(*rr);
> +
> +	for (idx = 0; idx < rr->vendor_count; idx++) {
> +		snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> +		data_len = vdata->length - sizeof(*vdata);
> +		data = (void *)vdata + sizeof(*vdata);
> +		pr_info("%sVendor Data ID: %pUl\n", newpfx, vdata->vendor_id);
> +		pr_info("%sRevision: 0x%x\n", newpfx, vdata->revision);
> +		snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> +		print_hex_dump(newpfx, "Data: ", DUMP_PREFIX_NONE, 16, 4,
> +			       data, data_len, false);
> +		vdata = (void *)vdata + vdata->length;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Print the Firmware Health Data Record.
> + */
> +static int phat_health_data_parse(const char *pfx,
> +				  struct acpi_phat_health_data *record)
> +{
> +	void *data;
> +	u32 data_len;
> +	char newpfx[64];
> +
> +	pr_info("%sHealth Records.\n", pfx);
> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> +	pr_info("%sDevice Signature: %pUl\n", newpfx, record->device_guid);
> +
> +	switch (record->health) {
> +	case ACPI_PHAT_ERRORS_FOUND:
> +		pr_info("%sAmHealthy: Errors found\n", newpfx);
> +		break;
> +	case ACPI_PHAT_NO_ERRORS:
> +		pr_info("%sAmHealthy: No errors found.\n", newpfx);
> +		break;
> +	case ACPI_PHAT_UNKNOWN_ERRORS:
> +		pr_info("%sAmHealthy: Unknown.\n", newpfx);
> +		break;
> +	case ACPI_PHAT_ADVISORY:
> +		pr_info("%sAmHealthy: Advisory – additional device-specific data exposed.\n",
> +			newpfx);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!record->device_specific_offset)
> +		return 0;
> +
> +	/* Reset Reason Health Record has a unique GUID and is created as
> +	 * a Health Record in the PHAT table. Check if this Health Record
> +	 * is a Reset Reason Health Record.
> +	 */
> +	if (guid_equal((guid_t *)record->device_guid, &reset_guid)) {
> +		phat_reset_reason_parse(newpfx, record);
> +		return 0;
> +	}
> +
> +	data = (void *)record + record->device_specific_offset;
> +	data_len = record->header.length - record->device_specific_offset;
> +	snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> +	print_hex_dump(newpfx, "Device Data: ", DUMP_PREFIX_NONE, 16, 4,
> +		       data, data_len, false);
> +
> +	return 0;
> +}
> +
> +static int parse_phat_table(const char *pfx, struct acpi_table_phat *phat_tab)
> +{
> +	char newpfx[64];
> +	u32 offset = sizeof(*phat_tab);
> +	struct acpi_phat_header *phat_header;
> +
> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> +
> +	while (offset < phat_tab->header.length) {
> +		phat_header = (void *)phat_tab + offset;
> +		switch (phat_header->type) {
> +		case ACPI_PHAT_TYPE_FW_VERSION_DATA:
> +			phat_version_data_parse(newpfx, (struct acpi_phat_version_data *)
> +			    phat_header);
> +			break;
> +		case ACPI_PHAT_TYPE_FW_HEALTH_DATA:
> +			phat_health_data_parse(newpfx, (struct acpi_phat_health_data *)
> +			    phat_header);
> +			break;
> +		default:
> +			break;
> +		}
> +		offset += phat_header->length;
> +	}
> +	return 0;
> +}
> +
> +static int __init setup_phat_disable(char *str)
> +{
> +	phat_disable = 1;
> +	return 1;
> +}
> +__setup("phat_disable", setup_phat_disable);
> +
> +static int __init acpi_phat_init(void)
> +{
> +	acpi_status status;
> +	struct acpi_table_phat *phat_tab;
> +
> +	if (acpi_disabled)
> +		return 0;
> +
> +	if (phat_disable) {
> +		pr_err("%sPHAT support has been disabled.\n", prefix);
> +		return 0;
> +	}
> +
> +	status = acpi_get_table(ACPI_SIG_PHAT, 0,
> +				(struct acpi_table_header **)&phat_tab);
> +
> +	if (status == AE_NOT_FOUND) {
> +		pr_info("%sPHAT Table not found.\n", prefix);
> +		return 0;
> +	} else if (ACPI_FAILURE(status)) {
> +		pr_err("%sFailed to get PHAT Table: %s.\n", prefix,
> +		       acpi_format_exception(status));
> +		return -EINVAL;
> +	}
> +
> +	pr_info("%sPlatform Telemetry Records.\n", prefix);
> +	parse_phat_table(prefix, phat_tab);
> +
> +	return 0;
> +}
> +late_initcall(acpi_phat_init);
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index 0029336775a9..c263893cbc7f 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -2360,6 +2360,24 @@ struct acpi_phat_health_data {
>  #define ACPI_PHAT_UNKNOWN_ERRORS        2
>  #define ACPI_PHAT_ADVISORY              3
>  
> +/* Reset Reason Health Record Structure */
> +
> +struct acpi_phat_reset_reason {
> +	u8 supported_reset_sources;
> +	u8 reset_source;
> +	u8 reset_sub_source;
> +	u8 reset_reason;
> +	u16 vendor_count;
> +};
> +
> +/* Reset Reason Health Record Vendor Data Entry */
> +
> +struct acpi_phat_vendor_reset_data {
> +	u8 vendor_id[16];
> +	u16 length;
> +	u16 revision;
> +};
> +
>  /*******************************************************************************
>   *
>   * PMTT - Platform Memory Topology Table (ACPI 5.0)

Hi,

On 8/11/2023 1:48 AM, Avadhut Naik wrote:
> ACPI Platform Health Assessment Table (PHAT) enables a platform to expose
> an extensible set of platform health related telemetry. The telemetry is
> exposed through Firmware Version and Firmware Health Data Records which
> provide version data and health-related information of their associated
> components respectively.
>
> Additionally, the platform also provides Reset Reason Health Record in
> the PHAT table highlighting the cause of last system reset or boot in case
> of both expected and unexpected events. Vendor-specific data capturing the
> underlying state of the system during reset can also be optionally provided
> through the record.[1]
>
> Add support to parse the PHAT table during system bootup and have its
> information logged into the dmesg buffer.
>
> [1] ACPI specification 6.5, section 5.2.31.5
>
> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |   4 +
>  drivers/acpi/Kconfig                          |   9 +
>  drivers/acpi/Makefile                         |   1 +
>  drivers/acpi/phat.c                           | 270 ++++++++++++++++++
>  include/acpi/actbl2.h                         |  18 ++
>  5 files changed, 302 insertions(+)
>  create mode 100644 drivers/acpi/phat.c
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 722b6eca2e93..33b932302ece 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4490,6 +4490,10 @@
>  			allocator.  This parameter is primarily	for debugging
>  			and performance comparison.
>  
> +	phat_disable=	[ACPI]
> +			Disable PHAT table parsing and logging of Firmware
> +			Version and Health Data records.
> +
>  	pirq=		[SMP,APIC] Manual mp-table setup
>  			See Documentation/arch/x86/i386/IO-APIC.rst.
>  
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 00dd309b6682..06a7dd6e5a40 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -96,6 +96,15 @@ config ACPI_FPDT
>  	  This table provides information on the timing of the system
>  	  boot, S3 suspend and S3 resume firmware code paths.
>  
> +config ACPI_PHAT
> +	bool "ACPI Platform Health Assessment Table (PHAT) support"
> +	depends on X86_64 || ARM64
> +	help
> +	  Enable support for Platform Health Assessment Table (PHAT).
> +	  This table exposes an extensible set of platform health
> +	  related telemetry through Firmware Version and Firmware Health
> +	  Data Records.
> +
>  config ACPI_LPIT
>  	bool
>  	depends on X86_64
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 3fc5a0d54f6e..93a4ec57ba6d 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -69,6 +69,7 @@ acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
>  acpi-$(CONFIG_ACPI_PRMT)	+= prmt.o
>  acpi-$(CONFIG_ACPI_PCC)		+= acpi_pcc.o
>  acpi-$(CONFIG_ACPI_FFH)		+= acpi_ffh.o
> +acpi-$(CONFIG_ACPI_PHAT)	+= phat.o
>  
>  # Address translation
>  acpi-$(CONFIG_ACPI_ADXL)	+= acpi_adxl.o
> diff --git a/drivers/acpi/phat.c b/drivers/acpi/phat.c
> new file mode 100644
> index 000000000000..6006dd7615fa
> --- /dev/null
> +++ b/drivers/acpi/phat.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Platform Health Assessment Table (PHAT) support
> + *
> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
> + *
> + * Author: Avadhut Naik <avadhut.naik@amd.com>
> + *
> + * This file implements parsing of the Platform Health Assessment Table
> + * through which a platform can expose an extensible set of platform
> + * health related telemetry. The telemetry is exposed through Firmware
> + * Version Data Records and Firmware Health Data Records. Additionally,
> + * a platform, through system firmware, also exposes Reset Reason Health
> + * Record to inform the operating system of the cause of last system
> + * reset or boot.
> + *
> + * For more information on PHAT, please refer to ACPI specification
> + * version 6.5, section 5.2.31
> + */
> +
> +#include <linux/acpi.h>
> +
> +static int phat_disable __initdata;
> +static const char *prefix = "ACPI PHAT: ";

Wouldn't it be better if you used pr_fmt macro instead ?

> +
> +/* Reset Reason Health Record GUID */
> +static const guid_t reset_guid =
> +	GUID_INIT(0x7a014ce2, 0xf263, 0x4b77,
> +		  0xb8, 0x8a, 0xe6, 0x33, 0x6b, 0x78, 0x2c, 0x14);
> +
> +static struct { u8 mask; const char *str; } const reset_sources[] = {
> +	{BIT(0), "Unknown source"},
> +	{BIT(1), "Hardware Source"},
> +	{BIT(2), "Firmware Source"},
> +	{BIT(3), "Software initiated reset"},
> +	{BIT(4), "Supervisor initiated reset"},
> +};
> +
> +static struct { u8 val; const char *str; } const reset_reasons[] = {
> +	{0, "UNKNOWN"},
> +	{1, "COLD BOOT"},
> +	{2, "COLD RESET"},
> +	{3, "WARM RESET"},
> +	{4, "UPDATE"},
> +	{32, "UNEXPECTED RESET"},
> +	{33, "FAULT"},
> +	{34, "TIMEOUT"},
> +	{35, "THERMAL"},
> +	{36, "POWER LOSS"},
> +	{37, "POWER BUTTON"},
> +};
> +
> +/*
> + * Print the last PHAT Version Element associated with a Firmware
> + * Version Data Record.
> + * Firmware Version Data Record consists of an array of PHAT Version
> + * Elements with each entry in the array representing a modification
> + * undertaken on a given platform component.
> + * In the event the array has multiple entries, minimize logs on the
> + * console and print only the last version element since it denotes
> + * the currently running instance of the component.
> + */
> +static int phat_version_data_parse(const char *pfx,
> +				   struct acpi_phat_version_data *version)
> +{
> +	char newpfx[64];
> +	u32 num_elems = version->element_count - 1;
> +	struct acpi_phat_version_element *element;
> +	int offset = sizeof(struct acpi_phat_version_data);
> +
> +	if (!version->element_count) {
> +		pr_info("%sNo PHAT Version Elements found.\n", prefix);
> +		return 0;
> +	}
> +
> +	offset += num_elems * sizeof(struct acpi_phat_version_element);
> +	element = (void *)version + offset;
> +
> +	pr_info("%sPHAT Version Element:\n", pfx);
> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> +	pr_info("%sComponent ID: %pUl\n", newpfx, element->guid);
> +	pr_info("%sVersion: 0x%llx\n", newpfx, element->version_value);
> +	snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> +	print_hex_dump(newpfx, "Producer ID: ", DUMP_PREFIX_NONE, 16, 4,
> +		       &element->producer_id, sizeof(element->producer_id), true);

I do have to admit that all this dancing with pfx and newpfx confuses me. Couldn't you
just use pr_fmt for everything printed using pr_* family of functions ? print_hex_dump()
is not impacted by pr_fmt, as it just uses printk to do it's printing.

> +
> +	return 0;
> +}
> +
> +/*
> + * Print the Reset Reason Health Record
> + */
> +static int phat_reset_reason_parse(const char *pfx,
> +				   struct acpi_phat_health_data *record)
> +{
> +	int idx;
> +	void *data;
> +	u32 data_len;
> +	char newpfx[64];
> +	struct acpi_phat_reset_reason *rr;
> +	struct acpi_phat_vendor_reset_data *vdata;
> +
> +	rr = (void *)record + record->device_specific_offset;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(reset_sources); idx++) {
> +		if (!rr->reset_source) {
> +			pr_info("%sUnknown Reset Source.\n", pfx);
> +			break;
> +		}
> +		if (rr->reset_source & reset_sources[idx].mask) {
> +			pr_info("%sReset Source: 0x%x\t%s\n", pfx, reset_sources[idx].mask,
> +				reset_sources[idx].str);
> +			/* According to ACPI v6.5 Table 5.168, Sub-Source is
> +			 * defined only for Software initiated reset.
> +			 */
> +			if (idx == 0x3 && rr->reset_sub_source)
> +				pr_info("%sReset Sub-Source: %s\n", pfx,
> +					rr->reset_sub_source == 0x1 ?
> +					"Operating System" : "Hypervisor");
> +			break;
> +		}
> +	}
> +
> +	for (idx = 0; idx < ARRAY_SIZE(reset_reasons); idx++) {
> +		if (rr->reset_reason == reset_reasons[idx].val) {
> +			pr_info("%sReset Reason: 0x%x\t%s\n", pfx, reset_reasons[idx].val,
> +				reset_reasons[idx].str);
> +			break;
> +		}
> +	}
> +
> +	if (!rr->vendor_count)
> +		return 0;
> +
> +	pr_info("%sReset Reason Vendor Data:\n", pfx);
> +	vdata = (void *)rr + sizeof(*rr);
> +
> +	for (idx = 0; idx < rr->vendor_count; idx++) {
> +		snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> +		data_len = vdata->length - sizeof(*vdata);
> +		data = (void *)vdata + sizeof(*vdata);
> +		pr_info("%sVendor Data ID: %pUl\n", newpfx, vdata->vendor_id);
> +		pr_info("%sRevision: 0x%x\n", newpfx, vdata->revision);
> +		snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> +		print_hex_dump(newpfx, "Data: ", DUMP_PREFIX_NONE, 16, 4,
> +			       data, data_len, false);
> +		vdata = (void *)vdata + vdata->length;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Print the Firmware Health Data Record.
> + */
> +static int phat_health_data_parse(const char *pfx,
> +				  struct acpi_phat_health_data *record)
> +{
> +	void *data;
> +	u32 data_len;
> +	char newpfx[64];
> +
> +	pr_info("%sHealth Records.\n", pfx);
> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> +	pr_info("%sDevice Signature: %pUl\n", newpfx, record->device_guid);
> +
> +	switch (record->health) {
> +	case ACPI_PHAT_ERRORS_FOUND:
> +		pr_info("%sAmHealthy: Errors found\n", newpfx);
> +		break;
> +	case ACPI_PHAT_NO_ERRORS:
> +		pr_info("%sAmHealthy: No errors found.\n", newpfx);
> +		break;
> +	case ACPI_PHAT_UNKNOWN_ERRORS:
> +		pr_info("%sAmHealthy: Unknown.\n", newpfx);
> +		break;
> +	case ACPI_PHAT_ADVISORY:
> +		pr_info("%sAmHealthy: Advisory – additional device-specific data exposed.\n",
> +			newpfx);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!record->device_specific_offset)
> +		return 0;
> +
> +	/* Reset Reason Health Record has a unique GUID and is created as
> +	 * a Health Record in the PHAT table. Check if this Health Record
> +	 * is a Reset Reason Health Record.
> +	 */
> +	if (guid_equal((guid_t *)record->device_guid, &reset_guid)) {
> +		phat_reset_reason_parse(newpfx, record);
> +		return 0;
> +	}
> +
> +	data = (void *)record + record->device_specific_offset;
> +	data_len = record->header.length - record->device_specific_offset;
> +	snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> +	print_hex_dump(newpfx, "Device Data: ", DUMP_PREFIX_NONE, 16, 4,
> +		       data, data_len, false);
> +
> +	return 0;
> +}
> +
> +static int parse_phat_table(const char *pfx, struct acpi_table_phat *phat_tab)
> +{
> +	char newpfx[64];
> +	u32 offset = sizeof(*phat_tab);
> +	struct acpi_phat_header *phat_header;
> +
> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> +
> +	while (offset < phat_tab->header.length) {
> +		phat_header = (void *)phat_tab + offset;
> +		switch (phat_header->type) {
> +		case ACPI_PHAT_TYPE_FW_VERSION_DATA:
> +			phat_version_data_parse(newpfx, (struct acpi_phat_version_data *)
> +			    phat_header);
> +			break;
> +		case ACPI_PHAT_TYPE_FW_HEALTH_DATA:
> +			phat_health_data_parse(newpfx, (struct acpi_phat_health_data *)
> +			    phat_header);
> +			break;
> +		default:
> +			break;
> +		}
> +		offset += phat_header->length;
> +	}
> +	return 0;
> +}
> +
> +static int __init setup_phat_disable(char *str)
> +{
> +	phat_disable = 1;
> +	return 1;
> +}
> +__setup("phat_disable", setup_phat_disable);
> +
> +static int __init acpi_phat_init(void)
> +{
> +	acpi_status status;
> +	struct acpi_table_phat *phat_tab;
> +
> +	if (acpi_disabled)
> +		return 0;
> +
> +	if (phat_disable) {
> +		pr_err("%sPHAT support has been disabled.\n", prefix);
> +		return 0;
> +	}
> +
> +	status = acpi_get_table(ACPI_SIG_PHAT, 0,
> +				(struct acpi_table_header **)&phat_tab);
> +
> +	if (status == AE_NOT_FOUND) {
> +		pr_info("%sPHAT Table not found.\n", prefix);
> +		return 0;
> +	} else if (ACPI_FAILURE(status)) {
> +		pr_err("%sFailed to get PHAT Table: %s.\n", prefix,
> +		       acpi_format_exception(status));
> +		return -EINVAL;
> +	}
> +
> +	pr_info("%sPlatform Telemetry Records.\n", prefix);
> +	parse_phat_table(prefix, phat_tab);

So for now you're only dumping tables to the dmesg output ?
Are you planning to create some sysfs interfaces similar to let's
say EINJ ?

> +
> +	return 0;
> +}
> +late_initcall(acpi_phat_init);
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index 0029336775a9..c263893cbc7f 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -2360,6 +2360,24 @@ struct acpi_phat_health_data {
>  #define ACPI_PHAT_UNKNOWN_ERRORS        2
>  #define ACPI_PHAT_ADVISORY              3
>  
> +/* Reset Reason Health Record Structure */
> +
> +struct acpi_phat_reset_reason {
> +	u8 supported_reset_sources;
> +	u8 reset_source;
> +	u8 reset_sub_source;
> +	u8 reset_reason;
> +	u16 vendor_count;
> +};
> +
> +/* Reset Reason Health Record Vendor Data Entry */
> +
> +struct acpi_phat_vendor_reset_data {
> +	u8 vendor_id[16];
> +	u16 length;
> +	u16 revision;
> +};
> +
>  /*******************************************************************************
>   *
>   * PMTT - Platform Memory Topology Table (ACPI 5.0)

Hi,

On 8/16/2023 06:35, Wilczynski, Michal wrote:
> Hi,
> 
> On 8/11/2023 1:48 AM, Avadhut Naik wrote:
>> ACPI Platform Health Assessment Table (PHAT) enables a platform to expose
>> an extensible set of platform health related telemetry. The telemetry is
>> exposed through Firmware Version and Firmware Health Data Records which
>> provide version data and health-related information of their associated
>> components respectively.
>>
>> Additionally, the platform also provides Reset Reason Health Record in
>> the PHAT table highlighting the cause of last system reset or boot in case
>> of both expected and unexpected events. Vendor-specific data capturing the
>> underlying state of the system during reset can also be optionally provided
>> through the record.[1]
>>
>> Add support to parse the PHAT table during system bootup and have its
>> information logged into the dmesg buffer.
>>
>> [1] ACPI specification 6.5, section 5.2.31.5
>>
>> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
>> ---
>>  .../admin-guide/kernel-parameters.txt         |   4 +
>>  drivers/acpi/Kconfig                          |   9 +
>>  drivers/acpi/Makefile                         |   1 +
>>  drivers/acpi/phat.c                           | 270 ++++++++++++++++++
>>  include/acpi/actbl2.h                         |  18 ++
>>  5 files changed, 302 insertions(+)
>>  create mode 100644 drivers/acpi/phat.c
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 722b6eca2e93..33b932302ece 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4490,6 +4490,10 @@
>>  			allocator.  This parameter is primarily	for debugging
>>  			and performance comparison.
>>  
>> +	phat_disable=	[ACPI]
>> +			Disable PHAT table parsing and logging of Firmware
>> +			Version and Health Data records.
>> +
>>  	pirq=		[SMP,APIC] Manual mp-table setup
>>  			See Documentation/arch/x86/i386/IO-APIC.rst.
>>  
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 00dd309b6682..06a7dd6e5a40 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -96,6 +96,15 @@ config ACPI_FPDT
>>  	  This table provides information on the timing of the system
>>  	  boot, S3 suspend and S3 resume firmware code paths.
>>  
>> +config ACPI_PHAT
>> +	bool "ACPI Platform Health Assessment Table (PHAT) support"
>> +	depends on X86_64 || ARM64
>> +	help
>> +	  Enable support for Platform Health Assessment Table (PHAT).
>> +	  This table exposes an extensible set of platform health
>> +	  related telemetry through Firmware Version and Firmware Health
>> +	  Data Records.
>> +
>>  config ACPI_LPIT
>>  	bool
>>  	depends on X86_64
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 3fc5a0d54f6e..93a4ec57ba6d 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -69,6 +69,7 @@ acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
>>  acpi-$(CONFIG_ACPI_PRMT)	+= prmt.o
>>  acpi-$(CONFIG_ACPI_PCC)		+= acpi_pcc.o
>>  acpi-$(CONFIG_ACPI_FFH)		+= acpi_ffh.o
>> +acpi-$(CONFIG_ACPI_PHAT)	+= phat.o
>>  
>>  # Address translation
>>  acpi-$(CONFIG_ACPI_ADXL)	+= acpi_adxl.o
>> diff --git a/drivers/acpi/phat.c b/drivers/acpi/phat.c
>> new file mode 100644
>> index 000000000000..6006dd7615fa
>> --- /dev/null
>> +++ b/drivers/acpi/phat.c
>> @@ -0,0 +1,270 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Platform Health Assessment Table (PHAT) support
>> + *
>> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Avadhut Naik <avadhut.naik@amd.com>
>> + *
>> + * This file implements parsing of the Platform Health Assessment Table
>> + * through which a platform can expose an extensible set of platform
>> + * health related telemetry. The telemetry is exposed through Firmware
>> + * Version Data Records and Firmware Health Data Records. Additionally,
>> + * a platform, through system firmware, also exposes Reset Reason Health
>> + * Record to inform the operating system of the cause of last system
>> + * reset or boot.
>> + *
>> + * For more information on PHAT, please refer to ACPI specification
>> + * version 6.5, section 5.2.31
>> + */
>> +
>> +#include <linux/acpi.h>
>> +
>> +static int phat_disable __initdata;
>> +static const char *prefix = "ACPI PHAT: ";
> 
> Wouldn't it be better if you used pr_fmt macro instead ?
> 
	Have explained below.
>> +
>> +/* Reset Reason Health Record GUID */
>> +static const guid_t reset_guid =
>> +	GUID_INIT(0x7a014ce2, 0xf263, 0x4b77,
>> +		  0xb8, 0x8a, 0xe6, 0x33, 0x6b, 0x78, 0x2c, 0x14);
>> +
>> +static struct { u8 mask; const char *str; } const reset_sources[] = {
>> +	{BIT(0), "Unknown source"},
>> +	{BIT(1), "Hardware Source"},
>> +	{BIT(2), "Firmware Source"},
>> +	{BIT(3), "Software initiated reset"},
>> +	{BIT(4), "Supervisor initiated reset"},
>> +};
>> +
>> +static struct { u8 val; const char *str; } const reset_reasons[] = {
>> +	{0, "UNKNOWN"},
>> +	{1, "COLD BOOT"},
>> +	{2, "COLD RESET"},
>> +	{3, "WARM RESET"},
>> +	{4, "UPDATE"},
>> +	{32, "UNEXPECTED RESET"},
>> +	{33, "FAULT"},
>> +	{34, "TIMEOUT"},
>> +	{35, "THERMAL"},
>> +	{36, "POWER LOSS"},
>> +	{37, "POWER BUTTON"},
>> +};
>> +
>> +/*
>> + * Print the last PHAT Version Element associated with a Firmware
>> + * Version Data Record.
>> + * Firmware Version Data Record consists of an array of PHAT Version
>> + * Elements with each entry in the array representing a modification
>> + * undertaken on a given platform component.
>> + * In the event the array has multiple entries, minimize logs on the
>> + * console and print only the last version element since it denotes
>> + * the currently running instance of the component.
>> + */
>> +static int phat_version_data_parse(const char *pfx,
>> +				   struct acpi_phat_version_data *version)
>> +{
>> +	char newpfx[64];
>> +	u32 num_elems = version->element_count - 1;
>> +	struct acpi_phat_version_element *element;
>> +	int offset = sizeof(struct acpi_phat_version_data);
>> +
>> +	if (!version->element_count) {
>> +		pr_info("%sNo PHAT Version Elements found.\n", prefix);
>> +		return 0;
>> +	}
>> +
>> +	offset += num_elems * sizeof(struct acpi_phat_version_element);
>> +	element = (void *)version + offset;
>> +
>> +	pr_info("%sPHAT Version Element:\n", pfx);
>> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>> +	pr_info("%sComponent ID: %pUl\n", newpfx, element->guid);
>> +	pr_info("%sVersion: 0x%llx\n", newpfx, element->version_value);
>> +	snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
>> +	print_hex_dump(newpfx, "Producer ID: ", DUMP_PREFIX_NONE, 16, 4,
>> +		       &element->producer_id, sizeof(element->producer_id), true);
> 
> I do have to admit that all this dancing with pfx and newpfx confuses me. Couldn't you
> just use pr_fmt for everything printed using pr_* family of functions ? print_hex_dump()
> is not impacted by pr_fmt, as it just uses printk to do it's printing.
> 
I had considered using pr_fmt initially but since the ACPI spec says that PHAT health records,
especially reset reason health record is intended to complement existing fault reporting
mechanisms like BERT Tables, CPER, decided to have their outputs in identical formats, like has
been implemented in cper_estatus_print().

>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Print the Reset Reason Health Record
>> + */
>> +static int phat_reset_reason_parse(const char *pfx,
>> +				   struct acpi_phat_health_data *record)
>> +{
>> +	int idx;
>> +	void *data;
>> +	u32 data_len;
>> +	char newpfx[64];
>> +	struct acpi_phat_reset_reason *rr;
>> +	struct acpi_phat_vendor_reset_data *vdata;
>> +
>> +	rr = (void *)record + record->device_specific_offset;
>> +
>> +	for (idx = 0; idx < ARRAY_SIZE(reset_sources); idx++) {
>> +		if (!rr->reset_source) {
>> +			pr_info("%sUnknown Reset Source.\n", pfx);
>> +			break;
>> +		}
>> +		if (rr->reset_source & reset_sources[idx].mask) {
>> +			pr_info("%sReset Source: 0x%x\t%s\n", pfx, reset_sources[idx].mask,
>> +				reset_sources[idx].str);
>> +			/* According to ACPI v6.5 Table 5.168, Sub-Source is
>> +			 * defined only for Software initiated reset.
>> +			 */
>> +			if (idx == 0x3 && rr->reset_sub_source)
>> +				pr_info("%sReset Sub-Source: %s\n", pfx,
>> +					rr->reset_sub_source == 0x1 ?
>> +					"Operating System" : "Hypervisor");
>> +			break;
>> +		}
>> +	}
>> +
>> +	for (idx = 0; idx < ARRAY_SIZE(reset_reasons); idx++) {
>> +		if (rr->reset_reason == reset_reasons[idx].val) {
>> +			pr_info("%sReset Reason: 0x%x\t%s\n", pfx, reset_reasons[idx].val,
>> +				reset_reasons[idx].str);
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!rr->vendor_count)
>> +		return 0;
>> +
>> +	pr_info("%sReset Reason Vendor Data:\n", pfx);
>> +	vdata = (void *)rr + sizeof(*rr);
>> +
>> +	for (idx = 0; idx < rr->vendor_count; idx++) {
>> +		snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>> +		data_len = vdata->length - sizeof(*vdata);
>> +		data = (void *)vdata + sizeof(*vdata);
>> +		pr_info("%sVendor Data ID: %pUl\n", newpfx, vdata->vendor_id);
>> +		pr_info("%sRevision: 0x%x\n", newpfx, vdata->revision);
>> +		snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
>> +		print_hex_dump(newpfx, "Data: ", DUMP_PREFIX_NONE, 16, 4,
>> +			       data, data_len, false);
>> +		vdata = (void *)vdata + vdata->length;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Print the Firmware Health Data Record.
>> + */
>> +static int phat_health_data_parse(const char *pfx,
>> +				  struct acpi_phat_health_data *record)
>> +{
>> +	void *data;
>> +	u32 data_len;
>> +	char newpfx[64];
>> +
>> +	pr_info("%sHealth Records.\n", pfx);
>> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>> +	pr_info("%sDevice Signature: %pUl\n", newpfx, record->device_guid);
>> +
>> +	switch (record->health) {
>> +	case ACPI_PHAT_ERRORS_FOUND:
>> +		pr_info("%sAmHealthy: Errors found\n", newpfx);
>> +		break;
>> +	case ACPI_PHAT_NO_ERRORS:
>> +		pr_info("%sAmHealthy: No errors found.\n", newpfx);
>> +		break;
>> +	case ACPI_PHAT_UNKNOWN_ERRORS:
>> +		pr_info("%sAmHealthy: Unknown.\n", newpfx);
>> +		break;
>> +	case ACPI_PHAT_ADVISORY:
>> +		pr_info("%sAmHealthy: Advisory – additional device-specific data exposed.\n",
>> +			newpfx);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	if (!record->device_specific_offset)
>> +		return 0;
>> +
>> +	/* Reset Reason Health Record has a unique GUID and is created as
>> +	 * a Health Record in the PHAT table. Check if this Health Record
>> +	 * is a Reset Reason Health Record.
>> +	 */
>> +	if (guid_equal((guid_t *)record->device_guid, &reset_guid)) {
>> +		phat_reset_reason_parse(newpfx, record);
>> +		return 0;
>> +	}
>> +
>> +	data = (void *)record + record->device_specific_offset;
>> +	data_len = record->header.length - record->device_specific_offset;
>> +	snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
>> +	print_hex_dump(newpfx, "Device Data: ", DUMP_PREFIX_NONE, 16, 4,
>> +		       data, data_len, false);
>> +
>> +	return 0;
>> +}
>> +
>> +static int parse_phat_table(const char *pfx, struct acpi_table_phat *phat_tab)
>> +{
>> +	char newpfx[64];
>> +	u32 offset = sizeof(*phat_tab);
>> +	struct acpi_phat_header *phat_header;
>> +
>> +	snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>> +
>> +	while (offset < phat_tab->header.length) {
>> +		phat_header = (void *)phat_tab + offset;
>> +		switch (phat_header->type) {
>> +		case ACPI_PHAT_TYPE_FW_VERSION_DATA:
>> +			phat_version_data_parse(newpfx, (struct acpi_phat_version_data *)
>> +			    phat_header);
>> +			break;
>> +		case ACPI_PHAT_TYPE_FW_HEALTH_DATA:
>> +			phat_health_data_parse(newpfx, (struct acpi_phat_health_data *)
>> +			    phat_header);
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +		offset += phat_header->length;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int __init setup_phat_disable(char *str)
>> +{
>> +	phat_disable = 1;
>> +	return 1;
>> +}
>> +__setup("phat_disable", setup_phat_disable);
>> +
>> +static int __init acpi_phat_init(void)
>> +{
>> +	acpi_status status;
>> +	struct acpi_table_phat *phat_tab;
>> +
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	if (phat_disable) {
>> +		pr_err("%sPHAT support has been disabled.\n", prefix);
>> +		return 0;
>> +	}
>> +
>> +	status = acpi_get_table(ACPI_SIG_PHAT, 0,
>> +				(struct acpi_table_header **)&phat_tab);
>> +
>> +	if (status == AE_NOT_FOUND) {
>> +		pr_info("%sPHAT Table not found.\n", prefix);
>> +		return 0;
>> +	} else if (ACPI_FAILURE(status)) {
>> +		pr_err("%sFailed to get PHAT Table: %s.\n", prefix,
>> +		       acpi_format_exception(status));
>> +		return -EINVAL;
>> +	}
>> +
>> +	pr_info("%sPlatform Telemetry Records.\n", prefix);
>> +	parse_phat_table(prefix, phat_tab);
> 
> So for now you're only dumping tables to the dmesg output ?
> Are you planning to create some sysfs interfaces similar to let's
> say EINJ ?
> 
Yes, for now, the output is being posted to dmesg only. If there is a
consensus, we can have the information exported through sysfs too.
The below location may be appropriate in that case:
/sys/firmware/acpi/
We already have FPDT and BGRT being exported from there.

Thanks,
Avadhut Naik
>> +
>> +	return 0;
>> +}
>> +late_initcall(acpi_phat_init);
>> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
>> index 0029336775a9..c263893cbc7f 100644
>> --- a/include/acpi/actbl2.h
>> +++ b/include/acpi/actbl2.h
>> @@ -2360,6 +2360,24 @@ struct acpi_phat_health_data {
>>  #define ACPI_PHAT_UNKNOWN_ERRORS        2
>>  #define ACPI_PHAT_ADVISORY              3
>>  
>> +/* Reset Reason Health Record Structure */
>> +
>> +struct acpi_phat_reset_reason {
>> +	u8 supported_reset_sources;
>> +	u8 reset_source;
>> +	u8 reset_sub_source;
>> +	u8 reset_reason;
>> +	u16 vendor_count;
>> +};
>> +
>> +/* Reset Reason Health Record Vendor Data Entry */
>> +
>> +struct acpi_phat_vendor_reset_data {
>> +	u8 vendor_id[16];
>> +	u16 length;
>> +	u16 revision;
>> +};
>> +
>>  /*******************************************************************************
>>   *
>>   * PMTT - Platform Memory Topology Table (ACPI 5.0)
> 

--

On 8/18/2023 2:49 AM, Rafael J. Wysocki wrote:
> On Thu, Aug 17, 2023 at 10:43 PM Avadhut Naik <avadnaik@amd.com> wrote:
>>
>> Hi,
>>
>> On 8/16/2023 06:35, Wilczynski, Michal wrote:
>>> Hi,
>>>
>>> On 8/11/2023 1:48 AM, Avadhut Naik wrote:
>>>> ACPI Platform Health Assessment Table (PHAT) enables a platform to expose
>>>> an extensible set of platform health related telemetry. The telemetry is
>>>> exposed through Firmware Version and Firmware Health Data Records which
>>>> provide version data and health-related information of their associated
>>>> components respectively.
>>>>
>>>> Additionally, the platform also provides Reset Reason Health Record in
>>>> the PHAT table highlighting the cause of last system reset or boot in case
>>>> of both expected and unexpected events. Vendor-specific data capturing the
>>>> underlying state of the system during reset can also be optionally provided
>>>> through the record.[1]
>>>>
>>>> Add support to parse the PHAT table during system bootup and have its
>>>> information logged into the dmesg buffer.
>>>>
>>>> [1] ACPI specification 6.5, section 5.2.31.5
>>>>
>>>> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
>>>> ---
>>>>   .../admin-guide/kernel-parameters.txt         |   4 +
>>>>   drivers/acpi/Kconfig                          |   9 +
>>>>   drivers/acpi/Makefile                         |   1 +
>>>>   drivers/acpi/phat.c                           | 270 ++++++++++++++++++
>>>>   include/acpi/actbl2.h                         |  18 ++
>>>>   5 files changed, 302 insertions(+)
>>>>   create mode 100644 drivers/acpi/phat.c
>>>>
>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>>> index 722b6eca2e93..33b932302ece 100644
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -4490,6 +4490,10 @@
>>>>                       allocator.  This parameter is primarily for debugging
>>>>                       and performance comparison.
>>>>
>>>> +    phat_disable=   [ACPI]
>>>> +                    Disable PHAT table parsing and logging of Firmware
>>>> +                    Version and Health Data records.
>>>> +
>>>>       pirq=           [SMP,APIC] Manual mp-table setup
>>>>                       See Documentation/arch/x86/i386/IO-APIC.rst.
>>>>
>>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>>> index 00dd309b6682..06a7dd6e5a40 100644
>>>> --- a/drivers/acpi/Kconfig
>>>> +++ b/drivers/acpi/Kconfig
>>>> @@ -96,6 +96,15 @@ config ACPI_FPDT
>>>>         This table provides information on the timing of the system
>>>>         boot, S3 suspend and S3 resume firmware code paths.
>>>>
>>>> +config ACPI_PHAT
>>>> +    bool "ACPI Platform Health Assessment Table (PHAT) support"
>>>> +    depends on X86_64 || ARM64
>>>> +    help
>>>> +      Enable support for Platform Health Assessment Table (PHAT).
>>>> +      This table exposes an extensible set of platform health
>>>> +      related telemetry through Firmware Version and Firmware Health
>>>> +      Data Records.
>>>> +
>>>>   config ACPI_LPIT
>>>>       bool
>>>>       depends on X86_64
>>>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>>>> index 3fc5a0d54f6e..93a4ec57ba6d 100644
>>>> --- a/drivers/acpi/Makefile
>>>> +++ b/drivers/acpi/Makefile
>>>> @@ -69,6 +69,7 @@ acpi-$(CONFIG_ACPI_WATCHDOG)       += acpi_watchdog.o
>>>>   acpi-$(CONFIG_ACPI_PRMT)    += prmt.o
>>>>   acpi-$(CONFIG_ACPI_PCC)             += acpi_pcc.o
>>>>   acpi-$(CONFIG_ACPI_FFH)             += acpi_ffh.o
>>>> +acpi-$(CONFIG_ACPI_PHAT)    += phat.o
>>>>
>>>>   # Address translation
>>>>   acpi-$(CONFIG_ACPI_ADXL)    += acpi_adxl.o
>>>> diff --git a/drivers/acpi/phat.c b/drivers/acpi/phat.c
>>>> new file mode 100644
>>>> index 000000000000..6006dd7615fa
>>>> --- /dev/null
>>>> +++ b/drivers/acpi/phat.c
>>>> @@ -0,0 +1,270 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Platform Health Assessment Table (PHAT) support
>>>> + *
>>>> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
>>>> + *
>>>> + * Author: Avadhut Naik <avadhut.naik@amd.com>
>>>> + *
>>>> + * This file implements parsing of the Platform Health Assessment Table
>>>> + * through which a platform can expose an extensible set of platform
>>>> + * health related telemetry. The telemetry is exposed through Firmware
>>>> + * Version Data Records and Firmware Health Data Records. Additionally,
>>>> + * a platform, through system firmware, also exposes Reset Reason Health
>>>> + * Record to inform the operating system of the cause of last system
>>>> + * reset or boot.
>>>> + *
>>>> + * For more information on PHAT, please refer to ACPI specification
>>>> + * version 6.5, section 5.2.31
>>>> + */
>>>> +
>>>> +#include <linux/acpi.h>
>>>> +
>>>> +static int phat_disable __initdata;
>>>> +static const char *prefix = "ACPI PHAT: ";
>>>
>>> Wouldn't it be better if you used pr_fmt macro instead ?
>>>
>>          Have explained below.
>>>> +
>>>> +/* Reset Reason Health Record GUID */
>>>> +static const guid_t reset_guid =
>>>> +    GUID_INIT(0x7a014ce2, 0xf263, 0x4b77,
>>>> +              0xb8, 0x8a, 0xe6, 0x33, 0x6b, 0x78, 0x2c, 0x14);
>>>> +
>>>> +static struct { u8 mask; const char *str; } const reset_sources[] = {
>>>> +    {BIT(0), "Unknown source"},
>>>> +    {BIT(1), "Hardware Source"},
>>>> +    {BIT(2), "Firmware Source"},
>>>> +    {BIT(3), "Software initiated reset"},
>>>> +    {BIT(4), "Supervisor initiated reset"},
>>>> +};
>>>> +
>>>> +static struct { u8 val; const char *str; } const reset_reasons[] = {
>>>> +    {0, "UNKNOWN"},
>>>> +    {1, "COLD BOOT"},
>>>> +    {2, "COLD RESET"},
>>>> +    {3, "WARM RESET"},
>>>> +    {4, "UPDATE"},
>>>> +    {32, "UNEXPECTED RESET"},
>>>> +    {33, "FAULT"},
>>>> +    {34, "TIMEOUT"},
>>>> +    {35, "THERMAL"},
>>>> +    {36, "POWER LOSS"},
>>>> +    {37, "POWER BUTTON"},
>>>> +};
>>>> +
>>>> +/*
>>>> + * Print the last PHAT Version Element associated with a Firmware
>>>> + * Version Data Record.
>>>> + * Firmware Version Data Record consists of an array of PHAT Version
>>>> + * Elements with each entry in the array representing a modification
>>>> + * undertaken on a given platform component.
>>>> + * In the event the array has multiple entries, minimize logs on the
>>>> + * console and print only the last version element since it denotes
>>>> + * the currently running instance of the component.
>>>> + */
>>>> +static int phat_version_data_parse(const char *pfx,
>>>> +                               struct acpi_phat_version_data *version)
>>>> +{
>>>> +    char newpfx[64];
>>>> +    u32 num_elems = version->element_count - 1;
>>>> +    struct acpi_phat_version_element *element;
>>>> +    int offset = sizeof(struct acpi_phat_version_data);
>>>> +
>>>> +    if (!version->element_count) {
>>>> +            pr_info("%sNo PHAT Version Elements found.\n", prefix);
>>>> +            return 0;
>>>> +    }
>>>> +
>>>> +    offset += num_elems * sizeof(struct acpi_phat_version_element);
>>>> +    element = (void *)version + offset;
>>>> +
>>>> +    pr_info("%sPHAT Version Element:\n", pfx);
>>>> +    snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>>>> +    pr_info("%sComponent ID: %pUl\n", newpfx, element->guid);
>>>> +    pr_info("%sVersion: 0x%llx\n", newpfx, element->version_value);
>>>> +    snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
>>>> +    print_hex_dump(newpfx, "Producer ID: ", DUMP_PREFIX_NONE, 16, 4,
>>>> +                   &element->producer_id, sizeof(element->producer_id), true);
>>>
>>> I do have to admit that all this dancing with pfx and newpfx confuses me. Couldn't you
>>> just use pr_fmt for everything printed using pr_* family of functions ? print_hex_dump()
>>> is not impacted by pr_fmt, as it just uses printk to do it's printing.
>>>
>> I had considered using pr_fmt initially but since the ACPI spec says that PHAT health records,
>> especially reset reason health record is intended to complement existing fault reporting
>> mechanisms like BERT Tables, CPER, decided to have their outputs in identical formats, like has
>> been implemented in cper_estatus_print().
>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Print the Reset Reason Health Record
>>>> + */
>>>> +static int phat_reset_reason_parse(const char *pfx,
>>>> +                               struct acpi_phat_health_data *record)
>>>> +{
>>>> +    int idx;
>>>> +    void *data;
>>>> +    u32 data_len;
>>>> +    char newpfx[64];
>>>> +    struct acpi_phat_reset_reason *rr;
>>>> +    struct acpi_phat_vendor_reset_data *vdata;
>>>> +
>>>> +    rr = (void *)record + record->device_specific_offset;
>>>> +
>>>> +    for (idx = 0; idx < ARRAY_SIZE(reset_sources); idx++) {
>>>> +            if (!rr->reset_source) {
>>>> +                    pr_info("%sUnknown Reset Source.\n", pfx);
>>>> +                    break;
>>>> +            }
>>>> +            if (rr->reset_source & reset_sources[idx].mask) {
>>>> +                    pr_info("%sReset Source: 0x%x\t%s\n", pfx, reset_sources[idx].mask,
>>>> +                            reset_sources[idx].str);
>>>> +                    /* According to ACPI v6.5 Table 5.168, Sub-Source is
>>>> +                     * defined only for Software initiated reset.
>>>> +                     */
>>>> +                    if (idx == 0x3 && rr->reset_sub_source)
>>>> +                            pr_info("%sReset Sub-Source: %s\n", pfx,
>>>> +                                    rr->reset_sub_source == 0x1 ?
>>>> +                                    "Operating System" : "Hypervisor");
>>>> +                    break;
>>>> +            }
>>>> +    }
>>>> +
>>>> +    for (idx = 0; idx < ARRAY_SIZE(reset_reasons); idx++) {
>>>> +            if (rr->reset_reason == reset_reasons[idx].val) {
>>>> +                    pr_info("%sReset Reason: 0x%x\t%s\n", pfx, reset_reasons[idx].val,
>>>> +                            reset_reasons[idx].str);
>>>> +                    break;
>>>> +            }
>>>> +    }
>>>> +
>>>> +    if (!rr->vendor_count)
>>>> +            return 0;
>>>> +
>>>> +    pr_info("%sReset Reason Vendor Data:\n", pfx);
>>>> +    vdata = (void *)rr + sizeof(*rr);
>>>> +
>>>> +    for (idx = 0; idx < rr->vendor_count; idx++) {
>>>> +            snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>>>> +            data_len = vdata->length - sizeof(*vdata);
>>>> +            data = (void *)vdata + sizeof(*vdata);
>>>> +            pr_info("%sVendor Data ID: %pUl\n", newpfx, vdata->vendor_id);
>>>> +            pr_info("%sRevision: 0x%x\n", newpfx, vdata->revision);
>>>> +            snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
>>>> +            print_hex_dump(newpfx, "Data: ", DUMP_PREFIX_NONE, 16, 4,
>>>> +                           data, data_len, false);
>>>> +            vdata = (void *)vdata + vdata->length;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Print the Firmware Health Data Record.
>>>> + */
>>>> +static int phat_health_data_parse(const char *pfx,
>>>> +                              struct acpi_phat_health_data *record)
>>>> +{
>>>> +    void *data;
>>>> +    u32 data_len;
>>>> +    char newpfx[64];
>>>> +
>>>> +    pr_info("%sHealth Records.\n", pfx);
>>>> +    snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>>>> +    pr_info("%sDevice Signature: %pUl\n", newpfx, record->device_guid);
>>>> +
>>>> +    switch (record->health) {
>>>> +    case ACPI_PHAT_ERRORS_FOUND:
>>>> +            pr_info("%sAmHealthy: Errors found\n", newpfx);
>>>> +            break;
>>>> +    case ACPI_PHAT_NO_ERRORS:
>>>> +            pr_info("%sAmHealthy: No errors found.\n", newpfx);
>>>> +            break;
>>>> +    case ACPI_PHAT_UNKNOWN_ERRORS:
>>>> +            pr_info("%sAmHealthy: Unknown.\n", newpfx);
>>>> +            break;
>>>> +    case ACPI_PHAT_ADVISORY:
>>>> +            pr_info("%sAmHealthy: Advisory – additional device-specific data exposed.\n",
>>>> +                    newpfx);
>>>> +            break;
>>>> +    default:
>>>> +            break;
>>>> +    }
>>>> +
>>>> +    if (!record->device_specific_offset)
>>>> +            return 0;
>>>> +
>>>> +    /* Reset Reason Health Record has a unique GUID and is created as
>>>> +     * a Health Record in the PHAT table. Check if this Health Record
>>>> +     * is a Reset Reason Health Record.
>>>> +     */
>>>> +    if (guid_equal((guid_t *)record->device_guid, &reset_guid)) {
>>>> +            phat_reset_reason_parse(newpfx, record);
>>>> +            return 0;
>>>> +    }
>>>> +
>>>> +    data = (void *)record + record->device_specific_offset;
>>>> +    data_len = record->header.length - record->device_specific_offset;
>>>> +    snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
>>>> +    print_hex_dump(newpfx, "Device Data: ", DUMP_PREFIX_NONE, 16, 4,
>>>> +                   data, data_len, false);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int parse_phat_table(const char *pfx, struct acpi_table_phat *phat_tab)
>>>> +{
>>>> +    char newpfx[64];
>>>> +    u32 offset = sizeof(*phat_tab);
>>>> +    struct acpi_phat_header *phat_header;
>>>> +
>>>> +    snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>>>> +
>>>> +    while (offset < phat_tab->header.length) {
>>>> +            phat_header = (void *)phat_tab + offset;
>>>> +            switch (phat_header->type) {
>>>> +            case ACPI_PHAT_TYPE_FW_VERSION_DATA:
>>>> +                    phat_version_data_parse(newpfx, (struct acpi_phat_version_data *)
>>>> +                        phat_header);
>>>> +                    break;
>>>> +            case ACPI_PHAT_TYPE_FW_HEALTH_DATA:
>>>> +                    phat_health_data_parse(newpfx, (struct acpi_phat_health_data *)
>>>> +                        phat_header);
>>>> +                    break;
>>>> +            default:
>>>> +                    break;
>>>> +            }
>>>> +            offset += phat_header->length;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int __init setup_phat_disable(char *str)
>>>> +{
>>>> +    phat_disable = 1;
>>>> +    return 1;
>>>> +}
>>>> +__setup("phat_disable", setup_phat_disable);
>>>> +
>>>> +static int __init acpi_phat_init(void)
>>>> +{
>>>> +    acpi_status status;
>>>> +    struct acpi_table_phat *phat_tab;
>>>> +
>>>> +    if (acpi_disabled)
>>>> +            return 0;
>>>> +
>>>> +    if (phat_disable) {
>>>> +            pr_err("%sPHAT support has been disabled.\n", prefix);
>>>> +            return 0;
>>>> +    }
>>>> +
>>>> +    status = acpi_get_table(ACPI_SIG_PHAT, 0,
>>>> +                            (struct acpi_table_header **)&phat_tab);
>>>> +
>>>> +    if (status == AE_NOT_FOUND) {
>>>> +            pr_info("%sPHAT Table not found.\n", prefix);
>>>> +            return 0;
>>>> +    } else if (ACPI_FAILURE(status)) {
>>>> +            pr_err("%sFailed to get PHAT Table: %s.\n", prefix,
>>>> +                   acpi_format_exception(status));
>>>> +            return -EINVAL;
>>>> +    }
>>>> +
>>>> +    pr_info("%sPlatform Telemetry Records.\n", prefix);
>>>> +    parse_phat_table(prefix, phat_tab);
>>>
>>> So for now you're only dumping tables to the dmesg output ?
>>> Are you planning to create some sysfs interfaces similar to let's
>>> say EINJ ?
>>>
>> Yes, for now, the output is being posted to dmesg only.
> 
> So it is not particularly useful for anything practical.
> 
>> If there is a consensus, we can have the information exported through
>> sysfs too.
> 
> That depends on how you want to use it which should be explained in
> the patch changelog.
> 
> Now it's basically "it's there, so dump it" which leads to the obvious
> question: "Who's going to need it?"
> 

I was just talking to some colleagues about PHAT recently as well.

The use case that jumps out is "system randomly rebooted while I was 
doing XYZ".  You don't know what happened, but you keep using your 
system.  Then it happens again.

If the reason for the random reboot is captured to dmesg you can cross 
reference your journal from the next boot after any random reboot and 
get the reason for it.  If a user reports this to a Gitlab issue tracker 
or Bugzilla it can be helpful in establishing a pattern.

>> The below location may be appropriate in that case:
>> /sys/firmware/acpi/
> 
> Yes, it may. >
>> We already have FPDT and BGRT being exported from there.
> 
> In fact, all of the ACPI tables can be retrieved verbatim from
> /sys/firmware/acpi/tables/ already, so why exactly do you want the
> kernel to parse PHAT in particular?
> 

It's not to say that /sys/firmware/acpi/PHAT isn't useful, but having 
something internal to the kernel "automatically" parsing it and saving
information to a place like the kernel log that is already captured by 
existing userspace tools I think is "more" useful.

On Mon, Aug 21, 2023 at 7:06 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
>
>
> On 8/18/2023 2:49 AM, Rafael J. Wysocki wrote:
> > On Thu, Aug 17, 2023 at 10:43 PM Avadhut Naik <avadnaik@amd.com> wrote:
> >>
> >> Hi,
> >>
> >> On 8/16/2023 06:35, Wilczynski, Michal wrote:
> >>> Hi,
> >>>
> >>> On 8/11/2023 1:48 AM, Avadhut Naik wrote:
> >>>> ACPI Platform Health Assessment Table (PHAT) enables a platform to expose
> >>>> an extensible set of platform health related telemetry. The telemetry is
> >>>> exposed through Firmware Version and Firmware Health Data Records which
> >>>> provide version data and health-related information of their associated
> >>>> components respectively.
> >>>>
> >>>> Additionally, the platform also provides Reset Reason Health Record in
> >>>> the PHAT table highlighting the cause of last system reset or boot in case
> >>>> of both expected and unexpected events. Vendor-specific data capturing the
> >>>> underlying state of the system during reset can also be optionally provided
> >>>> through the record.[1]
> >>>>
> >>>> Add support to parse the PHAT table during system bootup and have its
> >>>> information logged into the dmesg buffer.
> >>>>
> >>>> [1] ACPI specification 6.5, section 5.2.31.5
> >>>>
> >>>> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
> >>>> ---
> >>>>   .../admin-guide/kernel-parameters.txt         |   4 +
> >>>>   drivers/acpi/Kconfig                          |   9 +
> >>>>   drivers/acpi/Makefile                         |   1 +
> >>>>   drivers/acpi/phat.c                           | 270 ++++++++++++++++++
> >>>>   include/acpi/actbl2.h                         |  18 ++
> >>>>   5 files changed, 302 insertions(+)
> >>>>   create mode 100644 drivers/acpi/phat.c
> >>>>
> >>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >>>> index 722b6eca2e93..33b932302ece 100644
> >>>> --- a/Documentation/admin-guide/kernel-parameters.txt
> >>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >>>> @@ -4490,6 +4490,10 @@
> >>>>                       allocator.  This parameter is primarily for debugging
> >>>>                       and performance comparison.
> >>>>
> >>>> +    phat_disable=   [ACPI]
> >>>> +                    Disable PHAT table parsing and logging of Firmware
> >>>> +                    Version and Health Data records.
> >>>> +
> >>>>       pirq=           [SMP,APIC] Manual mp-table setup
> >>>>                       See Documentation/arch/x86/i386/IO-APIC.rst.
> >>>>
> >>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >>>> index 00dd309b6682..06a7dd6e5a40 100644
> >>>> --- a/drivers/acpi/Kconfig
> >>>> +++ b/drivers/acpi/Kconfig
> >>>> @@ -96,6 +96,15 @@ config ACPI_FPDT
> >>>>         This table provides information on the timing of the system
> >>>>         boot, S3 suspend and S3 resume firmware code paths.
> >>>>
> >>>> +config ACPI_PHAT
> >>>> +    bool "ACPI Platform Health Assessment Table (PHAT) support"
> >>>> +    depends on X86_64 || ARM64
> >>>> +    help
> >>>> +      Enable support for Platform Health Assessment Table (PHAT).
> >>>> +      This table exposes an extensible set of platform health
> >>>> +      related telemetry through Firmware Version and Firmware Health
> >>>> +      Data Records.
> >>>> +
> >>>>   config ACPI_LPIT
> >>>>       bool
> >>>>       depends on X86_64
> >>>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >>>> index 3fc5a0d54f6e..93a4ec57ba6d 100644
> >>>> --- a/drivers/acpi/Makefile
> >>>> +++ b/drivers/acpi/Makefile
> >>>> @@ -69,6 +69,7 @@ acpi-$(CONFIG_ACPI_WATCHDOG)       += acpi_watchdog.o
> >>>>   acpi-$(CONFIG_ACPI_PRMT)    += prmt.o
> >>>>   acpi-$(CONFIG_ACPI_PCC)             += acpi_pcc.o
> >>>>   acpi-$(CONFIG_ACPI_FFH)             += acpi_ffh.o
> >>>> +acpi-$(CONFIG_ACPI_PHAT)    += phat.o
> >>>>
> >>>>   # Address translation
> >>>>   acpi-$(CONFIG_ACPI_ADXL)    += acpi_adxl.o
> >>>> diff --git a/drivers/acpi/phat.c b/drivers/acpi/phat.c
> >>>> new file mode 100644
> >>>> index 000000000000..6006dd7615fa
> >>>> --- /dev/null
> >>>> +++ b/drivers/acpi/phat.c
> >>>> @@ -0,0 +1,270 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0
> >>>> +/*
> >>>> + * Platform Health Assessment Table (PHAT) support
> >>>> + *
> >>>> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
> >>>> + *
> >>>> + * Author: Avadhut Naik <avadhut.naik@amd.com>
> >>>> + *
> >>>> + * This file implements parsing of the Platform Health Assessment Table
> >>>> + * through which a platform can expose an extensible set of platform
> >>>> + * health related telemetry. The telemetry is exposed through Firmware
> >>>> + * Version Data Records and Firmware Health Data Records. Additionally,
> >>>> + * a platform, through system firmware, also exposes Reset Reason Health
> >>>> + * Record to inform the operating system of the cause of last system
> >>>> + * reset or boot.
> >>>> + *
> >>>> + * For more information on PHAT, please refer to ACPI specification
> >>>> + * version 6.5, section 5.2.31
> >>>> + */
> >>>> +
> >>>> +#include <linux/acpi.h>
> >>>> +
> >>>> +static int phat_disable __initdata;
> >>>> +static const char *prefix = "ACPI PHAT: ";
> >>>
> >>> Wouldn't it be better if you used pr_fmt macro instead ?
> >>>
> >>          Have explained below.
> >>>> +
> >>>> +/* Reset Reason Health Record GUID */
> >>>> +static const guid_t reset_guid =
> >>>> +    GUID_INIT(0x7a014ce2, 0xf263, 0x4b77,
> >>>> +              0xb8, 0x8a, 0xe6, 0x33, 0x6b, 0x78, 0x2c, 0x14);
> >>>> +
> >>>> +static struct { u8 mask; const char *str; } const reset_sources[] = {
> >>>> +    {BIT(0), "Unknown source"},
> >>>> +    {BIT(1), "Hardware Source"},
> >>>> +    {BIT(2), "Firmware Source"},
> >>>> +    {BIT(3), "Software initiated reset"},
> >>>> +    {BIT(4), "Supervisor initiated reset"},
> >>>> +};
> >>>> +
> >>>> +static struct { u8 val; const char *str; } const reset_reasons[] = {
> >>>> +    {0, "UNKNOWN"},
> >>>> +    {1, "COLD BOOT"},
> >>>> +    {2, "COLD RESET"},
> >>>> +    {3, "WARM RESET"},
> >>>> +    {4, "UPDATE"},
> >>>> +    {32, "UNEXPECTED RESET"},
> >>>> +    {33, "FAULT"},
> >>>> +    {34, "TIMEOUT"},
> >>>> +    {35, "THERMAL"},
> >>>> +    {36, "POWER LOSS"},
> >>>> +    {37, "POWER BUTTON"},
> >>>> +};
> >>>> +
> >>>> +/*
> >>>> + * Print the last PHAT Version Element associated with a Firmware
> >>>> + * Version Data Record.
> >>>> + * Firmware Version Data Record consists of an array of PHAT Version
> >>>> + * Elements with each entry in the array representing a modification
> >>>> + * undertaken on a given platform component.
> >>>> + * In the event the array has multiple entries, minimize logs on the
> >>>> + * console and print only the last version element since it denotes
> >>>> + * the currently running instance of the component.
> >>>> + */
> >>>> +static int phat_version_data_parse(const char *pfx,
> >>>> +                               struct acpi_phat_version_data *version)
> >>>> +{
> >>>> +    char newpfx[64];
> >>>> +    u32 num_elems = version->element_count - 1;
> >>>> +    struct acpi_phat_version_element *element;
> >>>> +    int offset = sizeof(struct acpi_phat_version_data);
> >>>> +
> >>>> +    if (!version->element_count) {
> >>>> +            pr_info("%sNo PHAT Version Elements found.\n", prefix);
> >>>> +            return 0;
> >>>> +    }
> >>>> +
> >>>> +    offset += num_elems * sizeof(struct acpi_phat_version_element);
> >>>> +    element = (void *)version + offset;
> >>>> +
> >>>> +    pr_info("%sPHAT Version Element:\n", pfx);
> >>>> +    snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> >>>> +    pr_info("%sComponent ID: %pUl\n", newpfx, element->guid);
> >>>> +    pr_info("%sVersion: 0x%llx\n", newpfx, element->version_value);
> >>>> +    snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> >>>> +    print_hex_dump(newpfx, "Producer ID: ", DUMP_PREFIX_NONE, 16, 4,
> >>>> +                   &element->producer_id, sizeof(element->producer_id), true);
> >>>
> >>> I do have to admit that all this dancing with pfx and newpfx confuses me. Couldn't you
> >>> just use pr_fmt for everything printed using pr_* family of functions ? print_hex_dump()
> >>> is not impacted by pr_fmt, as it just uses printk to do it's printing.
> >>>
> >> I had considered using pr_fmt initially but since the ACPI spec says that PHAT health records,
> >> especially reset reason health record is intended to complement existing fault reporting
> >> mechanisms like BERT Tables, CPER, decided to have their outputs in identical formats, like has
> >> been implemented in cper_estatus_print().
> >>
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Print the Reset Reason Health Record
> >>>> + */
> >>>> +static int phat_reset_reason_parse(const char *pfx,
> >>>> +                               struct acpi_phat_health_data *record)
> >>>> +{
> >>>> +    int idx;
> >>>> +    void *data;
> >>>> +    u32 data_len;
> >>>> +    char newpfx[64];
> >>>> +    struct acpi_phat_reset_reason *rr;
> >>>> +    struct acpi_phat_vendor_reset_data *vdata;
> >>>> +
> >>>> +    rr = (void *)record + record->device_specific_offset;
> >>>> +
> >>>> +    for (idx = 0; idx < ARRAY_SIZE(reset_sources); idx++) {
> >>>> +            if (!rr->reset_source) {
> >>>> +                    pr_info("%sUnknown Reset Source.\n", pfx);
> >>>> +                    break;
> >>>> +            }
> >>>> +            if (rr->reset_source & reset_sources[idx].mask) {
> >>>> +                    pr_info("%sReset Source: 0x%x\t%s\n", pfx, reset_sources[idx].mask,
> >>>> +                            reset_sources[idx].str);
> >>>> +                    /* According to ACPI v6.5 Table 5.168, Sub-Source is
> >>>> +                     * defined only for Software initiated reset.
> >>>> +                     */
> >>>> +                    if (idx == 0x3 && rr->reset_sub_source)
> >>>> +                            pr_info("%sReset Sub-Source: %s\n", pfx,
> >>>> +                                    rr->reset_sub_source == 0x1 ?
> >>>> +                                    "Operating System" : "Hypervisor");
> >>>> +                    break;
> >>>> +            }
> >>>> +    }
> >>>> +
> >>>> +    for (idx = 0; idx < ARRAY_SIZE(reset_reasons); idx++) {
> >>>> +            if (rr->reset_reason == reset_reasons[idx].val) {
> >>>> +                    pr_info("%sReset Reason: 0x%x\t%s\n", pfx, reset_reasons[idx].val,
> >>>> +                            reset_reasons[idx].str);
> >>>> +                    break;
> >>>> +            }
> >>>> +    }
> >>>> +
> >>>> +    if (!rr->vendor_count)
> >>>> +            return 0;
> >>>> +
> >>>> +    pr_info("%sReset Reason Vendor Data:\n", pfx);
> >>>> +    vdata = (void *)rr + sizeof(*rr);
> >>>> +
> >>>> +    for (idx = 0; idx < rr->vendor_count; idx++) {
> >>>> +            snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> >>>> +            data_len = vdata->length - sizeof(*vdata);
> >>>> +            data = (void *)vdata + sizeof(*vdata);
> >>>> +            pr_info("%sVendor Data ID: %pUl\n", newpfx, vdata->vendor_id);
> >>>> +            pr_info("%sRevision: 0x%x\n", newpfx, vdata->revision);
> >>>> +            snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> >>>> +            print_hex_dump(newpfx, "Data: ", DUMP_PREFIX_NONE, 16, 4,
> >>>> +                           data, data_len, false);
> >>>> +            vdata = (void *)vdata + vdata->length;
> >>>> +    }
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Print the Firmware Health Data Record.
> >>>> + */
> >>>> +static int phat_health_data_parse(const char *pfx,
> >>>> +                              struct acpi_phat_health_data *record)
> >>>> +{
> >>>> +    void *data;
> >>>> +    u32 data_len;
> >>>> +    char newpfx[64];
> >>>> +
> >>>> +    pr_info("%sHealth Records.\n", pfx);
> >>>> +    snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> >>>> +    pr_info("%sDevice Signature: %pUl\n", newpfx, record->device_guid);
> >>>> +
> >>>> +    switch (record->health) {
> >>>> +    case ACPI_PHAT_ERRORS_FOUND:
> >>>> +            pr_info("%sAmHealthy: Errors found\n", newpfx);
> >>>> +            break;
> >>>> +    case ACPI_PHAT_NO_ERRORS:
> >>>> +            pr_info("%sAmHealthy: No errors found.\n", newpfx);
> >>>> +            break;
> >>>> +    case ACPI_PHAT_UNKNOWN_ERRORS:
> >>>> +            pr_info("%sAmHealthy: Unknown.\n", newpfx);
> >>>> +            break;
> >>>> +    case ACPI_PHAT_ADVISORY:
> >>>> +            pr_info("%sAmHealthy: Advisory – additional device-specific data exposed.\n",
> >>>> +                    newpfx);
> >>>> +            break;
> >>>> +    default:
> >>>> +            break;
> >>>> +    }
> >>>> +
> >>>> +    if (!record->device_specific_offset)
> >>>> +            return 0;
> >>>> +
> >>>> +    /* Reset Reason Health Record has a unique GUID and is created as
> >>>> +     * a Health Record in the PHAT table. Check if this Health Record
> >>>> +     * is a Reset Reason Health Record.
> >>>> +     */
> >>>> +    if (guid_equal((guid_t *)record->device_guid, &reset_guid)) {
> >>>> +            phat_reset_reason_parse(newpfx, record);
> >>>> +            return 0;
> >>>> +    }
> >>>> +
> >>>> +    data = (void *)record + record->device_specific_offset;
> >>>> +    data_len = record->header.length - record->device_specific_offset;
> >>>> +    snprintf(newpfx, sizeof(newpfx), KERN_INFO "%s ", pfx);
> >>>> +    print_hex_dump(newpfx, "Device Data: ", DUMP_PREFIX_NONE, 16, 4,
> >>>> +                   data, data_len, false);
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +static int parse_phat_table(const char *pfx, struct acpi_table_phat *phat_tab)
> >>>> +{
> >>>> +    char newpfx[64];
> >>>> +    u32 offset = sizeof(*phat_tab);
> >>>> +    struct acpi_phat_header *phat_header;
> >>>> +
> >>>> +    snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> >>>> +
> >>>> +    while (offset < phat_tab->header.length) {
> >>>> +            phat_header = (void *)phat_tab + offset;
> >>>> +            switch (phat_header->type) {
> >>>> +            case ACPI_PHAT_TYPE_FW_VERSION_DATA:
> >>>> +                    phat_version_data_parse(newpfx, (struct acpi_phat_version_data *)
> >>>> +                        phat_header);
> >>>> +                    break;
> >>>> +            case ACPI_PHAT_TYPE_FW_HEALTH_DATA:
> >>>> +                    phat_health_data_parse(newpfx, (struct acpi_phat_health_data *)
> >>>> +                        phat_header);
> >>>> +                    break;
> >>>> +            default:
> >>>> +                    break;
> >>>> +            }
> >>>> +            offset += phat_header->length;
> >>>> +    }
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +static int __init setup_phat_disable(char *str)
> >>>> +{
> >>>> +    phat_disable = 1;
> >>>> +    return 1;
> >>>> +}
> >>>> +__setup("phat_disable", setup_phat_disable);
> >>>> +
> >>>> +static int __init acpi_phat_init(void)
> >>>> +{
> >>>> +    acpi_status status;
> >>>> +    struct acpi_table_phat *phat_tab;
> >>>> +
> >>>> +    if (acpi_disabled)
> >>>> +            return 0;
> >>>> +
> >>>> +    if (phat_disable) {
> >>>> +            pr_err("%sPHAT support has been disabled.\n", prefix);
> >>>> +            return 0;
> >>>> +    }
> >>>> +
> >>>> +    status = acpi_get_table(ACPI_SIG_PHAT, 0,
> >>>> +                            (struct acpi_table_header **)&phat_tab);
> >>>> +
> >>>> +    if (status == AE_NOT_FOUND) {
> >>>> +            pr_info("%sPHAT Table not found.\n", prefix);
> >>>> +            return 0;
> >>>> +    } else if (ACPI_FAILURE(status)) {
> >>>> +            pr_err("%sFailed to get PHAT Table: %s.\n", prefix,
> >>>> +                   acpi_format_exception(status));
> >>>> +            return -EINVAL;
> >>>> +    }
> >>>> +
> >>>> +    pr_info("%sPlatform Telemetry Records.\n", prefix);
> >>>> +    parse_phat_table(prefix, phat_tab);
> >>>
> >>> So for now you're only dumping tables to the dmesg output ?
> >>> Are you planning to create some sysfs interfaces similar to let's
> >>> say EINJ ?
> >>>
> >> Yes, for now, the output is being posted to dmesg only.
> >
> > So it is not particularly useful for anything practical.
> >
> >> If there is a consensus, we can have the information exported through
> >> sysfs too.
> >
> > That depends on how you want to use it which should be explained in
> > the patch changelog.
> >
> > Now it's basically "it's there, so dump it" which leads to the obvious
> > question: "Who's going to need it?"
> >
>
> I was just talking to some colleagues about PHAT recently as well.
>
> The use case that jumps out is "system randomly rebooted while I was
> doing XYZ".  You don't know what happened, but you keep using your
> system.  Then it happens again.
>
> If the reason for the random reboot is captured to dmesg you can cross
> reference your journal from the next boot after any random reboot and
> get the reason for it.  If a user reports this to a Gitlab issue tracker
> or Bugzilla it can be helpful in establishing a pattern.
>
> >> The below location may be appropriate in that case:
> >> /sys/firmware/acpi/
> >
> > Yes, it may. >
> >> We already have FPDT and BGRT being exported from there.
> >
> > In fact, all of the ACPI tables can be retrieved verbatim from
> > /sys/firmware/acpi/tables/ already, so why exactly do you want the
> > kernel to parse PHAT in particular?
> >
>
> It's not to say that /sys/firmware/acpi/PHAT isn't useful, but having
> something internal to the kernel "automatically" parsing it and saving
> information to a place like the kernel log that is already captured by
> existing userspace tools I think is "more" useful.

What existing user space tools do you mean?  Is there anything already
making use of the kernel's PHAT output?

And why can't user space simply parse PHAT by itself?

There are multiple ACPI tables that could be dumped into the kernel
log, but they aren't.  Guess why.

On Mon, Aug 21, 2023 at 7:17 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> On 8/21/2023 12:12 PM, Rafael J. Wysocki wrote:
> <snip>
> >> I was just talking to some colleagues about PHAT recently as well.
> >>
> >> The use case that jumps out is "system randomly rebooted while I was
> >> doing XYZ".  You don't know what happened, but you keep using your
> >> system.  Then it happens again.
> >>
> >> If the reason for the random reboot is captured to dmesg you can cross
> >> reference your journal from the next boot after any random reboot and
> >> get the reason for it.  If a user reports this to a Gitlab issue tracker
> >> or Bugzilla it can be helpful in establishing a pattern.
> >>
> >>>> The below location may be appropriate in that case:
> >>>> /sys/firmware/acpi/
> >>>
> >>> Yes, it may. >
> >>>> We already have FPDT and BGRT being exported from there.
> >>>
> >>> In fact, all of the ACPI tables can be retrieved verbatim from
> >>> /sys/firmware/acpi/tables/ already, so why exactly do you want the
> >>> kernel to parse PHAT in particular?
> >>>
> >>
> >> It's not to say that /sys/firmware/acpi/PHAT isn't useful, but having
> >> something internal to the kernel "automatically" parsing it and saving
> >> information to a place like the kernel log that is already captured by
> >> existing userspace tools I think is "more" useful.
> >
> > What existing user space tools do you mean?  Is there anything already
> > making use of the kernel's PHAT output?
> >
>
> I was meaning things like systemd already capture the kernel long
> ringbuffer.  If you save stuff like this into the kernel log, it's going
> to be indexed and easier to grep for boots that had it.
>
> > And why can't user space simply parse PHAT by itself?
> >  > There are multiple ACPI tables that could be dumped into the kernel
> > log, but they aren't.  Guess why.
>
> Right; there's not reason it can't be done by userspace directly.
>
> Another way to approach this problem could be to modify tools that
> excavate records from a reboot to also get PHAT.  For example
> systemd-pstore will get any kernel panics from the previous boot from
> the EFI pstore and put them into /var/lib/systemd/pstore.
>
> No reason that couldn't be done automatically for PHAT too.

I'm not sure about the connection between the PHAT dump in the kernel
log and pstore.

The PHAT dump would be from the time before the failure, so it is
unclear to me how useful it can be for diagnosing it.  However, after
a reboot one should be able to retrieve PHAT data from the table
directly and that may include some information regarding the failure.

With pstore, the assumption is that there will be some information
relevant for diagnosing the failure in the kernel buffer, but I'm not
sure how the PHAT dump from before the failure can help here?

On Mon, Aug 21, 2023 at 7:35 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
>
>
> On 8/21/2023 12:29 PM, Rafael J. Wysocki wrote:
> > On Mon, Aug 21, 2023 at 7:17 PM Limonciello, Mario
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 8/21/2023 12:12 PM, Rafael J. Wysocki wrote:
> >> <snip>
> >>>> I was just talking to some colleagues about PHAT recently as well.
> >>>>
> >>>> The use case that jumps out is "system randomly rebooted while I was
> >>>> doing XYZ".  You don't know what happened, but you keep using your
> >>>> system.  Then it happens again.
> >>>>
> >>>> If the reason for the random reboot is captured to dmesg you can cross
> >>>> reference your journal from the next boot after any random reboot and
> >>>> get the reason for it.  If a user reports this to a Gitlab issue tracker
> >>>> or Bugzilla it can be helpful in establishing a pattern.
> >>>>
> >>>>>> The below location may be appropriate in that case:
> >>>>>> /sys/firmware/acpi/
> >>>>>
> >>>>> Yes, it may. >
> >>>>>> We already have FPDT and BGRT being exported from there.
> >>>>>
> >>>>> In fact, all of the ACPI tables can be retrieved verbatim from
> >>>>> /sys/firmware/acpi/tables/ already, so why exactly do you want the
> >>>>> kernel to parse PHAT in particular?
> >>>>>
> >>>>
> >>>> It's not to say that /sys/firmware/acpi/PHAT isn't useful, but having
> >>>> something internal to the kernel "automatically" parsing it and saving
> >>>> information to a place like the kernel log that is already captured by
> >>>> existing userspace tools I think is "more" useful.
> >>>
> >>> What existing user space tools do you mean?  Is there anything already
> >>> making use of the kernel's PHAT output?
> >>>
> >>
> >> I was meaning things like systemd already capture the kernel long
> >> ringbuffer.  If you save stuff like this into the kernel log, it's going
> >> to be indexed and easier to grep for boots that had it.
> >>
> >>> And why can't user space simply parse PHAT by itself?
> >>>   > There are multiple ACPI tables that could be dumped into the kernel
> >>> log, but they aren't.  Guess why.
> >>
> >> Right; there's not reason it can't be done by userspace directly.
> >>
> >> Another way to approach this problem could be to modify tools that
> >> excavate records from a reboot to also get PHAT.  For example
> >> systemd-pstore will get any kernel panics from the previous boot from
> >> the EFI pstore and put them into /var/lib/systemd/pstore.
> >>
> >> No reason that couldn't be done automatically for PHAT too.
> >
> > I'm not sure about the connection between the PHAT dump in the kernel
> > log and pstore.
> >
> > The PHAT dump would be from the time before the failure, so it is
> > unclear to me how useful it can be for diagnosing it.  However, after
> > a reboot one should be able to retrieve PHAT data from the table
> > directly and that may include some information regarding the failure.
>
> Right so the thought is that at bootup you get the last entry from PHAT
> and save that into the log.
>
> Let's say you have 3 boots:
> X - Triggered a random reboot
> Y - Cleanly shut down
> Z - Boot after a clean shut down
>
> So on boot Y you would have in your logs the reason that boot X rebooted.

Yes, and the same can be retrieved from the PHAT directly from user
space at that time, can't it?

> On boot Z you would see something about how boot Y's reason.
>
> >
> > With pstore, the assumption is that there will be some information
> > relevant for diagnosing the failure in the kernel buffer, but I'm not
> > sure how the PHAT dump from before the failure can help here?
>
> Alone it's not useful.
> I had figured if you can put it together with other data it's useful.
> For example if you had some thermal data in the logs showing which
> component overheated or if you looked at pstore and found a NULL pointer
> dereference.

IIUC, the current PHAT content can be useful.  The PHAT content from
boot X (before the failure) which is what will be there in pstore
after the random reboot, is of limited value AFAICS.

On Mon, Aug 21, 2023 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Aug 21, 2023 at 7:35 PM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
> >
> >
> >
> > On 8/21/2023 12:29 PM, Rafael J. Wysocki wrote:
> > > On Mon, Aug 21, 2023 at 7:17 PM Limonciello, Mario
> > > <mario.limonciello@amd.com> wrote:
> > >>
> > >> On 8/21/2023 12:12 PM, Rafael J. Wysocki wrote:
> > >> <snip>
> > >>>> I was just talking to some colleagues about PHAT recently as well.
> > >>>>
> > >>>> The use case that jumps out is "system randomly rebooted while I was
> > >>>> doing XYZ".  You don't know what happened, but you keep using your
> > >>>> system.  Then it happens again.
> > >>>>
> > >>>> If the reason for the random reboot is captured to dmesg you can cross
> > >>>> reference your journal from the next boot after any random reboot and
> > >>>> get the reason for it.  If a user reports this to a Gitlab issue tracker
> > >>>> or Bugzilla it can be helpful in establishing a pattern.
> > >>>>
> > >>>>>> The below location may be appropriate in that case:
> > >>>>>> /sys/firmware/acpi/
> > >>>>>
> > >>>>> Yes, it may. >
> > >>>>>> We already have FPDT and BGRT being exported from there.
> > >>>>>
> > >>>>> In fact, all of the ACPI tables can be retrieved verbatim from
> > >>>>> /sys/firmware/acpi/tables/ already, so why exactly do you want the
> > >>>>> kernel to parse PHAT in particular?
> > >>>>>
> > >>>>
> > >>>> It's not to say that /sys/firmware/acpi/PHAT isn't useful, but having
> > >>>> something internal to the kernel "automatically" parsing it and saving
> > >>>> information to a place like the kernel log that is already captured by
> > >>>> existing userspace tools I think is "more" useful.
> > >>>
> > >>> What existing user space tools do you mean?  Is there anything already
> > >>> making use of the kernel's PHAT output?
> > >>>
> > >>
> > >> I was meaning things like systemd already capture the kernel long
> > >> ringbuffer.  If you save stuff like this into the kernel log, it's going
> > >> to be indexed and easier to grep for boots that had it.
> > >>
> > >>> And why can't user space simply parse PHAT by itself?
> > >>>   > There are multiple ACPI tables that could be dumped into the kernel
> > >>> log, but they aren't.  Guess why.
> > >>
> > >> Right; there's not reason it can't be done by userspace directly.
> > >>
> > >> Another way to approach this problem could be to modify tools that
> > >> excavate records from a reboot to also get PHAT.  For example
> > >> systemd-pstore will get any kernel panics from the previous boot from
> > >> the EFI pstore and put them into /var/lib/systemd/pstore.
> > >>
> > >> No reason that couldn't be done automatically for PHAT too.
> > >
> > > I'm not sure about the connection between the PHAT dump in the kernel
> > > log and pstore.
> > >
> > > The PHAT dump would be from the time before the failure, so it is
> > > unclear to me how useful it can be for diagnosing it.  However, after
> > > a reboot one should be able to retrieve PHAT data from the table
> > > directly and that may include some information regarding the failure.
> >
> > Right so the thought is that at bootup you get the last entry from PHAT
> > and save that into the log.
> >
> > Let's say you have 3 boots:
> > X - Triggered a random reboot
> > Y - Cleanly shut down
> > Z - Boot after a clean shut down
> >
> > So on boot Y you would have in your logs the reason that boot X rebooted.
>
> Yes, and the same can be retrieved from the PHAT directly from user
> space at that time, can't it?
>
> > On boot Z you would see something about how boot Y's reason.
> >
> > >
> > > With pstore, the assumption is that there will be some information
> > > relevant for diagnosing the failure in the kernel buffer, but I'm not
> > > sure how the PHAT dump from before the failure can help here?
> >
> > Alone it's not useful.
> > I had figured if you can put it together with other data it's useful.
> > For example if you had some thermal data in the logs showing which
> > component overheated or if you looked at pstore and found a NULL pointer
> > dereference.
>
> IIUC, the current PHAT content can be useful.  The PHAT content from
> boot X (before the failure) which is what will be there in pstore
> after the random reboot, is of limited value AFAICS.

To be more precise, I don't see why the kernel needs to be made a
man-in-the-middle between the firmware which is the source of the
information and user space that consumes it.

On 8/21/23 2:01 PM, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2023 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Aug 21, 2023 at 7:35 PM Limonciello, Mario
>> <mario.limonciello@amd.com> wrote:
>>>
>>>
>>>
>>> On 8/21/2023 12:29 PM, Rafael J. Wysocki wrote:
>>>> On Mon, Aug 21, 2023 at 7:17 PM Limonciello, Mario
>>>> <mario.limonciello@amd.com> wrote:
>>>>>
>>>>> On 8/21/2023 12:12 PM, Rafael J. Wysocki wrote:
>>>>> <snip>
>>>>>>> I was just talking to some colleagues about PHAT recently as well.
>>>>>>>
>>>>>>> The use case that jumps out is "system randomly rebooted while I was
>>>>>>> doing XYZ".  You don't know what happened, but you keep using your
>>>>>>> system.  Then it happens again.
>>>>>>>
>>>>>>> If the reason for the random reboot is captured to dmesg you can cross
>>>>>>> reference your journal from the next boot after any random reboot and
>>>>>>> get the reason for it.  If a user reports this to a Gitlab issue tracker
>>>>>>> or Bugzilla it can be helpful in establishing a pattern.
>>>>>>>
>>>>>>>>> The below location may be appropriate in that case:
>>>>>>>>> /sys/firmware/acpi/
>>>>>>>>
>>>>>>>> Yes, it may. >
>>>>>>>>> We already have FPDT and BGRT being exported from there.
>>>>>>>>
>>>>>>>> In fact, all of the ACPI tables can be retrieved verbatim from
>>>>>>>> /sys/firmware/acpi/tables/ already, so why exactly do you want the
>>>>>>>> kernel to parse PHAT in particular?
>>>>>>>>
>>>>>>>
>>>>>>> It's not to say that /sys/firmware/acpi/PHAT isn't useful, but having
>>>>>>> something internal to the kernel "automatically" parsing it and saving
>>>>>>> information to a place like the kernel log that is already captured by
>>>>>>> existing userspace tools I think is "more" useful.
>>>>>>
>>>>>> What existing user space tools do you mean?  Is there anything already
>>>>>> making use of the kernel's PHAT output?
>>>>>>
>>>>>
>>>>> I was meaning things like systemd already capture the kernel long
>>>>> ringbuffer.  If you save stuff like this into the kernel log, it's going
>>>>> to be indexed and easier to grep for boots that had it.
>>>>>
>>>>>> And why can't user space simply parse PHAT by itself?
>>>>>>   > There are multiple ACPI tables that could be dumped into the kernel
>>>>>> log, but they aren't.  Guess why.
>>>>>
>>>>> Right; there's not reason it can't be done by userspace directly.
>>>>>
>>>>> Another way to approach this problem could be to modify tools that
>>>>> excavate records from a reboot to also get PHAT.  For example
>>>>> systemd-pstore will get any kernel panics from the previous boot from
>>>>> the EFI pstore and put them into /var/lib/systemd/pstore.
>>>>>
>>>>> No reason that couldn't be done automatically for PHAT too.
>>>>
>>>> I'm not sure about the connection between the PHAT dump in the kernel
>>>> log and pstore.
>>>>
>>>> The PHAT dump would be from the time before the failure, so it is
>>>> unclear to me how useful it can be for diagnosing it.  However, after
>>>> a reboot one should be able to retrieve PHAT data from the table
>>>> directly and that may include some information regarding the failure.
>>>
>>> Right so the thought is that at bootup you get the last entry from PHAT
>>> and save that into the log.
>>>
>>> Let's say you have 3 boots:
>>> X - Triggered a random reboot
>>> Y - Cleanly shut down
>>> Z - Boot after a clean shut down
>>>
>>> So on boot Y you would have in your logs the reason that boot X rebooted.
>>
>> Yes, and the same can be retrieved from the PHAT directly from user
>> space at that time, can't it?
>>
>>> On boot Z you would see something about how boot Y's reason.
>>>
>>>>
>>>> With pstore, the assumption is that there will be some information
>>>> relevant for diagnosing the failure in the kernel buffer, but I'm not
>>>> sure how the PHAT dump from before the failure can help here?
>>>
>>> Alone it's not useful.
>>> I had figured if you can put it together with other data it's useful.
>>> For example if you had some thermal data in the logs showing which
>>> component overheated or if you looked at pstore and found a NULL pointer
>>> dereference.
>>
>> IIUC, the current PHAT content can be useful.  The PHAT content from
>> boot X (before the failure) which is what will be there in pstore
>> after the random reboot, is of limited value AFAICS.
> 
> To be more precise, I don't see why the kernel needs to be made a
> man-in-the-middle between the firmware which is the source of the
> information and user space that consumes it.

I think that's a fair point.

Is there a preferred set of tools that can be updated?

If not, would it make sense to develop a set of common kernel tools for
this?

In my experience, it seems many folks use tools from their vendors or
custom tools.

Thanks,
Yazen

On 8/21/23 2:49 PM, Rafael J. Wysocki wrote:
> On Mon, Aug 21, 2023 at 8:44 PM Yazen Ghannam <yazen.ghannam@amd.com> wrote:
>>
>> On 8/21/23 2:01 PM, Rafael J. Wysocki wrote:
>>> On Mon, Aug 21, 2023 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>>
>>>> On Mon, Aug 21, 2023 at 7:35 PM Limonciello, Mario
>>>> <mario.limonciello@amd.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 8/21/2023 12:29 PM, Rafael J. Wysocki wrote:
>>>>>> On Mon, Aug 21, 2023 at 7:17 PM Limonciello, Mario
>>>>>> <mario.limonciello@amd.com> wrote:
>>>>>>>
>>>>>>> On 8/21/2023 12:12 PM, Rafael J. Wysocki wrote:
>>>>>>> <snip>
>>>>>>>>> I was just talking to some colleagues about PHAT recently as well.
>>>>>>>>>
>>>>>>>>> The use case that jumps out is "system randomly rebooted while I was
>>>>>>>>> doing XYZ".  You don't know what happened, but you keep using your
>>>>>>>>> system.  Then it happens again.
>>>>>>>>>
>>>>>>>>> If the reason for the random reboot is captured to dmesg you can cross
>>>>>>>>> reference your journal from the next boot after any random reboot and
>>>>>>>>> get the reason for it.  If a user reports this to a Gitlab issue tracker
>>>>>>>>> or Bugzilla it can be helpful in establishing a pattern.
>>>>>>>>>
>>>>>>>>>>> The below location may be appropriate in that case:
>>>>>>>>>>> /sys/firmware/acpi/
>>>>>>>>>>
>>>>>>>>>> Yes, it may. >
>>>>>>>>>>> We already have FPDT and BGRT being exported from there.
>>>>>>>>>>
>>>>>>>>>> In fact, all of the ACPI tables can be retrieved verbatim from
>>>>>>>>>> /sys/firmware/acpi/tables/ already, so why exactly do you want the
>>>>>>>>>> kernel to parse PHAT in particular?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It's not to say that /sys/firmware/acpi/PHAT isn't useful, but having
>>>>>>>>> something internal to the kernel "automatically" parsing it and saving
>>>>>>>>> information to a place like the kernel log that is already captured by
>>>>>>>>> existing userspace tools I think is "more" useful.
>>>>>>>>
>>>>>>>> What existing user space tools do you mean?  Is there anything already
>>>>>>>> making use of the kernel's PHAT output?
>>>>>>>>
>>>>>>>
>>>>>>> I was meaning things like systemd already capture the kernel long
>>>>>>> ringbuffer.  If you save stuff like this into the kernel log, it's going
>>>>>>> to be indexed and easier to grep for boots that had it.
>>>>>>>
>>>>>>>> And why can't user space simply parse PHAT by itself?
>>>>>>>>   > There are multiple ACPI tables that could be dumped into the kernel
>>>>>>>> log, but they aren't.  Guess why.
>>>>>>>
>>>>>>> Right; there's not reason it can't be done by userspace directly.
>>>>>>>
>>>>>>> Another way to approach this problem could be to modify tools that
>>>>>>> excavate records from a reboot to also get PHAT.  For example
>>>>>>> systemd-pstore will get any kernel panics from the previous boot from
>>>>>>> the EFI pstore and put them into /var/lib/systemd/pstore.
>>>>>>>
>>>>>>> No reason that couldn't be done automatically for PHAT too.
>>>>>>
>>>>>> I'm not sure about the connection between the PHAT dump in the kernel
>>>>>> log and pstore.
>>>>>>
>>>>>> The PHAT dump would be from the time before the failure, so it is
>>>>>> unclear to me how useful it can be for diagnosing it.  However, after
>>>>>> a reboot one should be able to retrieve PHAT data from the table
>>>>>> directly and that may include some information regarding the failure.
>>>>>
>>>>> Right so the thought is that at bootup you get the last entry from PHAT
>>>>> and save that into the log.
>>>>>
>>>>> Let's say you have 3 boots:
>>>>> X - Triggered a random reboot
>>>>> Y - Cleanly shut down
>>>>> Z - Boot after a clean shut down
>>>>>
>>>>> So on boot Y you would have in your logs the reason that boot X rebooted.
>>>>
>>>> Yes, and the same can be retrieved from the PHAT directly from user
>>>> space at that time, can't it?
>>>>
>>>>> On boot Z you would see something about how boot Y's reason.
>>>>>
>>>>>>
>>>>>> With pstore, the assumption is that there will be some information
>>>>>> relevant for diagnosing the failure in the kernel buffer, but I'm not
>>>>>> sure how the PHAT dump from before the failure can help here?
>>>>>
>>>>> Alone it's not useful.
>>>>> I had figured if you can put it together with other data it's useful.
>>>>> For example if you had some thermal data in the logs showing which
>>>>> component overheated or if you looked at pstore and found a NULL pointer
>>>>> dereference.
>>>>
>>>> IIUC, the current PHAT content can be useful.  The PHAT content from
>>>> boot X (before the failure) which is what will be there in pstore
>>>> after the random reboot, is of limited value AFAICS.
>>>
>>> To be more precise, I don't see why the kernel needs to be made a
>>> man-in-the-middle between the firmware which is the source of the
>>> information and user space that consumes it.
>>
>> I think that's a fair point.
>>
>> Is there a preferred set of tools that can be updated?
> 
> I think you need to talk to distro people about this.
> 
>> If not, would it make sense to develop a set of common kernel tools for
>> this?
> 
> Yes, it would, but please see above in the first place.
> 
>> In my experience, it seems many folks use tools from their vendors or
>> custom tools.
> 
> This observation matches my own experience.

For the sake of discussion, and from a kernel developer's point of view,
should the tools be part of a separate project? Or should the tools be
part of the kernel tree like perf, etc.? Assuming that this needs to
start from scratch and not extending an existing project.

Thanks,
Yazen

On Mon, Aug 21, 2023 at 9:02 PM Yazen Ghannam <yazen.ghannam@amd.com> wrote:
>
> On 8/21/23 2:49 PM, Rafael J. Wysocki wrote:
> > On Mon, Aug 21, 2023 at 8:44 PM Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> >>
> >> On 8/21/23 2:01 PM, Rafael J. Wysocki wrote:
> >>> On Mon, Aug 21, 2023 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>>>
> >>>> On Mon, Aug 21, 2023 at 7:35 PM Limonciello, Mario
> >>>> <mario.limonciello@amd.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 8/21/2023 12:29 PM, Rafael J. Wysocki wrote:
> >>>>>> On Mon, Aug 21, 2023 at 7:17 PM Limonciello, Mario
> >>>>>> <mario.limonciello@amd.com> wrote:
> >>>>>>>
> >>>>>>> On 8/21/2023 12:12 PM, Rafael J. Wysocki wrote:
> >>>>>>> <snip>
> >>>>>>>>> I was just talking to some colleagues about PHAT recently as well.
> >>>>>>>>>
> >>>>>>>>> The use case that jumps out is "system randomly rebooted while I was
> >>>>>>>>> doing XYZ".  You don't know what happened, but you keep using your
> >>>>>>>>> system.  Then it happens again.
> >>>>>>>>>
> >>>>>>>>> If the reason for the random reboot is captured to dmesg you can cross
> >>>>>>>>> reference your journal from the next boot after any random reboot and
> >>>>>>>>> get the reason for it.  If a user reports this to a Gitlab issue tracker
> >>>>>>>>> or Bugzilla it can be helpful in establishing a pattern.
> >>>>>>>>>
> >>>>>>>>>>> The below location may be appropriate in that case:
> >>>>>>>>>>> /sys/firmware/acpi/
> >>>>>>>>>>
> >>>>>>>>>> Yes, it may. >
> >>>>>>>>>>> We already have FPDT and BGRT being exported from there.
> >>>>>>>>>>
> >>>>>>>>>> In fact, all of the ACPI tables can be retrieved verbatim from
> >>>>>>>>>> /sys/firmware/acpi/tables/ already, so why exactly do you want the
> >>>>>>>>>> kernel to parse PHAT in particular?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> It's not to say that /sys/firmware/acpi/PHAT isn't useful, but having
> >>>>>>>>> something internal to the kernel "automatically" parsing it and saving
> >>>>>>>>> information to a place like the kernel log that is already captured by
> >>>>>>>>> existing userspace tools I think is "more" useful.
> >>>>>>>>
> >>>>>>>> What existing user space tools do you mean?  Is there anything already
> >>>>>>>> making use of the kernel's PHAT output?
> >>>>>>>>
> >>>>>>>
> >>>>>>> I was meaning things like systemd already capture the kernel long
> >>>>>>> ringbuffer.  If you save stuff like this into the kernel log, it's going
> >>>>>>> to be indexed and easier to grep for boots that had it.
> >>>>>>>
> >>>>>>>> And why can't user space simply parse PHAT by itself?
> >>>>>>>>   > There are multiple ACPI tables that could be dumped into the kernel
> >>>>>>>> log, but they aren't.  Guess why.
> >>>>>>>
> >>>>>>> Right; there's not reason it can't be done by userspace directly.
> >>>>>>>
> >>>>>>> Another way to approach this problem could be to modify tools that
> >>>>>>> excavate records from a reboot to also get PHAT.  For example
> >>>>>>> systemd-pstore will get any kernel panics from the previous boot from
> >>>>>>> the EFI pstore and put them into /var/lib/systemd/pstore.
> >>>>>>>
> >>>>>>> No reason that couldn't be done automatically for PHAT too.
> >>>>>>
> >>>>>> I'm not sure about the connection between the PHAT dump in the kernel
> >>>>>> log and pstore.
> >>>>>>
> >>>>>> The PHAT dump would be from the time before the failure, so it is
> >>>>>> unclear to me how useful it can be for diagnosing it.  However, after
> >>>>>> a reboot one should be able to retrieve PHAT data from the table
> >>>>>> directly and that may include some information regarding the failure.
> >>>>>
> >>>>> Right so the thought is that at bootup you get the last entry from PHAT
> >>>>> and save that into the log.
> >>>>>
> >>>>> Let's say you have 3 boots:
> >>>>> X - Triggered a random reboot
> >>>>> Y - Cleanly shut down
> >>>>> Z - Boot after a clean shut down
> >>>>>
> >>>>> So on boot Y you would have in your logs the reason that boot X rebooted.
> >>>>
> >>>> Yes, and the same can be retrieved from the PHAT directly from user
> >>>> space at that time, can't it?
> >>>>
> >>>>> On boot Z you would see something about how boot Y's reason.
> >>>>>
> >>>>>>
> >>>>>> With pstore, the assumption is that there will be some information
> >>>>>> relevant for diagnosing the failure in the kernel buffer, but I'm not
> >>>>>> sure how the PHAT dump from before the failure can help here?
> >>>>>
> >>>>> Alone it's not useful.
> >>>>> I had figured if you can put it together with other data it's useful.
> >>>>> For example if you had some thermal data in the logs showing which
> >>>>> component overheated or if you looked at pstore and found a NULL pointer
> >>>>> dereference.
> >>>>
> >>>> IIUC, the current PHAT content can be useful.  The PHAT content from
> >>>> boot X (before the failure) which is what will be there in pstore
> >>>> after the random reboot, is of limited value AFAICS.
> >>>
> >>> To be more precise, I don't see why the kernel needs to be made a
> >>> man-in-the-middle between the firmware which is the source of the
> >>> information and user space that consumes it.
> >>
> >> I think that's a fair point.
> >>
> >> Is there a preferred set of tools that can be updated?
> >
> > I think you need to talk to distro people about this.
> >
> >> If not, would it make sense to develop a set of common kernel tools for
> >> this?
> >
> > Yes, it would, but please see above in the first place.
> >
> >> In my experience, it seems many folks use tools from their vendors or
> >> custom tools.
> >
> > This observation matches my own experience.
>
> For the sake of discussion, and from a kernel developer's point of view,
> should the tools be part of a separate project? Or should the tools be
> part of the kernel tree like perf, etc.? Assuming that this needs to
> start from scratch and not extending an existing project.

It can be both in principle, but from the practical standpoint it is
more likely to get all of the people to use the same set of tools if
they are included into the kernel source tree.

On 8/21/2023 2:16 PM, Rafael J. Wysocki wrote:
<snip>
>>>> Is there a preferred set of tools that can be updated?
>>>
>>> I think you need to talk to distro people about this.
>>>
>>>> If not, would it make sense to develop a set of common kernel tools for
>>>> this?
>>>
>>> Yes, it would, but please see above in the first place.
>>>
>>>> In my experience, it seems many folks use tools from their vendors or
>>>> custom tools.
>>>
>>> This observation matches my own experience.
>>
>> For the sake of discussion, and from a kernel developer's point of view,
>> should the tools be part of a separate project? Or should the tools be
>> part of the kernel tree like perf, etc.? Assuming that this needs to
>> start from scratch and not extending an existing project.
> 
> It can be both in principle, but from the practical standpoint it is
> more likely to get all of the people to use the same set of tools if
> they are included into the kernel source tree.

Yazen,

You generally envision tools like this to only be used when there is a 
problem, and not something that's run critical path on every boot right?

If so, how about doing it in a high level language with easily 
importable libraries like Python?

Then the tools can still be stored "in kernel tree" and distributed with 
distro "kernel tools" packages but you can more easily use them on 
random old kernels too since the binary via /sys/firmware/acpi/tables 
should be widely available.

On 8/21/23 3:23 PM, Limonciello, Mario wrote:
> 
> 
> On 8/21/2023 2:16 PM, Rafael J. Wysocki wrote:
> <snip>
>>>>> Is there a preferred set of tools that can be updated?
>>>>
>>>> I think you need to talk to distro people about this.
>>>>
>>>>> If not, would it make sense to develop a set of common kernel tools for
>>>>> this?
>>>>
>>>> Yes, it would, but please see above in the first place.
>>>>
>>>>> In my experience, it seems many folks use tools from their vendors or
>>>>> custom tools.
>>>>
>>>> This observation matches my own experience.
>>>
>>> For the sake of discussion, and from a kernel developer's point of view,
>>> should the tools be part of a separate project? Or should the tools be
>>> part of the kernel tree like perf, etc.? Assuming that this needs to
>>> start from scratch and not extending an existing project.
>>
>> It can be both in principle, but from the practical standpoint it is
>> more likely to get all of the people to use the same set of tools if
>> they are included into the kernel source tree.
> 
> Yazen,
> 
> You generally envision tools like this to only be used when there is a problem, and not something that's run critical path on every boot right?
>

Hi Mario,

Generally, I think yes. But you summarized one issue earlier, and that
is the case where a user doesn't explicitly fetch the information and it
gets lost. This can be especially painful if the issue is difficult to
reproduce or has a long time to failure. Of course, this is new and
supplemental info, but every clue helps during debug.

Some highlights from the ACPI spec...

The PHAT is not urgent nor actionable by the OS:
"It is not expected that the OSPM would act on the data being exposed."

The info may be useful on each boot regardless of any problems:
"The Reset Reason Health Record defines a mechanism to describe the
cause of the last system reset or boot. The record will be created as a
Health Record in the PHAT table. This provides a standard way for system
firmware to inform the operating system of the cause of the last reset.
This includes both expected and unexpected events to support insights
across a fleet of systems by way of collecting the reset reason records
on each boot."

Note that it says "last reset", so it doesn't seem intended to keep a
running log to be fetched later.

> If so, how about doing it in a high level language with easily importable libraries like Python?
>

This sounds good to me. Anything that can handle binary files.

> Then the tools can still be stored "in kernel tree" and distributed with distro "kernel tools" packages but you can more easily use them on random old kernels too since the binary via /sys/firmware/acpi/tables should be widely available.

Yes, I agree. And I think we should give examples for running the tools
as services at boot. And documentation is needed, of course.

I don't exactly follow your last statement. Do you mean that new ACPI
tables will be exposed in sysfs even without explicit kernel updates?

Thanks,
Yazen