Re: [PATCH v3 6/8] acpi/ghes: Use error_abort in acpi_ghes_memory_errors()

[Gavin Shan <gshan@redhat.com>]

Hi Igor,

On 11/12/25 10:49 PM, Igor Mammedov wrote:
> On Tue, 11 Nov 2025 13:58:46 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>>
>> 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 <imammedo@redhat.com>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>>    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)
> 

error_abort has been replaced by error_fatal as Markus suggested in another
reply.

>>>> +                kvm_inject_arm_sea(c);
>>>>                }
>>>>                return;
>>>>            }
>>>    

Thanks,
Gavin