Hi Markus, On 11/13/25 5:41 PM, Markus Armbruster wrote:
Gavin Shan <[email protected]> writes:Use error_fatal in acpi_ghes_memory_errors() so that the caller needn't explicitly terminate on errors. With error_fatal, a qemu core dump won't be provided as it doesn't provide anything needed by debugging. There is no way to call ghes-stu.c::acpi_ghes_memory_errors(), an abort() is put there as explicit marker. Besides, the return value of acpi_ghes_memory_errors() is changed from 'int' to 'bool' as the error indicator. ghes_record_cper_errors() also return a 'bool' value for that, to be compatible to what is documented in error.h. Suggested-by: Igor Mammedov <[email protected]> Suggested-by: Markus Armbruster <[email protected]> Signed-off-by: Gavin Shan <[email protected]> Reviewed-by: Jonathan Cameron <[email protected]>This commit does a number of things: 1. Change abort() to exit() on error, and drop the extra error message "failed to record the error". 2. Use &error_fatal to separate concerns and simplify the caller. This item covers both the new Error ** parameter and the returning bool instead of void. 3. Make the unreachable stub abort(). The commit message could use polish to make the three distinct things more clear. In general, patches that do just one thing are easier to understand and describe (in the commit message) than patches that do multiple things. That said, a single commit feels okay in this case. Up to you.
Yes, it would be nice to do one thing per patch. I will split this into 3 patches as you suggested in next revision. I would wait a while to see if Igor has more comments prior to next revision.
--- hw/acpi/ghes-stub.c | 7 ++++--- hw/acpi/ghes.c | 27 +++++++++++++-------------- include/hw/acpi/ghes.h | 7 ++++--- target/arm/kvm.c | 10 +++------- 4 files changed, 24 insertions(+), 27 deletions(-) diff --git a/hw/acpi/ghes-stub.c b/hw/acpi/ghes-stub.c index 4faf573aeb..fc7374b0a6 100644 --- a/hw/acpi/ghes-stub.c +++ b/hw/acpi/ghes-stub.c @@ -11,10 +11,11 @@ #include "qemu/osdep.h" #include "hw/acpi/ghes.h"-int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id,- uint64_t *addresses, uint32_t num_of_addresses) +bool acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id, + uint64_t *addresses, uint32_t num_of_addresses, + Error **errp) { - return -1; + abort(); }AcpiGhesState *acpi_ghes_get_state(void)diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c index d3d6c11197..7160cf37d0 100644 --- a/hw/acpi/ghes.c +++ b/hw/acpi/ghes.c @@ -508,14 +508,14 @@ static bool get_ghes_source_offsets(uint16_t source_id, NotifierList acpi_generic_error_notifiers = NOTIFIER_LIST_INITIALIZER(acpi_generic_error_notifiers);-void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len,+bool ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len, uint16_t source_id, Error **errp) { uint64_t cper_addr = 0, read_ack_register_addr = 0, read_ack_register;if (len > ghes_max_raw_data_length(ags)) {error_setg(errp, "GHES CPER record is too big: %zd", len); - return; + return false; }if (!ags->use_hest_addr) {@@ -524,7 +524,7 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len, } else if (!get_ghes_source_offsets(source_id, le64_to_cpu(ags->hest_addr_le), &cper_addr, &read_ack_register_addr, errp)) { - return; + return false; }cpu_physical_memory_read(read_ack_register_addr,@@ -535,7 +535,7 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len, error_setg(errp, "OSPM does not acknowledge previous error," " so can not record CPER for current error anymore"); - return; + return false; }read_ack_register = cpu_to_le64(0);@@ -550,10 +550,13 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len, cpu_physical_memory_write(cper_addr, cper, len);notifier_list_notify(&acpi_generic_error_notifiers, &source_id);+ + return true; }-int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id,- uint64_t *addresses, uint32_t num_of_addresses) +bool acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id, + uint64_t *addresses, uint32_t num_of_addresses, + Error **errp) { /* Memory Error Section Type */ const uint8_t guid[] = @@ -564,10 +567,10 @@ int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id, * Table 17-13 Generic Error Data Entry */ QemuUUID fru_id = {}; - Error *errp = NULL; int data_length; GArray *block; uint32_t block_status = 0, i; + bool ret;block = g_array_new(false, true /* clear */, 1); @@ -605,16 +608,12 @@ int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id,}/* Report the error */This comment is now stale. I'd simply drop it.
Indeed, thanks.
- ghes_record_cper_errors(ags, block->data, block->len, source_id, &errp); + ret = ghes_record_cper_errors(ags, block->data, block->len, + source_id, errp);g_array_free(block, true); - if (errp) {- error_report_err(errp); - return -1; - } - - return 0; + return ret;I figure you could use g_autoptr() to simplify this further. Something along the lines of g_autoptr(GArray) block = g_array_new(false, true, 1); [...] return ghes_record_cper_errors(ags, block->data, block->len, source_id, errp);
Yes. It seems the pattern g_autoptr(GArray) isn't widely used in QEMU yet. I will have one separate patch for this before the patches improving the error (Error instead of memory error) handling in next revision.
}AcpiGhesState *acpi_ghes_get_state(void)diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h index f7b084c039..c1f01ac25c 100644 --- a/include/hw/acpi/ghes.h +++ b/include/hw/acpi/ghes.h @@ -99,9 +99,10 @@ void acpi_build_hest(AcpiGhesState *ags, GArray *table_data, const char *oem_id, const char *oem_table_id); void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s, GArray *hardware_errors); -int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id, - uint64_t *addresses, uint32_t num_of_addresses); -void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len, +bool acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id, + uint64_t *addresses, uint32_t num_of_addresses, + Error **errp); +bool ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t len, uint16_t source_id, Error **errp);/**diff --git a/target/arm/kvm.c b/target/arm/kvm.c index 459ca4a9b0..b8c3ad2ad9 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -2458,13 +2458,9 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr) addresses[0] = paddr; if (code == BUS_MCEERR_AR) { kvm_cpu_synchronize_state(c); - if (!acpi_ghes_memory_errors(ags, ACPI_HEST_SRC_ID_SYNC, - addresses, 1)) { - kvm_inject_arm_sea(c); - } else { - error_report("failed to record the error"); - abort(); - } + acpi_ghes_memory_errors(ags, ACPI_HEST_SRC_ID_SYNC, + addresses, 1, &error_fatal); + kvm_inject_arm_sea(c); } return; }Readability improves nicely here.
Yes :-) Thanks, Gavin
