Re: [PATCH v2 RESEND 2/4] ACPI: APEI: GHES: Add NVIDIA Vera decoder

[Shuai Xue <xueshuai@linux.alibaba.com>]

On 7/24/26 8:20 PM, Kai-Heng Feng wrote:
> Vera is NVIDIA's next-generation server SoC. Its CPER section uses a
> different GUID and a different binary layout from Grace, so it needs
> its own decoder. Without this, firmware-reported hardware errors on
> Vera platforms are received but not decoded.
> 
> Signed-off-by: Kai-Heng Feng <kaihengf@nvidia.com>
> ---
>   drivers/acpi/apei/ghes-nvidia.c | 368 ++++++++++++++++++++++++++++++--
>   drivers/acpi/apei/ghes-nvidia.h |  29 ++-
>   2 files changed, 382 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes-nvidia.c b/drivers/acpi/apei/ghes-nvidia.c
> index af445152def0..c74c155dd2ba 100644
> --- a/drivers/acpi/apei/ghes-nvidia.c
> +++ b/drivers/acpi/apei/ghes-nvidia.c
> @@ -7,18 +7,27 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/module.h>
> +#include <linux/overflow.h>
>   #include <linux/platform_device.h>
> +#include <linux/slab.h>
>   #include <linux/types.h>
> +#include <linux/unaligned.h>
>   #include <linux/uuid.h>
> +#include <kunit/visibility.h>
>   #include <acpi/ghes.h>
>   
> -#include <kunit/visibility.h>
>   #include "ghes-nvidia.h"
>   
> +#define NVIDIA_GHES_VERA_VERSION	1
> +
>   static const guid_t nvidia_grace_sec_guid =
>   	GUID_INIT(0x6d5244f2, 0x2712, 0x11ec,
>   		  0xbe, 0xa7, 0xcb, 0x3f, 0xdb, 0x95, 0xc7, 0x86);
>   
> +static const guid_t nvidia_vera_sec_guid =
> +	GUID_INIT(0x9068e568, 0x6ca0, 0x11f0,
> +		  0xae, 0xaf, 0x15, 0x93, 0x43, 0x59, 0x1e, 0xac);
> +
>   struct cper_sec_nvidia {
>   	char	signature[16];
>   	__le16	error_type;
> @@ -31,11 +40,51 @@ struct cper_sec_nvidia {
>   	struct nvidia_ghes_grace_reg regs[] __counted_by(number_regs);
>   };
>   
> +struct cper_sec_nvidia_vera_event {
> +	u8	version;
> +	u8	event_context_count;
> +	u8	source_device_type;
> +	u8	reserved;
> +	__le16	event_type;
> +	__le16	event_sub_type;
> +	__le64	event_link_id;
> +	char	source_module_signature[16];
> +} __packed;
> +
> +struct cper_sec_nvidia_vera_cpu_info {
> +	__le16	info_version;
> +	u8	info_size;
> +	u8	socket_number;
> +	__le32	architecture;
> +	u8	chip_serial_number[16];
> +	__le64	instance_base;
> +} __packed;
> +
> +struct cper_sec_nvidia_vera_context {
> +	__le32	context_size;
> +	__le16	context_version;
> +	__le16	reserved;
> +	__le16	data_format_type;
> +	__le16	data_format_version;
> +	__le32	data_size;
> +} __packed;
> +
>   struct nvidia_ghes_private {
>   	struct notifier_block	nb;
>   	struct device		*dev;
>   };
>   
> +VISIBLE_IF_KUNIT
> +enum nvidia_ghes_format nvidia_ghes_format_from_guid(const guid_t *guid)
> +{
> +	if (guid_equal(guid, &nvidia_grace_sec_guid))
> +		return NVIDIA_GHES_FORMAT_GRACE;
> +	if (guid_equal(guid, &nvidia_vera_sec_guid))
> +		return NVIDIA_GHES_FORMAT_VERA;
> +	return NVIDIA_GHES_FORMAT_UNKNOWN;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(nvidia_ghes_format_from_guid);
> +
>   VISIBLE_IF_KUNIT
>   int nvidia_ghes_decode_grace(struct device *dev, const void *buf,
>   			     size_t len,
> @@ -81,7 +130,7 @@ 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)
> +			       unsigned int index, u64 *addr, u64 *val)
>   {
>   	const struct nvidia_ghes_grace_reg *regs;
>   
> @@ -98,6 +147,220 @@ int nvidia_ghes_grace_reg_pair(const struct nvidia_ghes_decoded *decoded,
>   }
>   EXPORT_SYMBOL_IF_KUNIT(nvidia_ghes_grace_reg_pair);
>   
> +static int nvidia_ghes_vera_validate_context_data(u16 data_format_type,
> +						  u32 data_size)
> +{
> +	switch (data_format_type) {
> +	case 0:
> +		return 0;
> +	case 1:
> +		return data_size % 16 ? -EINVAL : 0;
> +	case 2:
> +	case 3:
> +		return data_size % 8 ? -EINVAL : 0;
> +	case 4:
> +		return data_size % 4 ? -EINVAL : 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +VISIBLE_IF_KUNIT
> +int nvidia_ghes_decode_vera(struct device *dev, const void *buf,
> +			    size_t len,
> +			    struct nvidia_ghes_decoded *decoded)
> +{
> +	const struct cper_sec_nvidia_vera_event *event = buf;
> +	const struct cper_sec_nvidia_vera_cpu_info *cpu_info;
> +	const struct cper_sec_nvidia_vera_context *context;
> +	const u8 *bytes = buf;
> +	size_t data_end_advance;
> +	size_t advance;
> +	size_t offset;
> +	int ret;
> +
> +	if (!buf || !decoded)
> +		return -EINVAL;
> +	if (len < sizeof(*event)) {
> +		if (dev)
> +			dev_err_ratelimited(dev, "Vera event header truncated (%zu < %zu)\n",
> +					    len, sizeof(*event));
> +		return -ENODATA;
> +	}
> +	if (event->version != NVIDIA_GHES_VERA_VERSION)
> +		return -EOPNOTSUPP;
> +	if (event->source_device_type != 0)
> +		return -EOPNOTSUPP;
> +
> +	offset = sizeof(*event);
> +	if (len - offset < sizeof(*cpu_info)) {
> +		if (dev)
> +			dev_err_ratelimited(dev, "Vera CPU info truncated (%zu < %zu)\n",
> +					    len - offset, sizeof(*cpu_info));
> +		return -ENODATA;
> +	}
> +
> +	cpu_info = (const void *)(bytes + offset);
> +	if (cpu_info->info_size < sizeof(*cpu_info)) {
> +		if (dev)
> +			dev_err_ratelimited(dev, "Vera CPU info size %u smaller than header %zu\n",
> +					    cpu_info->info_size, sizeof(*cpu_info));
> +		return -EINVAL;
> +	}
> +	if (len - offset < cpu_info->info_size) {
> +		if (dev)
> +			dev_err_ratelimited(dev, "Vera CPU info extends past section (%u > %zu)\n",
> +					    cpu_info->info_size, len - offset);
> +		return -ENODATA;
> +	}
> +
> +	offset += cpu_info->info_size;
> +	if (event->event_context_count > NVIDIA_GHES_MAX_CONTEXTS) {
> +		if (dev)
> +			dev_err_ratelimited(dev, "Vera context count %u exceeds maximum %u\n",
> +					    event->event_context_count,
> +					    NVIDIA_GHES_MAX_CONTEXTS);
> +		return -E2BIG;
> +	}
> +
> +	memset(decoded, 0, sizeof(*decoded));
> +	decoded->format = NVIDIA_GHES_FORMAT_VERA;
> +	memcpy(decoded->signature, event->source_module_signature,
> +	       sizeof(event->source_module_signature));
> +	decoded->signature[sizeof(event->source_module_signature)] = '\0';
> +	decoded->event_context_count = event->event_context_count;
> +	decoded->source_device_type = event->source_device_type;
> +	decoded->event_type = get_unaligned_le16(&event->event_type);
> +	decoded->event_sub_type = get_unaligned_le16(&event->event_sub_type);
> +	decoded->event_link_id = get_unaligned_le64(&event->event_link_id);
> +	decoded->socket = cpu_info->socket_number;
> +	decoded->architecture = get_unaligned_le32(&cpu_info->architecture);
> +	memcpy(decoded->chip_serial_number, cpu_info->chip_serial_number,
> +	       sizeof(cpu_info->chip_serial_number));
> +	decoded->instance_base = get_unaligned_le64(&cpu_info->instance_base);
> +
> +	for (int i = 0; i < event->event_context_count; i++) {
> +		struct nvidia_ghes_vera_context *decoded_context = &decoded->contexts[i];
> +		u32 context_size;
> +		u32 data_size;
> +		u16 data_format_type;
> +
> +		if (len - offset < sizeof(*context)) {
> +			if (dev)
> +				dev_err_ratelimited(dev, "Vera context[%d] header truncated (%zu < %zu)\n",
> +						    i, len - offset, sizeof(*context));
> +			return -ENODATA;
> +		}
> +
> +		context = (const void *)(bytes + offset);
> +		context_size = get_unaligned_le32(&context->context_size);
> +		data_format_type = get_unaligned_le16(&context->data_format_type);
> +		data_size = get_unaligned_le32(&context->data_size);
> +
> +		if (context_size < sizeof(*context)) {
> +			if (dev)
> +				dev_err_ratelimited(dev,
> +						    "Vera context[%d] size %u smaller than header %zu\n",
> +						    i, context_size, sizeof(*context));
> +			return -EINVAL;
> +		}
> +		if (data_format_type > 4) {
> +			if (dev)
> +				dev_dbg(dev,
> +					"Vera context[%d] unsupported data format %u\n",
> +					i, data_format_type);
> +			return -EOPNOTSUPP;
> +		}
> +		if (check_add_overflow((size_t)data_size, sizeof(*context),
> +				       &data_end_advance)) {
> +			if (dev)
> +				dev_err_ratelimited(dev,
> +						    "Vera context[%d] data_size %u overflows section accounting\n",
> +						    i, data_size);
> +			return -EOVERFLOW;
> +		}
> +
> +		if (data_end_advance > len - offset) {
> +			if (dev)
> +				dev_err_ratelimited(dev,
> +						    "Vera context[%d] data extends past section (%zu > %zu)\n",
> +						    i, data_end_advance, len - offset);
> +			return -ENODATA;
> +		}
> +
> +		/*
> +		 * Some Vera payloads use only the header size here and
> +		 * place the format-specific payload immediately after it.
> +		 */
> +		if (context_size == sizeof(*context))
> +			advance = data_end_advance;
> +		else if (data_size <= context_size - sizeof(*context))
> +			advance = context_size;
> +		else {
> +			if (dev)
> +				dev_err_ratelimited(dev,
> +						    "Vera context[%d] data_size %u exceeds context_size %u\n",
> +						    i, data_size, context_size);
> +			return -EINVAL;
> +		}

This is the part that I would like to understand better before relying on
this parser in production.

The code accepts two different encodings:

   - context_size == sizeof(*context), with the payload length coming from
     data_size, and
   - context_size covering both the context header and the payload.

If both forms are part of the Vera CPER ABI, please make that explicit in
the comment, preferably with the terminology used by the firmware
specification.  Otherwise this looks like the parser is guessing around
ambiguous firmware data.

Please also add a KUnit case with more than one context, ideally covering
the second form where context_size is larger than the header and the next
context is found via offset += context_size.  That would test the most
important offset accounting rule here.


> +
> +		if (advance > len - offset) {
> +			if (dev)
> +				dev_err_ratelimited(dev,
> +						    "Vera context[%d] advance %zu extends past section (%zu)\n",
> +						    i, advance, len - offset);
> +			return -ENODATA;
> +		}
> +
> +		ret = nvidia_ghes_vera_validate_context_data(data_format_type, data_size);
> +		if (ret) {
> +			if (dev)
> +				dev_err_ratelimited(dev,
> +						    "Vera context[%d] format %u rejected data_size %u (ret=%d)\n",
> +						    i, data_format_type, data_size, ret);
> +			return ret;
> +		}
> +
> +		decoded_context->context_size = context_size;
> +		decoded_context->context_version =
> +			get_unaligned_le16(&context->context_version);
> +		decoded_context->data_format_type = data_format_type;
> +		decoded_context->data_format_version =
> +			get_unaligned_le16(&context->data_format_version);
> +		decoded_context->data_size = data_size;
> +		decoded_context->data = bytes + offset + sizeof(*context);
> +		offset += advance;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(nvidia_ghes_decode_vera);
> +
> +VISIBLE_IF_KUNIT
> +int nvidia_ghes_vera_context_entry_count(const struct nvidia_ghes_vera_context *ctx)
> +{
> +	if (!ctx)
> +		return -EINVAL;
> +	if (ctx->data_size > INT_MAX)
> +		return -EOVERFLOW;
> +
> +	switch (ctx->data_format_type) {
> +	case 0:
> +		return 0;
> +	case 1:
> +		return ctx->data_size / 16;
> +	case 2:
> +		return ctx->data_size / 8;
> +	case 3:
> +		return ctx->data_size / 8;
> +	case 4:
> +		return ctx->data_size / 4;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL_IF_KUNIT(nvidia_ghes_vera_context_entry_count);
> +
>   static void nvidia_ghes_print_grace(struct device *dev,
>   				    const struct nvidia_ghes_decoded *decoded,
>   				    bool fatal)
> @@ -111,7 +374,8 @@ static void nvidia_ghes_print_grace(struct device *dev,
>   	dev_printk(level, dev, "severity: %u\n", decoded->severity);
>   	dev_printk(level, dev, "socket: %u\n", decoded->socket);
>   	dev_printk(level, dev, "number_regs: %u\n", decoded->number_regs);
> -	dev_printk(level, dev, "instance_base: 0x%016llx\n", decoded->instance_base);
> +	dev_printk(level, dev, "instance_base: 0x%016llx\n",
> +		   decoded->instance_base);
>   
>   	for (int i = 0; i < decoded->number_regs; i++) {
>   		if (nvidia_ghes_grace_reg_pair(decoded, i, &addr, &val))
> @@ -121,12 +385,52 @@ static void nvidia_ghes_print_grace(struct device *dev,
>   	}
>   }
>   
> +static void nvidia_ghes_print_vera(struct device *dev,
> +				   const struct nvidia_ghes_decoded *decoded,
> +				   bool fatal, unsigned long ghes_severity)
> +{
> +	const char *level = fatal ? KERN_ERR : KERN_INFO;
> +
> +	dev_printk(level, dev, "signature: %s\n", decoded->signature);
> +	dev_printk(level, dev, "event_type: %u\n", decoded->event_type);
> +	dev_printk(level, dev, "event_sub_type: %u\n", decoded->event_sub_type);
> +	dev_printk(level, dev, "ghes_severity: %lu\n", ghes_severity);
> +	dev_printk(level, dev, "event_link_id: 0x%016llx\n",
> +		   decoded->event_link_id);
> +	dev_printk(level, dev, "socket: %u\n", decoded->socket);
> +	dev_printk(level, dev, "architecture: 0x%x\n", decoded->architecture);
> +	dev_printk(level, dev, "chip_serial_number: %*phN\n",
> +		   (int)sizeof(decoded->chip_serial_number),
> +		   decoded->chip_serial_number);
> +	dev_printk(level, dev, "instance_base: 0x%016llx\n", decoded->instance_base);
> +	dev_printk(level, dev, "event_context_count: %u\n", decoded->event_context_count);
> +
> +	for (int i = 0; i < decoded->event_context_count; i++) {
> +		const struct nvidia_ghes_vera_context *ctx = &decoded->contexts[i];
> +		int entries = nvidia_ghes_vera_context_entry_count(ctx);
> +
> +		dev_printk(level, dev,
> +			   "context[%d]: version=%u format=%u format_version=%u context_size=%u data_size=%u\n",
> +			   i, ctx->context_version, ctx->data_format_type,
> +			   ctx->data_format_version, ctx->context_size, ctx->data_size);
> +		if (ctx->data_format_type == 0 && ctx->data_size > 0) {
> +			int prefix_len = ctx->data_size > 16 ? 16 : ctx->data_size;
> +
> +			dev_printk(level, dev, "context[%d]_opaque_prefix: %*phN\n",
> +				   i, prefix_len, ctx->data);
> +		} else if (entries >= 0) {
> +			dev_printk(level, dev, "context[%d]_entries: %d\n", i, entries);
> +		}
> +	}
> +}
> +
>   static int nvidia_ghes_notify(struct notifier_block *nb,
>   			      unsigned long event, void *data)
>   {
>   	struct acpi_hest_generic_data *gdata = data;
> -	struct nvidia_ghes_decoded decoded;
> +	struct nvidia_ghes_decoded *decoded;
>   	struct nvidia_ghes_private *priv;
> +	enum nvidia_ghes_format format;
>   	const void *payload;
>   	guid_t sec_guid;
>   	u32 len;
> @@ -134,26 +438,64 @@ static int nvidia_ghes_notify(struct notifier_block *nb,
>   	bool fatal;
>   
>   	import_guid(&sec_guid, gdata->section_type);
> -	if (!guid_equal(&sec_guid, &nvidia_grace_sec_guid))
> +	format = nvidia_ghes_format_from_guid(&sec_guid);
> +	if (format == NVIDIA_GHES_FORMAT_UNKNOWN)
>   		return NOTIFY_DONE;
>   
>   	priv = container_of(nb, struct nvidia_ghes_private, nb);
>   	len = acpi_hest_get_error_length(gdata);
> +
>   	payload = acpi_hest_get_payload(gdata);
>   	fatal = event >= GHES_SEV_RECOVERABLE;
> +	decoded = kzalloc_obj(*decoded);
> +	if (!decoded) {
> +		dev_err_ratelimited(priv->dev,
> +				    "Failed to allocate NVIDIA CPER decode buffer\n");
> +		return NOTIFY_OK;
> +	}

This runs from the GHES vendor workqueue, so sleeping allocation is not
the issue.  But this is still the error reporting path, and struct
nvidia_ghes_decoded looks small enough to avoid a per-event allocation.
Please consider keeping it on the stack, or explain why heap allocation
is needed here.

> +
> +	switch (format) {
> +	case NVIDIA_GHES_FORMAT_GRACE:
> +		ret = nvidia_ghes_decode_grace(priv->dev, payload, len, decoded);
> +		break;
> +	case NVIDIA_GHES_FORMAT_VERA:
> +		ret = nvidia_ghes_decode_vera(priv->dev, payload, len, decoded);
> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
>   
> -	ret = nvidia_ghes_decode_grace(priv->dev, payload, len, &decoded);
>   	if (ret) {
> -		dev_err(priv->dev,
> -			"Malformed NVIDIA CPER section, error_data_length: %u, ret: %d\n",
> -			len, ret);
> -		return NOTIFY_OK;
> +		if (ret == -EOPNOTSUPP && format == NVIDIA_GHES_FORMAT_VERA)
> +			dev_info(priv->dev,
> +				 "Unsupported NVIDIA Vera CPER section, error_data_length: %u, ret: %d\n",
> +				 len, ret);
> +		else if (format == NVIDIA_GHES_FORMAT_GRACE)
> +			dev_err(priv->dev,
> +				"Malformed NVIDIA Grace CPER section, error_data_length: %u, ret: %d\n",
> +				len, ret);
> +		else
> +			dev_err(priv->dev,
> +				"Malformed NVIDIA Vera CPER section, error_data_length: %u, ret: %d\n",
> +				len, ret);
> +		goto out;

Most of the detailed parser errors are already rate limited inside the
decoder, but this outer message is not.  If firmware repeatedly reports an
unsupported or malformed vendor section, this can still generate one line
per event.

Please make the outer message rate limited as well, or avoid the second
message when the decoder has already emitted a detailed diagnostic.

Thanks.
Shuai