Re: [PATCH v2 RESEND 1/4] ACPI: APEI: GHES: Refactor Grace decoder helpers

On 7/24/26 8:20 PM, Kai-Heng Feng wrote:
> Split the Grace CPER processing into a separate decode step and a
> print step so the parser can be exercised by KUnit without a live
> ACPI device. Introduce ghes-nvidia.h to hold shared types that the
> Vera decoder added in the next commit will also reference.
> 
> Signed-off-by: Kai-Heng Feng <[email protected]>
> ---
> v2:
>   - No change.
> 
>   drivers/acpi/apei/ghes-nvidia.c | 148 +++++++++++++++++++++-----------
>   drivers/acpi/apei/ghes-nvidia.h |  38 ++++++++
>   2 files changed, 137 insertions(+), 49 deletions(-)
>   create mode 100644 drivers/acpi/apei/ghes-nvidia.h
> 
> diff --git a/drivers/acpi/apei/ghes-nvidia.c b/drivers/acpi/apei/ghes-nvidia.c
> index 597275d81de8..af445152def0 100644
> --- a/drivers/acpi/apei/ghes-nvidia.c
> +++ b/drivers/acpi/apei/ghes-nvidia.c
> @@ -12,7 +12,10 @@
>   #include <linux/uuid.h>
>   #include <acpi/ghes.h>
>   
> -static const guid_t nvidia_sec_guid =
> +#include <kunit/visibility.h>
> +#include "ghes-nvidia.h"
> +
> +static const guid_t nvidia_grace_sec_guid =
>   	GUID_INIT(0x6d5244f2, 0x2712, 0x11ec,
>   		  0xbe, 0xa7, 0xcb, 0x3f, 0xdb, 0x95, 0xc7, 0x86);
>   
> @@ -25,10 +28,7 @@ struct cper_sec_nvidia {
>   	u8	number_regs;
>   	u8	reserved;
>   	__le64	instance_base;
> -	struct {
> -		__le64	addr;
> -		__le64	val;
> -	} regs[] __counted_by(number_regs);
> +	struct nvidia_ghes_grace_reg regs[] __counted_by(number_regs);
>   };

The Vera side treats the payload as a packed wire format and uses
get_unaligned_leXX(), but the Grace side still casts the CPER payload to
struct cper_sec_nvidia and directly dereferences __le16/__le64 fields.

Unless the GHES vendor payload is guaranteed to be naturally aligned here,
please make the Grace decoder follow the same rule as Vera and use
get_unaligned_le16()/get_unaligned_le64() for all multi-byte fields,
including the register pairs.

>   
>   struct nvidia_ghes_private {
> @@ -36,73 +36,123 @@ struct nvidia_ghes_private {
>   	struct device		*dev;
>   };
>   
> -static void nvidia_ghes_print_error(struct device *dev,
> -				    const struct cper_sec_nvidia *nvidia_err,
> -				    size_t error_data_length, bool fatal)
> +VISIBLE_IF_KUNIT
> +int nvidia_ghes_decode_grace(struct device *dev, const void *buf,
> +			     size_t len,
> +			     struct nvidia_ghes_decoded *decoded)
>   {
> -	const char *level = fatal ? KERN_ERR : KERN_INFO;
> +	const struct cper_sec_nvidia *nvidia_err = buf;
>   	size_t min_size;
>   
> -	dev_printk(level, dev, "signature: %.16s\n", nvidia_err->signature);
> -	dev_printk(level, dev, "error_type: %u\n", le16_to_cpu(nvidia_err->error_type));
> -	dev_printk(level, dev, "error_instance: %u\n", le16_to_cpu(nvidia_err->error_instance));
> -	dev_printk(level, dev, "severity: %u\n", nvidia_err->severity);
> -	dev_printk(level, dev, "socket: %u\n", nvidia_err->socket);
> -	dev_printk(level, dev, "number_regs: %u\n", nvidia_err->number_regs);
> -	dev_printk(level, dev, "instance_base: 0x%016llx\n",
> -		   le64_to_cpu(nvidia_err->instance_base));
> -
> -	if (nvidia_err->number_regs == 0)
> -		return;
> -
> -	/*
> -	 * Validate that all registers fit within error_data_length.
> -	 * Each register pair is two little-endian u64s.
> -	 */
> +	if (!buf || !decoded)
> +		return -EINVAL;
> +	if (len < sizeof(*nvidia_err)) {
> +		if (dev)
> +			dev_err(dev, "Section too small (%zu < %zu)\n",
> +				len, sizeof(*nvidia_err));
> +		return -ENODATA;
> +	}
> +
>   	min_size = struct_size(nvidia_err, regs, nvidia_err->number_regs);
> -	if (error_data_length < min_size) {
> -		dev_err(dev, "Invalid number_regs %u (section size %zu, need %zu)\n",
> -			nvidia_err->number_regs, error_data_length, min_size);
> -		return;
> +	if (len < min_size) {
> +		if (dev)
> +			dev_err(dev,
> +				"Invalid number_regs %u (section size %zu, need %zu)\n",
> +				nvidia_err->number_regs, len, min_size);
> +		return -ENODATA;
>   	}
>   
> -	for (int i = 0; i < nvidia_err->number_regs; i++)
> +	memset(decoded, 0, sizeof(*decoded));
> +	decoded->format = NVIDIA_GHES_FORMAT_GRACE;
> +	memcpy(decoded->signature, nvidia_err->signature, sizeof(nvidia_err->signature));
> +	decoded->signature[sizeof(nvidia_err->signature)] = '\0';
> +	decoded->error_type = le16_to_cpu(nvidia_err->error_type);
> +	decoded->error_instance = le16_to_cpu(nvidia_err->error_instance);
> +	decoded->severity = nvidia_err->severity;
> +	decoded->socket = nvidia_err->socket;
> +	decoded->number_regs = nvidia_err->number_regs;
> +	decoded->instance_base = le64_to_cpu(nvidia_err->instance_base);
> +	if (nvidia_err->number_regs)
> +		decoded->grace_regs = nvidia_err->regs;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(nvidia_ghes_decode_grace);
> +
> +VISIBLE_IF_KUNIT
> +int nvidia_ghes_grace_reg_pair(const struct nvidia_ghes_decoded *decoded,
> +				      unsigned int index, u64 *addr, u64 *val)
> +{
> +	const struct nvidia_ghes_grace_reg *regs;
> +
> +	if (!decoded || decoded->format != NVIDIA_GHES_FORMAT_GRACE || !addr || !val)
> +		return -EINVAL;
> +	if (index >= decoded->number_regs)
> +		return -ERANGE;
> +
> +	regs = decoded->grace_regs;

This is fine for objects produced by nvidia_ghes_decode_grace(), but now
that the helper is visible to KUnit, please either document that contract
or reject number_regs != 0 && !decoded->grace_regs here.

Thanks.
Shuai