On Tue, 4 Mar 2025 14:30:56 +0100
Mauro Carvalho Chehab <mchehab+hua...@kernel.org> wrote:
> The only reason why we're passing ags to acpi HEST table build
> is to check if migration will be used or not.
>
> Well, we only need migration for arm, as other architectures
> will only use the new code. So, move this out of acpi_build_hest(),
> as otherwise we can't use it for x86, as the hotplug logic there
> may not initialize ags during acpi table build time.
can you expand on what hotplug logic you are talking about?
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+hua...@kernel.org>
> ---
> hw/acpi/ghes.c | 16 ++++++++--------
> hw/arm/virt-acpi-build.c | 12 ++++++++----
> include/hw/acpi/ghes.h | 25 +++++++++++++------------
> 3 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 1fd5ba941771..ea00fed75c16 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -243,7 +243,7 @@ ghes_gen_err_data_uncorrectable_recoverable(GArray *block,
> * Initialize "etc/hardware_errors" and "etc/hardware_errors_addr" fw_cfg
> blobs.
> * See docs/specs/acpi_hest_ghes.rst for blobs format.
> */
> -static void build_ghes_error_table(AcpiGhesState *ags, GArray
> *hardware_errors,
> +static void build_ghes_error_table(GArray *hardware_errors, bool
> use_hest_addr,
> BIOSLinker *linker, int num_sources)
> {
> int i, error_status_block_offset;
> @@ -289,7 +289,7 @@ static void build_ghes_error_table(AcpiGhesState *ags,
> GArray *hardware_errors,
> i * ACPI_GHES_MAX_RAW_DATA_LENGTH);
> }
>
> - if (!ags->use_hest_addr) {
> + if (!use_hest_addr) {
> /*
> * Tell firmware to write hardware_errors GPA into
> * hardware_errors_addr fw_cfg, once the former has been initialized.
> @@ -372,7 +372,7 @@ static void build_ghes_v2_entry(GArray *table_data,
> }
>
> /* Build Hardware Error Source Table */
> -void acpi_build_hest(AcpiGhesState *ags, GArray *table_data,
> +void acpi_build_hest(GArray *table_data, bool use_hest_addr,
> GArray *hardware_errors,
> BIOSLinker *linker,
> const AcpiNotificationSourceId *notif_source,
> @@ -386,7 +386,7 @@ void acpi_build_hest(AcpiGhesState *ags, GArray
> *table_data,
>
> hest_offset = table_data->len;
>
> - build_ghes_error_table(ags, hardware_errors, linker, num_sources);
> + build_ghes_error_table(hardware_errors, use_hest_addr, linker,
> num_sources);
>
> acpi_table_begin(&table, table_data);
>
> @@ -398,7 +398,7 @@ void acpi_build_hest(AcpiGhesState *ags, GArray
> *table_data,
>
> acpi_table_end(linker, &table);
>
> - if (ags->use_hest_addr) {
> + if (use_hest_addr) {
> /*
> * Tell firmware to write into GPA the address of HEST via fw_cfg,
> * once initialized.
> @@ -411,13 +411,13 @@ void acpi_build_hest(AcpiGhesState *ags, GArray
> *table_data,
> }
>
> void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
> - GArray *hardware_error)
> + bool use_hest_addr, GArray *hardware_error)
> {
> /* Create a read-only fw_cfg file for GHES */
> fw_cfg_add_file(s, ACPI_HW_ERROR_FW_CFG_FILE, hardware_error->data,
> hardware_error->len);
>
> - if (ags->use_hest_addr) {
> + if (use_hest_addr) {
> fw_cfg_add_file_callback(s, ACPI_HEST_ADDR_FW_CFG_FILE, NULL, NULL,
> NULL, &(ags->hest_addr_le), sizeof(ags->hest_addr_le), false);
> } else {
> @@ -529,7 +529,7 @@ void ghes_record_cper_errors(AcpiGhesState *ags, const
> void *cper, size_t len,
> return;
> }
>
> - if (!ags->use_hest_addr) {
> + if (ags->hw_error_le) {
> get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
> &cper_addr, &read_ack_register_addr);
> } else {
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 154337e1a77b..71da17b652b2 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -964,9 +964,11 @@ void virt_acpi_build(VirtMachineState *vms,
> AcpiBuildTables *tables)
> acpi_ged_state = ACPI_GED(vms->acpi_dev);
> ags = &acpi_ged_state->ghes_state;
> if (ags) {
> + bool use_hest_addr = ags->use_hest_addr;
> +
> acpi_add_table(table_offsets, tables_blob);
>
> - if (!ags->use_hest_addr) {
> + if (!use_hest_addr) {
> notify = hest_ghes_notify_9_2;
> notify_sz = ARRAY_SIZE(hest_ghes_notify_9_2);
> } else {
> @@ -974,7 +976,7 @@ void virt_acpi_build(VirtMachineState *vms,
> AcpiBuildTables *tables)
> notify_sz = ARRAY_SIZE(hest_ghes_notify);
> }
>
> - acpi_build_hest(ags, tables_blob, tables->hardware_errors,
> + acpi_build_hest(tables_blob, use_hest_addr,
> tables->hardware_errors,
> tables->linker, notify, notify_sz,
> vms->oem_id, vms->oem_table_id);
> }
> @@ -1143,8 +1145,10 @@ void virt_acpi_setup(VirtMachineState *vms)
> if (vms->ras) {
> assert(vms->acpi_dev);
> acpi_ged_state = ACPI_GED(vms->acpi_dev);
> - acpi_ghes_add_fw_cfg(&acpi_ged_state->ghes_state,
> - vms->fw_cfg, tables.hardware_errors);
> + AcpiGhesState *ags = &acpi_ged_state->ghes_state;
> +
> + acpi_ghes_add_fw_cfg(ags, vms->fw_cfg, ags->use_hest_addr,
> + tables.hardware_errors);
> }
>
> build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update,
> diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
> index df2ecbf6e4a9..eae6d4d0a562 100644
> --- a/include/hw/acpi/ghes.h
> +++ b/include/hw/acpi/ghes.h
> @@ -73,31 +73,32 @@ typedef struct AcpiNotificationSourceId {
> enum AcpiGhesNotifyType notify;
> } AcpiNotificationSourceId;
>
> -/*
> - * AcpiGhesState stores GPA values that will be used to fill HEST entries.
> +/**
> + * struct AcpiGhesState - GPA values that will be used to fill HEST entries
> *
> - * When use_hest_addr is false, the GPA of the etc/hardware_errors firmware
> - * is stored at hw_error_le. This is the default on QEMU 9.x.
> + * @hest_addr_le: GPA of the HEST table. Used on QEMU 10.x and above.
> + * @hw_error_le: GPA of the etc/hardware_errors firmware.
> + * Used only on arm64 virt-9.x to preserve compatibility
> + * with QEMU 9.x.
> + * @use_hest_addr: True if HEST address is present. Used only on arm64,
> + * to identify if QEMU 9.x migration is needed.
> *
> - * When use_hest_addr is true, the GPA of the HEST table is stored at
> - * hest_addr_le. This is the default for QEMU 10.x and above.
> - *
> - * Whe both GPA values are equal to zero means that GHES is not present.
> + * When both GPA values are equal to zero means that GHES is not present.
> */
> typedef struct AcpiGhesState {
> uint64_t hest_addr_le;
> uint64_t hw_error_le;
> - bool use_hest_addr; /* True if HEST address is present */
> + bool use_hest_addr;
> } AcpiGhesState;
>
> -void acpi_build_hest(AcpiGhesState *ags, GArray *table_data,
> +void acpi_build_hest(GArray *table_data, bool use_hest_addr,
> GArray *hardware_errors,
> BIOSLinker *linker,
> const AcpiNotificationSourceId * const notif_source,
> int num_sources,
> const char *oem_id, const char *oem_table_id);
> -void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s,
> - GArray *hardware_errors);
> +void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
> + bool use_hest_addr, GArray *hardware_error);
> int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id,
> uint64_t error_physical_addr);
> void ghes_record_cper_errors(AcpiGhesState *ags, const void *cper, size_t
> len,
