Re: [RFC PATCH 3/3] x86/cpu/amd: Fetch S5_RESET_STATUS from PHAT when supported

On 6/3/26 01:33, K Prateek Nayak wrote:
> Firmware can consume the S5_RESET_STATUS from the FCH register and the
> OS may see an incorrect value at the time of boot on certain platforms.
> 
> Vilas, Yazen noted that S5_RESET_STATUS is also populated in ACPI PHAT
> on newer AMD platforms which is more reliable than the status from the
> FCH register.
> 
> Use the S5_RESET_STATUS from the ACPI PHAT which is described in the
> "Platform Health Assessment Table (PHAT)" section of "AMD Family 1Ah
> Models 00h–0Fh and Models 10h–1Fh ACPI v6.5 Porting Guide" [1] on
> supported platform and fallback to the legacy FCH path if the PHAT
> record is not found.
> 
> Unlike the FCH register, PHAT only provides a read-only value which
> cannot be cleared once consumed and will persist across kexec boots.
> 
> Link: https://docs.amd.com/v/u/en-US/58088_0.90_PUB [1]
> Suggested-by: Vilas Sridharan <[email protected]>
> Suggested-by: Yazen Ghannam <[email protected]>
> Signed-off-by: K Prateek Nayak <[email protected]>
> ---
>   arch/x86/kernel/cpu/amd.c | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 2f8e8ff2d000..d3dee7fd4c51 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   #include <linux/export.h>
> +#include <linux/acpi.h>
>   #include <linux/bitops.h>
>   #include <linux/dmi.h>
>   #include <linux/elf.h>
> @@ -11,6 +12,8 @@
>   #include <linux/random.h>
>   #include <linux/topology.h>
>   #include <linux/platform_data/x86/amd-fch.h>
> +
> +#include <acpi/actbl2.h>
>   #include <asm/processor.h>
>   #include <asm/apic.h>
>   #include <asm/cacheinfo.h>
> @@ -1342,8 +1345,21 @@ static const char * const s5_reset_reason_txt[] = {
>   	[31] = "a software sync flood event occurred",
>   };
>   
> +/*
> + * "AMD Family 1Ah Models 00h–0Fh and Models 10h–1Fh ACPI v6.5 Porting Guide"
> + * specifies the vendor-specific GUID of FCH::PM::S5_RESET_STATUS record as
> + * "1f425831-da46-4f65-9296-3c4d44c387ab" alongside the format of reset reason.
> + */
> +#define S5_RESET_STATUS_GUID GUID_INIT(0x1f425831, 0xda46, 0x4f65, 0x92, 0x96,\
> +				       0x3c, 0x4d, 0x44, 0xc3, 0x87, 0xab)
> +
>   static __init int print_s5_reset_status_mmio(void)
>   {
> +	struct reset_status_record {
> +		struct acpi_phat_vendor_element		header;
> +		u32					s5_reset_status;
> +	} __packed *record;
> +	guid_t guid = S5_RESET_STATUS_GUID;
>   	void __iomem *addr;
>   	u32 value;
>   	int i;
> @@ -1351,6 +1367,23 @@ static __init int print_s5_reset_status_mmio(void)
>   	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
>   		return 0;
>   
> +	record = (void *)acpi_phat_get_vendor_reset_reason(&guid);
> +	if (!IS_ERR(record)) {
> +		bool record_valid = false;
> +
> +		/* Sanity check before using the parsed record. */
> +		if (record->header.length == sizeof(*record)) {

Why not do this sanity check in acpi_phat_get_vendor_reset_reason() 
directly?  Then we can always assume if something is returned it's valid 
and it's simpler here (and any other potential callers in future).

> +			pr_debug("Using ACPI PHAT record for S5_RESET_STATUS\n");
> +			value = record->s5_reset_status;
> +			record_valid = true;
> +		}
> +
> +		acpi_phat_put_vendor_reset_reason((void *)record);
> +
> +		if (record_valid)
> +			goto parse_status;
> +	}
> +
>   	addr = ioremap(FCH_PM_BASE + FCH_PM_S5_RESET_STATUS, sizeof(value));
>   	if (!addr)
>   		return 0;
> @@ -1374,6 +1407,7 @@ static __init int print_s5_reset_status_mmio(void)
>   	iowrite32(value, addr);
>   	iounmap(addr);
>   
> +parse_status:
>   	for (i = 0; i < ARRAY_SIZE(s5_reset_reason_txt); i++) {
>   		if (!(value & BIT(i)))
>   			continue;