On Wed, 22 Jan 2025 16:46:24 +0100
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> Move the check logic into a common function and simplify the
> code which checks if GHES is enabled and was properly setup.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> ---
> hw/acpi/ghes-stub.c | 4 ++--
> hw/acpi/ghes.c | 33 +++++++++++----------------------
> include/hw/acpi/ghes.h | 9 +++++----
> target/arm/kvm.c | 2 +-
> 4 files changed, 19 insertions(+), 29 deletions(-)
>
> diff --git a/hw/acpi/ghes-stub.c b/hw/acpi/ghes-stub.c
> index 7cec1812dad9..fbabf955155a 100644
> --- a/hw/acpi/ghes-stub.c
> +++ b/hw/acpi/ghes-stub.c
> @@ -16,7 +16,7 @@ int acpi_ghes_memory_errors(uint16_t source_id, uint64_t physical_address)
> return -1;
> }
>
> -bool acpi_ghes_present(void)
> +AcpiGhesState *acpi_ghes_get_state(void)
> {
> - return false;
> + return NULL;
> }
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 961fc38ea8f5..5d29db3918dd 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -420,10 +420,6 @@ static void get_hw_error_offsets(uint64_t ghes_addr,
> uint64_t *cper_addr,
> uint64_t *read_ack_register_addr)
> {
> - if (!ghes_addr) {
> - return;
> - }
> -
> /*
> * non-HEST version supports only one source, so no need to change
> * the start offset based on the source ID. Also, we can't validate
> @@ -451,10 +447,6 @@ static void get_ghes_source_offsets(uint16_t source_id, uint64_t hest_addr,
> uint64_t err_source_struct, error_block_addr;
> uint32_t num_sources, i;
>
> - if (!hest_addr) {
> - return;
> - }
> -
> cpu_physical_memory_read(hest_addr, &num_sources, sizeof(num_sources));
> num_sources = le32_to_cpu(num_sources);
>
> @@ -513,7 +505,6 @@ void ghes_record_cper_errors(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;
> - AcpiGedState *acpi_ged_state;
> AcpiGhesState *ags;
>
> if (len > ACPI_GHES_MAX_RAW_DATA_LENGTH) {
> @@ -521,13 +512,10 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> return;
> }
>
> - acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
> - NULL));
> - if (!acpi_ged_state) {
> - error_setg(errp, "Can't find ACPI_GED object");
> + ags = acpi_ghes_get_state();
1)
> + if (!ags) {
> return;
> }
> - ags = &acpi_ged_state->ghes_state;
>
> if (!ags->hest_lookup) {
> get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
> @@ -537,11 +525,6 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> &cper_addr, &read_ack_register_addr, errp);
> }
>
> - if (!cper_addr) {
> - error_setg(errp, "can not find Generic Error Status Block");
> - return;
> - }
> -
> cpu_physical_memory_read(read_ack_register_addr,
> &read_ack_register, sizeof(read_ack_register));
>
> @@ -605,7 +588,7 @@ int acpi_ghes_memory_errors(uint16_t source_id, uint64_t physical_address)
> return 0;
> }
>
> -bool acpi_ghes_present(void)
> +AcpiGhesState *acpi_ghes_get_state(void)
> {
> AcpiGedState *acpi_ged_state;
> AcpiGhesState *ags;
> @@ -614,8 +597,14 @@ bool acpi_ghes_present(void)
> NULL));
>
> if (!acpi_ged_state) {
> - return false;
> + return NULL;
> }
> ags = &acpi_ged_state->ghes_state;
> - return ags->present;
> + if (!ags->present) {
> + return NULL;
> + }
redundant check, check below vvvv should be sufficient
> + if (!ags->hw_error_le && !ags->hest_addr_le) {
> + return NULL;
> + }
> + return ags;
> }
> diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
> index 2e8405edfe27..64fe2b5bea65 100644
> --- a/include/hw/acpi/ghes.h
> +++ b/include/hw/acpi/ghes.h
> @@ -91,10 +91,11 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> uint16_t source_id, Error **errp);
>
> /**
> - * acpi_ghes_present: Report whether ACPI GHES table is present
> + * acpi_ghes_get_state: Get a pointer for ACPI ghes state
> *
> - * Returns: true if the system has an ACPI GHES table and it is
> - * safe to call acpi_ghes_memory_errors() to record a memory error.
> + * Returns: a pointer to ghes state if the system has an ACPI GHES table,
> + * it is enabled and it is safe to call acpi_ghes_memory_errors() to record
> + * a memory error. Returns false, otherwise.
> */
> -bool acpi_ghes_present(void);
> +AcpiGhesState *acpi_ghes_get_state(void);
> #endif
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index da30bdbb2349..0283089713b9 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -2369,7 +2369,7 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
>
> assert(code == BUS_MCEERR_AR || code == BUS_MCEERR_AO);
>
> - if (acpi_ghes_present() && addr) {
> + if (acpi_ghes_get_state() && addr) {
double lookup, 1sh here and then in [1],
suggest store state here and pass it as an argument to down the call chain
(i.e. to acpi_ghes_memory_errors() and below)
> ram_addr = qemu_ram_addr_from_host(addr);
> if (ram_addr != RAM_ADDR_INVALID &&
> kvm_physical_memory_addr_from_host(c->kvm_state, addr, &paddr)) {
