1. Patchew
  2. QEMU
  3. [PATCH v3 0/8] target/arm/kvm: Improve memory error handling
  4. View patch

[PATCH v3 5/8] acpi/ghes: Bail early on error from get_ghes_source_offsets()

Gavin Shan posted 8 patches 1 week, 2 days ago
Maintainers: Dongjiu Geng <gengdongjiu1@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>
[PATCH v3 5/8] acpi/ghes: Bail early on error from get_ghes_source_offsets()
Posted by Gavin Shan 1 week, 2 days ago 
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>
---
 hw/acpi/ghes.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 527b85c8d8..055e5d719a 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -513,6 +513,9 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len,
     } else {
         get_ghes_source_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
                                 &cper_addr, &read_ack_register_addr, errp);
+        if (*errp) {
+            return;
+        }
     }
 
     cpu_physical_memory_read(read_ack_register_addr,
-- 
2.51.0
Re: [PATCH v3 5/8] acpi/ghes: Bail early on error from get_ghes_source_offsets()
Posted by Philippe Mathieu-Daudé 4 days ago 
On 5/11/25 12:44, Gavin Shan wrote:
> 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>
> ---
>   hw/acpi/ghes.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 527b85c8d8..055e5d719a 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -513,6 +513,9 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len,
>       } else {
>           get_ghes_source_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
>                                   &cper_addr, &read_ack_register_addr, errp);
> +        if (*errp) {
> +            return;
> +        }

If get_ghes_source_offsets() can fail, then lets have it return a
boolean.

   if (!get_ghes_source_offsets(..., errp)) {
       return;
   }

See commit e3fe3988d78 ("error: Document Error API usage rules").
Re: [PATCH v3 5/8] acpi/ghes: Bail early on error from get_ghes_source_offsets()
Posted by Gavin Shan 3 days, 11 hours ago 
Hi Philippe,

On 11/11/25 12:50 AM, Philippe Mathieu-Daudé wrote:
> On 5/11/25 12:44, Gavin Shan wrote:
>> 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>
>> ---
>>   hw/acpi/ghes.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
>> index 527b85c8d8..055e5d719a 100644
>> --- a/hw/acpi/ghes.c
>> +++ b/hw/acpi/ghes.c
>> @@ -513,6 +513,9 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len,
>>       } else {
>>           get_ghes_source_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
>>                                   &cper_addr, &read_ack_register_addr, errp);
>> +        if (*errp) {
>> +            return;
>> +        }
> 
> If get_ghes_source_offsets() can fail, then lets have it return a
> boolean.
> 
>    if (!get_ghes_source_offsets(..., errp)) {
>        return;
>    }
> 
> See commit e3fe3988d78 ("error: Document Error API usage rules").
> 

Fair point. The caller ghes_record_cper_errors() shouldn't check '*errp' according
to commit e3fe3988d78. So we need get_ghes_source_offsets() to return false on
errors. It will be improved in next revision.

Thanks,
Gavin
Re: [PATCH v3 5/8] acpi/ghes: Bail early on error from get_ghes_source_offsets()
Posted by Igor Mammedov 4 days ago 
On Wed,  5 Nov 2025 21:44:50 +1000
Gavin Shan <gshan@redhat.com> wrote:

> 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: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/ghes.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 527b85c8d8..055e5d719a 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -513,6 +513,9 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len,
>      } else {
>          get_ghes_source_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
>                                  &cper_addr, &read_ack_register_addr, errp);
> +        if (*errp) {
> +            return;
> +        }
>      }
>  
>      cpu_physical_memory_read(read_ack_register_addr,
Re: [PATCH v3 5/8] acpi/ghes: Bail early on error from get_ghes_source_offsets()
Posted by Jonathan Cameron via 1 week, 2 days ago 
On Wed,  5 Nov 2025 21:44:50 +1000
Gavin Shan <gshan@redhat.com> wrote:

> 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>
Seems reasonable to me.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.