Re: [RFC PATCH 2/3] ACPI: PHAT: Add generic helper to parse PHAT records

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/3/26 01:33, K Prateek Nayak wrote:
> The ACPI Specification v6.5 [1] under Sec. 5.2.31 "Platform Health
> Assessment Table (PHAT)" provides a well defined data format for reset
> reason records, including the ones that are vendor specific.
> 
> Provide a generic helper to locate the vendor specific records and parse
> them into a buffer that can be consumed by the users.
> 
> The first user for the interface is added in the subsequent commit with
> AMD processors using it to parse the s5_RESET_STATUS from PHAT records.
> 
> Make the Kconfig depend on ACPI && CPU_SUP_AMD to prevent the need for
> unnecessarily compiling in ACPI PHAT support when not needed.
> 
> Link: https://uefi.org/sites/default/files/resources/ACPI_Spec_6_5_Aug29.pdf [1]
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
>   drivers/acpi/Kconfig     |  11 ++++
>   drivers/acpi/Makefile    |   1 +
>   drivers/acpi/acpi_phat.c | 133 +++++++++++++++++++++++++++++++++++++++
>   include/linux/acpi.h     |  14 +++++
>   4 files changed, 159 insertions(+)
>   create mode 100644 drivers/acpi/acpi_phat.c
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index f165d14cf61a..494c757a62d7 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -607,6 +607,17 @@ config ACPI_PRMT
>   	  substantially increase computational overhead related to the
>   	  initialization of some server systems.
>   
> +config ACPI_PHAT
> +	bool "ACPI PHAT Record parsing support"
> +	depends on X86 && ACPI && CPU_SUP_AMD
> +	default y
> +	help
> +	  The Platform Health Assessment Table (PHAT) allows platforms to
> +	  expose read-only data for diagnostics and health assessments.
> +
> +	  Enable this feature to access generic helpers for parsing PHAT
> +	  data records.
> +
>   endif	# ACPI
>   
>   config X86_PM_TIMER
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index d1b0affb844f..d6cf252ca4ea 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -108,6 +108,7 @@ obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
>   obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
>   obj-$(CONFIG_ACPI_PPTT) 	+= pptt.o
>   obj-$(CONFIG_ACPI_PFRUT)	+= pfr_update.o pfr_telemetry.o
> +obj-$(CONFIG_ACPI_PHAT) 	+= acpi_phat.o
>   
>   # processor has its own "processor." module_param namespace
>   processor-y			:= processor_driver.o processor_thermal.o
> diff --git a/drivers/acpi/acpi_phat.c b/drivers/acpi/acpi_phat.c
> new file mode 100644
> index 000000000000..de612ae018e0
> --- /dev/null
> +++ b/drivers/acpi/acpi_phat.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Helpers for parsing Platform Health Assessment Table (PHAT) records.
> + */
> +#include <linux/kernel.h>
> +#include <linux/acpi.h>
> +
> +#include <acpi/actbl2.h>
> +
> +/*
> + * __phat_get_firmware_health_data() - Internal helper to locate the
> + * "Reset Reason Health Record" within the Platform Health Assessment
> + * Table (PHAT).
> + *
> + * @table: Pointer to the beginning of PHAT.
> + *
> + * Return: A pointer within the @table if "Reset Reason Health Record"
> + * is found; NULL otherwise.
> + */
> +static struct acpi_phat_health_data *
> +__phat_get_firmware_health_data(struct acpi_table_phat *table)
> +{
> +	unsigned int length = table->header.length;
> +	void *header = table;
> +
> +	/* No records. */
> +	if (length <= sizeof(struct acpi_table_header))
> +		return NULL;
> +
> +	/*
> +	 * Advance to the end of the table header
> +	 * where the first record starts.
> +	 */
> +	header += sizeof(struct acpi_table_header);
> +	length -= sizeof(struct acpi_table_header);
> +
> +	/*
> +	 * Search for PHAT firmware health data record header
> +	 * with type == ACPI_PHAT_TYPE_FW_HEALTH_DATA.
> +	 */
> +	while (length) {
> +		struct acpi_phat_health_data *data = header;
> +
> +		if (data->header.type == ACPI_PHAT_TYPE_FW_HEALTH_DATA)
> +			return header;
> +
> +		/* Move to the next header */
> +		header += data->header.length;
> +		length -= data->header.length;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * acpi_phat_get_vendor_reset_reason - Find a "Vendor Specific Reset Reason
> + * Entry" with the matching @guid from the "Firmware Health Data Record". If
> + * successfully located, the function will allocate an object of the size
> + * "acpi_phat_vendor_element.length" and return a pointer populated with the
> + * content of the record.
> + *
> + * @guid: The "Vendor Data ID" of the reset reason record.
> + *
> + * Return: A valid pointer to an allocated "acpi_phat_vendor_element" populated
> + * with the data from the record with matching @guid; an ERR_PTR() otherwise if
> + * no matching records were found, or if the element could not be allocated.
> + * If a valid pointer was returned, the user must call
> + * acpi_phat_put_vendor_reset_reason() for the object once done to reclaim the
> + * allocated memory.
> + */
> +struct acpi_phat_vendor_element *acpi_phat_get_vendor_reset_reason(guid_t *guid)
> +{
> +	struct acpi_table_header *phat_tbl __free(acpi_put_table) = NULL;
> +	struct acpi_phat_health_data *fw_health_data;
> +	struct acpi_phat_device_data *dev_data;
> +	acpi_status status;
> +	void *data;
> +	int i;
> +
> +	status = acpi_get_table(ACPI_SIG_PHAT, 0, &phat_tbl);
> +	if (ACPI_FAILURE(status))
> +		return ERR_PTR(-ENODEV);

For some further sanity checking, should you look at the PHAT revision 
here matches 2 as well?  Table 55 in the linked spec.

> +
> +	fw_health_data =
> +		__phat_get_firmware_health_data((struct acpi_table_phat *)phat_tbl);
> +	if (!fw_health_data)
> +		return ERR_PTR(-ENODEV);
> +
> +	/* Check if Device-specific data record is present. */
> +	if (!fw_health_data->device_specific_offset)
> +		return ERR_PTR(-ENODEV);
> +
> +	dev_data = (void *)fw_health_data + fw_health_data->device_specific_offset;
> +	if (!dev_data->vendor_count)
> +		return ERR_PTR(-ENODEV);
> +
> +	/* Vendor data starts after Device-specific data */
> +	data = (void *)dev_data + sizeof(*dev_data);
> +
> +	for (i = 0; i <= dev_data->vendor_count; ++i) {
> +		struct acpi_phat_vendor_element *vendor_data = data;
> +		int length = vendor_data->length;
> +
> +		/*
> +		 * Move to the next Vendor specific entry if
> +		 * the GUID of entry doesn't match.
> +		 */
> +		if (!guid_equal(guid, (guid_t *)vendor_data->vendor_guid)) {
> +			data += vendor_data->length;
> +			continue;
> +		}
> +
> +		vendor_data = kmalloc(length, GFP_KERNEL);
> +		if (!vendor_data)
> +			return ERR_PTR(-ENOMEM);
> +
> +		memcpy(vendor_data, data, length);
> +		return vendor_data;
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +/**
> + * acpi_phat_put_vendor_reset_reason - Reclaim the object allocated by
> + * acpi_phat_get_vendor_reset_reason().
> + *
> + * @reason: A valid pointer returned by acpi_phat_get_vendor_reset_reason()
> + */
> +void acpi_phat_put_vendor_reset_reason(struct acpi_phat_vendor_element *reason)
> +{
> +	kfree(reason);
> +}
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 67effb91fa98..daed1d66ca43 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1627,6 +1627,20 @@ extern int acpi_ffh_address_space_arch_handler(acpi_integer *value,
>   static inline void acpi_init_ffh(void) { }
>   #endif
>   
> +#ifdef CONFIG_ACPI_PHAT
> +struct acpi_phat_vendor_element *acpi_phat_get_vendor_reset_reason(guid_t *guid);
> +void acpi_phat_put_vendor_reset_reason(struct acpi_phat_vendor_element *reason);
> +#else
> +static inline struct acpi_phat_vendor_element *
> +acpi_phat_get_vendor_reset_reason(guid_t *guid)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static inline void
> +acpi_phat_put_vendor_reset_reason(struct acpi_phat_vendor_element *reason) { }
> +#endif
> +
>   #ifdef CONFIG_ACPI
>   extern void acpi_device_notify(struct device *dev);
>   extern void acpi_device_notify_remove(struct device *dev);