Re: [PATCH v4 7/8] acpi/ghes: Use error_fatal in acpi_ghes_memory_errors()

[Gavin Shan <gshan@redhat.com>]

Hi Markus,

On 11/13/25 5:41 PM, Markus Armbruster wrote:
> Gavin Shan <gshan@redhat.com> 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 <imammedo@redhat.com>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> 
> 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