Add a new ags flag to change the way HEST offsets are calculated.
Currently, offsets needed to store ACPI HEST offsets and read ack
are calculated based on a previous knowledge from the logic
which creates the HEST table.
Such logic is not generic, not allowing to easily add more HEST
entries nor replicates what OSPM does.
As the next patches will be adding a more generic logic, add a
new use_hest_addr, set to false, in preparation for such changes.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
hw/acpi/ghes.c | 46 ++++++++++++++++++++++++----------------
hw/arm/virt-acpi-build.c | 15 ++++++++++---
include/hw/acpi/ghes.h | 14 ++++++++++--
3 files changed, 52 insertions(+), 23 deletions(-)
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index b709c177cdea..e49a03fdb94e 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -206,7 +206,8 @@ 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(GArray *hardware_errors, BIOSLinker *linker)
+static void build_ghes_error_table(AcpiGhesState *ags, GArray *hardware_errors,
+ BIOSLinker *linker)
{
int i, error_status_block_offset;
@@ -251,13 +252,15 @@ static void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker)
i * ACPI_GHES_MAX_RAW_DATA_LENGTH);
}
- /*
- * tell firmware to write hardware_errors GPA into
- * hardware_errors_addr fw_cfg, once the former has been initialized.
- */
- bios_linker_loader_write_pointer(linker, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, 0,
- sizeof(uint64_t),
- ACPI_HW_ERROR_FW_CFG_FILE, 0);
+ if (!ags->use_hest_addr) {
+ /*
+ * Tell firmware to write hardware_errors GPA into
+ * hardware_errors_addr fw_cfg, once the former has been initialized.
+ */
+ bios_linker_loader_write_pointer(linker, ACPI_HW_ERROR_ADDR_FW_CFG_FILE,
+ 0, sizeof(uint64_t),
+ ACPI_HW_ERROR_FW_CFG_FILE, 0);
+ }
}
/* Build Generic Hardware Error Source version 2 (GHESv2) */
@@ -331,14 +334,15 @@ static void build_ghes_v2(GArray *table_data,
}
/* Build Hardware Error Source Table */
-void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
+void acpi_build_hest(AcpiGhesState *ags, GArray *table_data,
+ GArray *hardware_errors,
BIOSLinker *linker,
const char *oem_id, const char *oem_table_id)
{
AcpiTable table = { .sig = "HEST", .rev = 1,
.oem_id = oem_id, .oem_table_id = oem_table_id };
- build_ghes_error_table(hardware_errors, linker);
+ build_ghes_error_table(ags, hardware_errors, linker);
acpi_table_begin(&table, table_data);
@@ -357,11 +361,11 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
fw_cfg_add_file(s, ACPI_HW_ERROR_FW_CFG_FILE, hardware_error->data,
hardware_error->len);
- /* Create a read-write fw_cfg file for Address */
- fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, NULL,
- NULL, &(ags->hw_error_le), sizeof(ags->hw_error_le), false);
-
- ags->present = true;
+ if (!ags->use_hest_addr) {
+ /* Create a read-write fw_cfg file for Address */
+ fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, NULL,
+ NULL, &(ags->hw_error_le), sizeof(ags->hw_error_le), false);
+ }
}
static void get_hw_error_offsets(uint64_t ghes_addr,
@@ -411,8 +415,11 @@ void ghes_record_cper_errors(const void *cper, size_t len,
ags = &acpi_ged_state->ghes_state;
assert(ACPI_GHES_ERROR_SOURCE_COUNT == 1);
- get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
- &cper_addr, &read_ack_register_addr);
+
+ if (!ags->use_hest_addr) {
+ get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
+ &cper_addr, &read_ack_register_addr);
+ }
if (!cper_addr) {
error_setg(errp, "can not find Generic Error Status Block");
@@ -494,5 +501,8 @@ bool acpi_ghes_present(void)
return false;
}
ags = &acpi_ged_state->ghes_state;
- return ags->present;
+ if (!ags->hw_error_le)
+ return false;
+
+ return true;
}
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3ac8f8e17861..8ab8d11b6536 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -946,9 +946,18 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
build_dbg2(tables_blob, tables->linker, vms);
if (vms->ras) {
- acpi_add_table(table_offsets, tables_blob);
- acpi_build_hest(tables_blob, tables->hardware_errors, tables->linker,
- vms->oem_id, vms->oem_table_id);
+ AcpiGedState *acpi_ged_state;
+ AcpiGhesState *ags;
+
+ acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
+ NULL));
+ if (acpi_ged_state) {
+ ags = &acpi_ged_state->ghes_state;
+
+ acpi_add_table(table_offsets, tables_blob);
+ acpi_build_hest(ags, tables_blob, tables->hardware_errors,
+ tables->linker, vms->oem_id, vms->oem_table_id);
+ }
}
if (ms->numa_state->num_nodes > 0) {
diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
index 39619a2457cb..a3d62b96584f 100644
--- a/include/hw/acpi/ghes.h
+++ b/include/hw/acpi/ghes.h
@@ -64,12 +64,22 @@ enum {
ACPI_GHES_ERROR_SOURCE_COUNT
};
+/*
+ * AcpiGhesState stores an offset that will be used to fill HEST entries.
+ *
+ * When use_hest_addr is false, the stored offset is placed at hw_error_le,
+ * meaning an offset from the etc/hardware_errors firmware address. This
+ * is the default on QEMU 9.x.
+ *
+ * An offset value equal to zero means that GHES is not present.
+ */
typedef struct AcpiGhesState {
uint64_t hw_error_le;
- bool present; /* True if GHES is present at all on this board */
+ bool use_hest_addr; /* Currently, always false */
} AcpiGhesState;
-void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
+void acpi_build_hest(AcpiGhesState *ags, GArray *table_data,
+ GArray *hardware_errors,
BIOSLinker *linker,
const char *oem_id, const char *oem_table_id);
void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s,
--
2.48.1On Fri, 21 Feb 2025 15:35:10 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> Add a new ags flag to change the way HEST offsets are calculated.
> Currently, offsets needed to store ACPI HEST offsets and read ack
> are calculated based on a previous knowledge from the logic
> which creates the HEST table.
>
> Such logic is not generic, not allowing to easily add more HEST
> entries nor replicates what OSPM does.
>
> As the next patches will be adding a more generic logic, add a
> new use_hest_addr, set to false, in preparation for such changes.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> hw/acpi/ghes.c | 46 ++++++++++++++++++++++++----------------
> hw/arm/virt-acpi-build.c | 15 ++++++++++---
> include/hw/acpi/ghes.h | 14 ++++++++++--
> 3 files changed, 52 insertions(+), 23 deletions(-)
>
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index b709c177cdea..e49a03fdb94e 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -206,7 +206,8 @@ 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(GArray *hardware_errors, BIOSLinker *linker)
> +static void build_ghes_error_table(AcpiGhesState *ags, GArray *hardware_errors,
> + BIOSLinker *linker)
> {
> int i, error_status_block_offset;
>
> @@ -251,13 +252,15 @@ static void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker)
> i * ACPI_GHES_MAX_RAW_DATA_LENGTH);
> }
>
> - /*
> - * tell firmware to write hardware_errors GPA into
> - * hardware_errors_addr fw_cfg, once the former has been initialized.
> - */
> - bios_linker_loader_write_pointer(linker, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, 0,
> - sizeof(uint64_t),
> - ACPI_HW_ERROR_FW_CFG_FILE, 0);
> + if (!ags->use_hest_addr) {
> + /*
> + * Tell firmware to write hardware_errors GPA into
> + * hardware_errors_addr fw_cfg, once the former has been initialized.
> + */
> + bios_linker_loader_write_pointer(linker, ACPI_HW_ERROR_ADDR_FW_CFG_FILE,
> + 0, sizeof(uint64_t),
> + ACPI_HW_ERROR_FW_CFG_FILE, 0);
> + }
> }
>
> /* Build Generic Hardware Error Source version 2 (GHESv2) */
> @@ -331,14 +334,15 @@ static void build_ghes_v2(GArray *table_data,
> }
>
> /* Build Hardware Error Source Table */
> -void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
> +void acpi_build_hest(AcpiGhesState *ags, GArray *table_data,
> + GArray *hardware_errors,
> BIOSLinker *linker,
> const char *oem_id, const char *oem_table_id)
> {
> AcpiTable table = { .sig = "HEST", .rev = 1,
> .oem_id = oem_id, .oem_table_id = oem_table_id };
>
> - build_ghes_error_table(hardware_errors, linker);
> + build_ghes_error_table(ags, hardware_errors, linker);
>
> acpi_table_begin(&table, table_data);
>
> @@ -357,11 +361,11 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
> fw_cfg_add_file(s, ACPI_HW_ERROR_FW_CFG_FILE, hardware_error->data,
> hardware_error->len);
>
> - /* Create a read-write fw_cfg file for Address */
> - fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, NULL,
> - NULL, &(ags->hw_error_le), sizeof(ags->hw_error_le), false);
> -
> - ags->present = true;
> + if (!ags->use_hest_addr) {
> + /* Create a read-write fw_cfg file for Address */
> + fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, NULL,
> + NULL, &(ags->hw_error_le), sizeof(ags->hw_error_le), false);
> + }
> }
>
> static void get_hw_error_offsets(uint64_t ghes_addr,
> @@ -411,8 +415,11 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> ags = &acpi_ged_state->ghes_state;
>
> assert(ACPI_GHES_ERROR_SOURCE_COUNT == 1);
> - get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
> - &cper_addr, &read_ack_register_addr);
> +
> + if (!ags->use_hest_addr) {
> + get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
> + &cper_addr, &read_ack_register_addr);
> + }
>
> if (!cper_addr) {
> error_setg(errp, "can not find Generic Error Status Block");
> @@ -494,5 +501,8 @@ bool acpi_ghes_present(void)
> return false;
> }
> ags = &acpi_ged_state->ghes_state;
> - return ags->present;
> + if (!ags->hw_error_le)
> + return false;
> +
> + return true;
> }
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 3ac8f8e17861..8ab8d11b6536 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -946,9 +946,18 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> build_dbg2(tables_blob, tables->linker, vms);
>
> if (vms->ras) {
> - acpi_add_table(table_offsets, tables_blob);
> - acpi_build_hest(tables_blob, tables->hardware_errors, tables->linker,
> - vms->oem_id, vms->oem_table_id);
> + AcpiGedState *acpi_ged_state;
> + AcpiGhesState *ags;
> +
> + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
^^^ will explode if object_resolve_path_type() returns NULL
> + NULL));
it's also expensive load-wise.
You have access to vms with ged pointer here, use that
(search for 'acpi_ged_state = ACPI_GED' example)
> + if (acpi_ged_state) {
hence, this check is not really needed,
we have to have GED at this point or abort
earlier code that instantiates GED should take care of
cleanly exiting if it failed to create GED so we would never get
to missing GED here
> + ags = &acpi_ged_state->ghes_state;
> +
> + acpi_add_table(table_offsets, tables_blob);
> + acpi_build_hest(ags, tables_blob, tables->hardware_errors,
> + tables->linker, vms->oem_id, vms->oem_table_id);
> + }
> }
>
> if (ms->numa_state->num_nodes > 0) {
> diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
> index 39619a2457cb..a3d62b96584f 100644
> --- a/include/hw/acpi/ghes.h
> +++ b/include/hw/acpi/ghes.h
> @@ -64,12 +64,22 @@ enum {
> ACPI_GHES_ERROR_SOURCE_COUNT
> };
>
> +/*
> + * AcpiGhesState stores an offset that will be used to fill HEST entries.
> + *
> + * When use_hest_addr is false, the stored offset is placed at hw_error_le,
> + * meaning an offset from the etc/hardware_errors firmware address. This
> + * is the default on QEMU 9.x.
> + *
> + * An offset value equal to zero means that GHES is not present.
> + */
> typedef struct AcpiGhesState {
> uint64_t hw_error_le;
> - bool present; /* True if GHES is present at all on this board */
> + bool use_hest_addr; /* Currently, always false */
> } AcpiGhesState;
>
> -void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
> +void acpi_build_hest(AcpiGhesState *ags, GArray *table_data,
> + GArray *hardware_errors,
> BIOSLinker *linker,
> const char *oem_id, const char *oem_table_id);
> void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s,
Em Wed, 26 Feb 2025 15:37:14 +0100
Igor Mammedov <imammedo@redhat.com> escreveu:
> On Fri, 21 Feb 2025 15:35:10 +0100
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
>
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 3ac8f8e17861..8ab8d11b6536 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -946,9 +946,18 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> > build_dbg2(tables_blob, tables->linker, vms);
> >
> > if (vms->ras) {
> > - acpi_add_table(table_offsets, tables_blob);
> > - acpi_build_hest(tables_blob, tables->hardware_errors, tables->linker,
> > - vms->oem_id, vms->oem_table_id);
> > + AcpiGedState *acpi_ged_state;
> > + AcpiGhesState *ags;
> > +
> > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
> ^^^ will explode if object_resolve_path_type() returns NULL
> > + NULL));
>
> it's also expensive load-wise.
> You have access to vms with ged pointer here, use that
> (search for 'acpi_ged_state = ACPI_GED' example)
Ok, but the state binding on ghes were designed to use ACPI_GED. I moved
the code that it is using ACPI_GED() to the beginning of v5 series,
just after the HEST table test addition.
With that, ACPI_GED() is now used only on two places inside ghes:
- at virt_acpi_build(), during VM initialization;
- at acpi_ghes_get_state().
If you want to replace it by some other solution, IMO we should do
it on some separate series, as this is not related to neither error
injection nor with offset calculation to get read ack address.
> > + if (acpi_ged_state) {
>
> hence, this check is not really needed,
> we have to have GED at this point or abort
>
> earlier code that instantiates GED should take care of
> cleanly exiting if it failed to create GED so we would never get
> to missing GED here
I dropped this check on v5.
Thanks,
Mauro
On Thu, 27 Feb 2025 12:45:38 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> Em Wed, 26 Feb 2025 15:37:14 +0100
> Igor Mammedov <imammedo@redhat.com> escreveu:
>
> > On Fri, 21 Feb 2025 15:35:10 +0100
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >
>
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 3ac8f8e17861..8ab8d11b6536 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -946,9 +946,18 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> > > build_dbg2(tables_blob, tables->linker, vms);
> > >
> > > if (vms->ras) {
> > > - acpi_add_table(table_offsets, tables_blob);
> > > - acpi_build_hest(tables_blob, tables->hardware_errors, tables->linker,
> > > - vms->oem_id, vms->oem_table_id);
> > > + AcpiGedState *acpi_ged_state;
> > > + AcpiGhesState *ags;
> > > +
> > > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
> > ^^^ will explode if object_resolve_path_type() returns NULL
> > > + NULL));
> >
> > it's also expensive load-wise.
> > You have access to vms with ged pointer here, use that
> > (search for 'acpi_ged_state = ACPI_GED' example)
>
> Ok, but the state binding on ghes were designed to use ACPI_GED. I moved
> the code that it is using ACPI_GED() to the beginning of v5 series,
> just after the HEST table test addition.
>
> With that, ACPI_GED() is now used only on two places inside ghes:
>
> - at virt_acpi_build(), during VM initialization;
ACPI_GED() is not expensive, what I'm referring to is
object_resolve_path_type()
given it's a new code and virt_acpi_build() has direct access
to ged pointer, there is no excuse to use object_resolve_path_type().
all you have to do here is:
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index e6328af5d2..040d875d4e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -949,8 +949,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
AcpiGedState *acpi_ged_state;
AcpiGhesState *ags;
- acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
- NULL));
+ acpi_ged_state = ACPI_GED(vms->acpi_dev);
ags = &acpi_ged_state->ghes_state;
if (ags) {
acpi_add_table(table_offsets, tables_blob);
> - at acpi_ghes_get_state().
this one is different, it doesn't have access to ged so it
has to look up for it.
>
> If you want to replace it by some other solution, IMO we should do
> it on some separate series, as this is not related to neither error
> injection nor with offset calculation to get read ack address.
>
> > > + if (acpi_ged_state) {
> >
> > hence, this check is not really needed,
> > we have to have GED at this point or abort
> >
> > earlier code that instantiates GED should take care of
> > cleanly exiting if it failed to create GED so we would never get
> > to missing GED here
>
> I dropped this check on v5.
>
> Thanks,
> Mauro
>
© 2016 - 2025 Red Hat, Inc.