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.
> ---
> 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.
> - 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);
> }
>
> 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.
