On Tue, 11 Nov 2025 13:58:46 +1000 Gavin Shan <[email protected]> wrote:
> Hi Philippe, > > On 11/11/25 12:54 AM, Philippe Mathieu-Daudé wrote: > > On 5/11/25 12:44, Gavin Shan wrote: > >> Use error_abort in acpi_ghes_memory_errors() so that the caller needn't > >> explicitly call abort() on errors. With this change, its return value > >> isn't needed any more. > >> > >> Suggested-by: Igor Mammedov <[email protected]> > >> Signed-off-by: Gavin Shan <[email protected]> > >> --- > >> hw/acpi/ghes-stub.c | 6 +++--- > >> hw/acpi/ghes.c | 15 ++++----------- > >> include/hw/acpi/ghes.h | 5 +++-- > >> target/arm/kvm.c | 10 +++------- > >> 4 files changed, 13 insertions(+), 23 deletions(-) > > > > > >> diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h > >> index f73908985d..35c7bbbb01 100644 > >> --- a/include/hw/acpi/ghes.h > >> +++ b/include/hw/acpi/ghes.h > >> @@ -98,8 +98,9 @@ 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 acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id, > >> + uint64_t *addresses, uint32_t > >> num_of_addresses, > >> + Error **errp); > > > > This is an anti-pattern w.r.t. commit e3fe3988d78 ("error: Document > > Error API usage rules"), because it can be called with an errp distinct > > of &error_abort. > > > > If you really want to abort(), remove the errp argument, directly call > > abort() and rename as acpi_ghes_memory_abort_on_errors(). > > > > Thanks for pointing it out. I will improve this like below in next revision. > > - Drop 'errp' argument from acpi_ghes_memory_errors(), but I prefer to keep > the function name. > > - In acpi_ghes_memory_errors(), a local variable 'Error *err' is added and > pass it to ghes_record_cper_errors(), which is also called by QMP handler > qmp_inject_ghes_v2_error(). > > Please let me know if there are any more improvements needed. > > >> void 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..a889315606 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_abort); I guess that was my request, with calling abort() directly you have to print error separately, while using 'error_abort' is much more cleaner (as this hunk demonstrates) > >> + kvm_inject_arm_sea(c); > >> } > >> return; > >> } > > > > Thanks, > Gavin >
