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

[Gavin Shan]

Hi Igor,

On 12/1/25 8:10 PM, Igor Mammedov wrote:
On Thu, 27 Nov 2025 10:44:34 +1000
Gavin Shan <gs...@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 <gs...@redhat.com>
Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>

patch itself LGTM
and with commit message fixed
   Reviewed-by: Igor Mammedov <imamm...@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