For one particular error (Error), we can't call error_setg() for twice.
Otherwise, the assert(*errp == NULL) will be triggered unexpectedly in
error_setv(). In ghes_record_cper_errors(), get_ghes_source_offsets()
can return a error initialized by error_setg(). Without bailing on
this error, it can call into the second error_setg() due to the
unexpected value from the read acknowledgement register.
Bail early in ghes_record_cper_errors() when error is received from
get_ghes_source_offsets() to avoid the exception.
Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
---
hw/acpi/ghes.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 6366c74248..c35883dfa9 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -443,7 +443,7 @@ static void get_hw_error_offsets(uint64_t ghes_addr,
*read_ack_register_addr = ghes_addr + sizeof(uint64_t);
}
-static void get_ghes_source_offsets(uint16_t source_id,
+static bool get_ghes_source_offsets(uint16_t source_id,
uint64_t hest_addr,
uint64_t *cper_addr,
uint64_t *read_ack_start_addr,
@@ -474,7 +474,7 @@ static void get_ghes_source_offsets(uint16_t source_id,
/* For now, we only know the size of GHESv2 table */
if (type != ACPI_GHES_SOURCE_GENERIC_ERROR_V2) {
error_setg(errp, "HEST: type %d not supported.", type);
- return;
+ return false;
}
/* Compare CPER source ID at the GHESv2 structure */
@@ -488,7 +488,7 @@ static void get_ghes_source_offsets(uint16_t source_id,
}
if (i == num_sources) {
error_setg(errp, "HEST: Source %d not found.", source_id);
- return;
+ return false;
}
/* Navigate through table address pointers */
@@ -508,6 +508,8 @@ static void get_ghes_source_offsets(uint16_t source_id,
cpu_physical_memory_read(hest_read_ack_addr, read_ack_start_addr,
sizeof(*read_ack_start_addr));
*read_ack_start_addr = le64_to_cpu(*read_ack_start_addr);
+
+ return true;
}
NotifierList acpi_generic_error_notifiers =
@@ -526,9 +528,10 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len,
if (!ags->use_hest_addr) {
get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
&cper_addr, &read_ack_register_addr);
- } else {
- get_ghes_source_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
- &cper_addr, &read_ack_register_addr, errp);
+ } else if (!get_ghes_source_offsets(source_id,
+ le64_to_cpu(ags->hest_addr_le),
+ &cper_addr, &read_ack_register_addr, errp)) {
+ return;
}
cpu_physical_memory_read(read_ack_register_addr,
--
2.51.1On Thu, 27 Nov 2025 10:44:34 +1000
Gavin Shan <gshan@redhat.com> wrote:
> For one particular error (Error), we can't call error_setg() for twice.
^^^^^^^^^^^^^^^^^^^^^^^^^^^
I can't really parse that
maybe rephrase it to make some sense?
> Otherwise, the assert(*errp == NULL) will be triggered unexpectedly in
> error_setv(). In ghes_record_cper_errors(), get_ghes_source_offsets()
> can return a error initialized by error_setg(). Without bailing on
> this error, it can call into the second error_setg() due to the
> unexpected value from the read acknowledgement register.
>
> Bail early in ghes_record_cper_errors() when error is received from
> get_ghes_source_offsets() to avoid the exception.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
patch itself LGTM
and with commit message fixed
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/acpi/ghes.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 6366c74248..c35883dfa9 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -443,7 +443,7 @@ static void get_hw_error_offsets(uint64_t ghes_addr,
> *read_ack_register_addr = ghes_addr + sizeof(uint64_t);
> }
>
> -static void get_ghes_source_offsets(uint16_t source_id,
> +static bool get_ghes_source_offsets(uint16_t source_id,
> uint64_t hest_addr,
> uint64_t *cper_addr,
> uint64_t *read_ack_start_addr,
> @@ -474,7 +474,7 @@ static void get_ghes_source_offsets(uint16_t source_id,
> /* For now, we only know the size of GHESv2 table */
> if (type != ACPI_GHES_SOURCE_GENERIC_ERROR_V2) {
> error_setg(errp, "HEST: type %d not supported.", type);
> - return;
> + return false;
> }
>
> /* Compare CPER source ID at the GHESv2 structure */
> @@ -488,7 +488,7 @@ static void get_ghes_source_offsets(uint16_t source_id,
> }
> if (i == num_sources) {
> error_setg(errp, "HEST: Source %d not found.", source_id);
> - return;
> + return false;
> }
>
> /* Navigate through table address pointers */
> @@ -508,6 +508,8 @@ static void get_ghes_source_offsets(uint16_t source_id,
> cpu_physical_memory_read(hest_read_ack_addr, read_ack_start_addr,
> sizeof(*read_ack_start_addr));
> *read_ack_start_addr = le64_to_cpu(*read_ack_start_addr);
> +
> + return true;
> }
>
> NotifierList acpi_generic_error_notifiers =
> @@ -526,9 +528,10 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len,
> if (!ags->use_hest_addr) {
> get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
> &cper_addr, &read_ack_register_addr);
> - } else {
> - get_ghes_source_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
> - &cper_addr, &read_ack_register_addr, errp);
> + } else if (!get_ghes_source_offsets(source_id,
> + le64_to_cpu(ags->hest_addr_le),
> + &cper_addr, &read_ack_register_addr, errp)) {
> + return;
> }
>
> cpu_physical_memory_read(read_ack_register_addr,
Hi Igor,
On 12/1/25 8:10 PM, Igor Mammedov wrote:
> On Thu, 27 Nov 2025 10:44:34 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
>> For one particular error (Error), we can't call error_setg() for twice.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I can't really parse that
> maybe rephrase it to make some sense?
>
I will drop this sentence in v2.
>> Otherwise, the assert(*errp == NULL) will be triggered unexpectedly in
>> error_setv(). In ghes_record_cper_errors(), get_ghes_source_offsets()
>> can return a error initialized by error_setg(). Without bailing on
>> this error, it can call into the second error_setg() due to the
>> unexpected value from the read acknowledgement register.
>>
>> Bail early in ghes_record_cper_errors() when error is received from
>> get_ghes_source_offsets() to avoid the exception.
>>
With above sentence dropped, the commit log improved to something like
below in v2.
In ghes_record_cper_errors(), get_ghes_source_offsets() can return
a error initialized by error_setg(). Without bailing on this error,
it can call into the second error_setg() due to the unexpected value
returned from the read acknowledgement register. The second error_setg()
can trigger assert(*errp == NULL) in its callee error_setv(), which
isn't expected.
Bail early in ghes_record_cper_errors() when error is received from
get_ghes_source_offsets() to avoid the unexpected behavior.
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> patch itself LGTM
> and with commit message fixed
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
>> ---
>> hw/acpi/ghes.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
>> index 6366c74248..c35883dfa9 100644
>> --- a/hw/acpi/ghes.c
>> +++ b/hw/acpi/ghes.c
>> @@ -443,7 +443,7 @@ static void get_hw_error_offsets(uint64_t ghes_addr,
>> *read_ack_register_addr = ghes_addr + sizeof(uint64_t);
>> }
>>
>> -static void get_ghes_source_offsets(uint16_t source_id,
>> +static bool get_ghes_source_offsets(uint16_t source_id,
>> uint64_t hest_addr,
>> uint64_t *cper_addr,
>> uint64_t *read_ack_start_addr,
>> @@ -474,7 +474,7 @@ static void get_ghes_source_offsets(uint16_t source_id,
>> /* For now, we only know the size of GHESv2 table */
>> if (type != ACPI_GHES_SOURCE_GENERIC_ERROR_V2) {
>> error_setg(errp, "HEST: type %d not supported.", type);
>> - return;
>> + return false;
>> }
>>
>> /* Compare CPER source ID at the GHESv2 structure */
>> @@ -488,7 +488,7 @@ static void get_ghes_source_offsets(uint16_t source_id,
>> }
>> if (i == num_sources) {
>> error_setg(errp, "HEST: Source %d not found.", source_id);
>> - return;
>> + return false;
>> }
>>
>> /* Navigate through table address pointers */
>> @@ -508,6 +508,8 @@ static void get_ghes_source_offsets(uint16_t source_id,
>> cpu_physical_memory_read(hest_read_ack_addr, read_ack_start_addr,
>> sizeof(*read_ack_start_addr));
>> *read_ack_start_addr = le64_to_cpu(*read_ack_start_addr);
>> +
>> + return true;
>> }
>>
>> NotifierList acpi_generic_error_notifiers =
>> @@ -526,9 +528,10 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len,
>> if (!ags->use_hest_addr) {
>> get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
>> &cper_addr, &read_ack_register_addr);
>> - } else {
>> - get_ghes_source_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
>> - &cper_addr, &read_ack_register_addr, errp);
>> + } else if (!get_ghes_source_offsets(source_id,
>> + le64_to_cpu(ags->hest_addr_le),
>> + &cper_addr, &read_ack_register_addr, errp)) {
>> + return;
>> }
>>
>> cpu_physical_memory_read(read_ack_register_addr,
>
Thanks,
Gavin
Gavin Shan <gshan@redhat.com> writes: > For one particular error (Error), we can't call error_setg() for twice. > Otherwise, the assert(*errp == NULL) will be triggered unexpectedly in > error_setv(). In ghes_record_cper_errors(), get_ghes_source_offsets() > can return a error initialized by error_setg(). Without bailing on > this error, it can call into the second error_setg() due to the > unexpected value from the read acknowledgement register. > > Bail early in ghes_record_cper_errors() when error is received from > get_ghes_source_offsets() to avoid the exception. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
© 2016 - 2026 Red Hat, Inc.