[PATCH RFC 0/5] Change ghes driver to use HEST-based offsets

[PATCH RFC 0/5] Change ghes driver to use HEST-based offsets

[Mauro Carvalho Chehab]

This RFC series was part of the previous PR to add generic error injection
support on GHES.

It contains only the changes of the math used to calculate offsets at
HEST table and hardware_error firmware file.

The first patch adds a new firmware file to store HEST address.
The second patch makes use of it.
The remaining ones add migration support.

PS.: I'm sending this as a RFC as using the proceudure defined at the
pseudo-migration of:

	https://www.linux-kvm.org/page/Migration

Didn't work. I tried to use two different QEMU versions to check a
real life case and also to use just one QEMU and trying to load a
virt-9.1 state on a virt-9.2 machine. 

For instance, trying to restore a virt-9.1 state on virt-9.2 gave me
this error:

	(qemu) qemu: Machine type received is 'virt-9.1' and local is 'virt-9.2'
	qemu: load of migration failed: Invalid argument

Yet, running virt-9.1 used the old math code (offsets from hardware_errors firmware
file) while running virt-9.2 executed the new math code using HEST address.

Mauro Carvalho Chehab (5):
  acpi/ghes: add a firmware file with HEST address
  acpi/ghes: Use HEST table offsets when preparing GHES records
  acpi/generic_event_device: Update GHES migration to cover hest addr
  acpi/generic_event_device: add logic to detect if HEST addr is
    available
  arm/virt-acpi-build: Properly handle virt-9.1

 hw/acpi/generic_event_device.c |  30 +++++++++
 hw/acpi/ghes.c                 | 108 ++++++++++++++++++++++++++++++---
 hw/arm/virt-acpi-build.c       |  30 +++++++--
 hw/core/machine.c              |   4 +-
 include/hw/acpi/ghes.h         |   2 +
 5 files changed, 159 insertions(+), 15 deletions(-)

-- 
2.46.1

[PATCH RFC 1/5] acpi/ghes: add a firmware file with HEST address

[Mauro Carvalho Chehab]

Store HEST table address at GPA, placing its content at
hest_addr_le variable.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

---

Change from v8:
- hest_addr_lr is now pointing to the error source size and data.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 hw/acpi/ghes.c         | 15 +++++++++++++++
 include/hw/acpi/ghes.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 8b3292be07e7..2c2cf444edeb 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -30,6 +30,7 @@
 
 #define ACPI_HW_ERROR_FW_CFG_FILE           "etc/hardware_errors"
 #define ACPI_HW_ERROR_ADDR_FW_CFG_FILE      "etc/hardware_errors_addr"
+#define ACPI_HEST_ADDR_FW_CFG_FILE          "etc/acpi_table_hest_addr"
 
 /* The max size in bytes for one error block */
 #define ACPI_GHES_MAX_RAW_DATA_LENGTH   (1 * KiB)
@@ -361,6 +362,8 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
 
     acpi_table_begin(&table, table_data);
 
+    int hest_offset = table_data->len;
+
     /* Error Source Count */
     build_append_int_noprefix(table_data, num_sources, 4);
     for (i = 0; i < num_sources; i++) {
@@ -368,6 +371,15 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
     }
 
     acpi_table_end(linker, &table);
+
+    /*
+     * tell firmware to write into GPA the address of HEST via fw_cfg,
+     * once initialized.
+     */
+    bios_linker_loader_write_pointer(linker,
+                                     ACPI_HEST_ADDR_FW_CFG_FILE, 0,
+                                     sizeof(uint64_t),
+                                     ACPI_BUILD_TABLE_FILE, hest_offset);
 }
 
 void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
@@ -381,6 +393,9 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
     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);
 
+    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);
+
     ags->present = true;
 }
 
diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
index e47ffacbb5c9..a07c30ef13b7 100644
--- a/include/hw/acpi/ghes.h
+++ b/include/hw/acpi/ghes.h
@@ -58,6 +58,7 @@ enum AcpiGhesNotifyType {
 };
 
 typedef struct AcpiGhesState {
+    uint64_t hest_addr_le;
     uint64_t hw_error_le;
     bool present; /* True if GHES is present at all on this board */
 } AcpiGhesState;
-- 
2.46.1

[PATCH RFC 2/5] acpi/ghes: Use HEST table offsets when preparing GHES records

[Mauro Carvalho Chehab]

There are two pointers that are needed during error injection:

1. The start address of the CPER block to be stored;
2. The address of the ack, which needs a reset before next error.

Calculate them preferrable from the HEST table, as this allows
checking the source ID, the size of the table and the type of
HEST error block structures.

Yet, keep the old code, as this is needed for migration purposes.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 hw/acpi/ghes.c | 93 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 83 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 2c2cf444edeb..313a6e453af6 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -61,6 +61,23 @@
  */
 #define ACPI_GHES_GESB_SIZE                 20
 
+/*
+ * Offsets with regards to the start of the HEST table stored at
+ * ags->hest_addr_le, according with the memory layout map at
+ * docs/specs/acpi_hest_ghes.rst.
+ */
+
+/* ACPI 6.2: 18.3.2.8 Generic Hardware Error Source version 2
+ * Table 18-382 Generic Hardware Error Source version 2 (GHESv2) Structure
+ */
+#define HEST_GHES_V2_TABLE_SIZE  92
+#define GHES_ACK_OFFSET          (64 + GAS_ADDR_OFFSET)
+
+/* ACPI 6.2: 18.3.2.7: Generic Hardware Error Source
+ * Table 18-380: 'Error Status Address' field
+ */
+#define GHES_ERR_ST_ADDR_OFFSET  (20 + GAS_ADDR_OFFSET)
+
 /*
  * Values for error_severity field
  */
@@ -218,14 +235,6 @@ static void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker,
 {
     int i, error_status_block_offset;
 
-    /*
-     * TODO: Current version supports only one source.
-     * A further patch will drop this check, after adding a proper migration
-     * code, as, for the code to work, we need to store a bios pointer to the
-     * HEST table.
-     */
-    assert(num_sources == 1);
-
     /* Build error_block_address */
     for (i = 0; i < num_sources; i++) {
         build_append_int_noprefix(hardware_errors, 0, sizeof(uint64_t));
@@ -425,6 +434,65 @@ static void get_ghes_offsets(uint64_t ghes_addr,
     *read_ack_register_addr = ghes_addr + sizeof(uint64_t);
 }
 
+static void get_hest_offsets(uint16_t source_id, uint64_t hest_addr,
+                             uint64_t *cper_addr,
+                             uint64_t *read_ack_start_addr,
+                              Error **errp)
+{
+    uint64_t hest_err_block_addr, hest_read_ack_start_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));
+
+    err_source_struct = hest_addr + sizeof(num_sources);
+
+    /*
+     * Currently, HEST Error source navigates only for GHESv2 tables
+     */
+
+    for (i = 0; i < num_sources; i++) {
+        uint64_t addr = err_source_struct;
+        uint16_t type, src_id;
+
+        cpu_physical_memory_read(addr, &type, sizeof(type));
+
+        /* For now, we only know the size of GHESv2 table */
+        assert(type == ACPI_GHES_SOURCE_GENERIC_ERROR_V2);
+
+        /* It is GHES. Compare CPER source address */
+        addr += sizeof(type);
+        cpu_physical_memory_read(addr, &src_id, sizeof(src_id));
+
+        if (src_id == source_id) {
+            break;
+        }
+
+        err_source_struct += HEST_GHES_V2_TABLE_SIZE;
+    }
+    if (i == num_sources) {
+        error_setg(errp, "HEST: Source %d not found.", source_id);
+        return;
+    }
+
+    /* Navigate though table address pointers */
+    hest_err_block_addr = err_source_struct + GHES_ERR_ST_ADDR_OFFSET;
+    hest_read_ack_start_addr = err_source_struct + GHES_ACK_OFFSET;
+
+    cpu_physical_memory_read(hest_err_block_addr, &error_block_addr,
+                             sizeof(error_block_addr));
+
+    cpu_physical_memory_read(error_block_addr, cper_addr,
+                             sizeof(*cper_addr));
+
+    cpu_physical_memory_read(hest_read_ack_start_addr, read_ack_start_addr,
+                             sizeof(*read_ack_start_addr));
+}
+
 void ghes_record_cper_errors(const void *cper, size_t len,
                              uint16_t source_id, Error **errp)
 {
@@ -445,8 +513,13 @@ void ghes_record_cper_errors(const void *cper, size_t len,
     }
     ags = &acpi_ged_state->ghes_state;
 
-    get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
-                     &cper_addr, &read_ack_register_addr);
+    if (!ags->hest_addr_le) {
+        get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
+                         &cper_addr, &read_ack_register_addr);
+    } else {
+        get_hest_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
+                         &cper_addr, &read_ack_register_addr, errp);
+    }
 
     if (!cper_addr) {
         error_setg(errp, "can not find Generic Error Status Block");
-- 
2.46.1

Re: [PATCH RFC 2/5] acpi/ghes: Use HEST table offsets when preparing GHES records

[Igor Mammedov]

On Tue,  1 Oct 2024 13:42:47 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> There are two pointers that are needed during error injection:
> 
> 1. The start address of the CPER block to be stored;
> 2. The address of the ack, which needs a reset before next error.
> 
> Calculate them preferrable from the HEST table, as this allows
> checking the source ID, the size of the table and the type of
> HEST error block structures.
> 
> Yet, keep the old code, as this is needed for migration purposes.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  hw/acpi/ghes.c | 93 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 83 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 2c2cf444edeb..313a6e453af6 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -61,6 +61,23 @@
>   */
>  #define ACPI_GHES_GESB_SIZE                 20
>  
> +/*
> + * Offsets with regards to the start of the HEST table stored at
> + * ags->hest_addr_le, according with the memory layout map at
> + * docs/specs/acpi_hest_ghes.rst.
> + */
> +
> +/* ACPI 6.2: 18.3.2.8 Generic Hardware Error Source version 2
> + * Table 18-382 Generic Hardware Error Source version 2 (GHESv2) Structure
> + */
> +#define HEST_GHES_V2_TABLE_SIZE  92
> +#define GHES_ACK_OFFSET          (64 + GAS_ADDR_OFFSET)
> +
> +/* ACPI 6.2: 18.3.2.7: Generic Hardware Error Source
> + * Table 18-380: 'Error Status Address' field
> + */
> +#define GHES_ERR_ST_ADDR_OFFSET  (20 + GAS_ADDR_OFFSET)
> +
>  /*
>   * Values for error_severity field
>   */
> @@ -218,14 +235,6 @@ static void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker,
>  {
>      int i, error_status_block_offset;
>  
> -    /*
> -     * TODO: Current version supports only one source.
> -     * A further patch will drop this check, after adding a proper migration
> -     * code, as, for the code to work, we need to store a bios pointer to the
> -     * HEST table.
> -     */
> -    assert(num_sources == 1);
> -
>      /* Build error_block_address */
>      for (i = 0; i < num_sources; i++) {
>          build_append_int_noprefix(hardware_errors, 0, sizeof(uint64_t));
> @@ -425,6 +434,65 @@ static void get_ghes_offsets(uint64_t ghes_addr,
>      *read_ack_register_addr = ghes_addr + sizeof(uint64_t);
>  }
>  
> +static void get_hest_offsets(uint16_t source_id, uint64_t hest_addr,
> +                             uint64_t *cper_addr,
> +                             uint64_t *read_ack_start_addr,
> +                              Error **errp)

cper/read_ack are GHES specific only, aren't they?

perhaps s/get_hest_offsets/get_ghes_source_offsets/


> +{
> +    uint64_t hest_err_block_addr, hest_read_ack_start_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));
> +
> +    err_source_struct = hest_addr + sizeof(num_sources);
> +
> +    /*
> +     * Currently, HEST Error source navigates only for GHESv2 tables
> +     */
> +
> +    for (i = 0; i < num_sources; i++) {

missing le2cpu(num_sources)

> +        uint64_t addr = err_source_struct;
> +        uint16_t type, src_id;
> +
> +        cpu_physical_memory_read(addr, &type, sizeof(type));

ditto for anything larger than 1 byte that you read from guest memory
(all over the patch)

> +
> +        /* For now, we only know the size of GHESv2 table */
> +        assert(type == ACPI_GHES_SOURCE_GENERIC_ERROR_V2);

Imagine in qemu-9.3 we add non GHES error source, and then try to migrate
such guest to qemu-9.2. It will explode here.
Of-cause we can add some compat property to ged or machine type to
make sure that code works old way in qemu-9.3 for virt-9.2
at expense of keeping 9.2 code in 9.3. Which adds to maintenance burden
and fragile, also it's a matter of time before we screw it up and release
non-migratable/broken QEMU.

So I'd like to avoid piling up compat code/knobs on to of each other
and do it in a way where this src id lookup could gracefully skip
not implemented yet error sources.
This way we won't need any compat knobs to deal with in the future. 

> +
> +        /* It is GHES. Compare CPER source address */
> +        addr += sizeof(type);
> +        cpu_physical_memory_read(addr, &src_id, sizeof(src_id));
> +
> +        if (src_id == source_id) {
> +            break;
> +        }
> +
> +        err_source_struct += HEST_GHES_V2_TABLE_SIZE;
> +    }
> +    if (i == num_sources) {
> +        error_setg(errp, "HEST: Source %d not found.", source_id);
> +        return;
> +    }
> +
> +    /* Navigate though table address pointers */
> +    hest_err_block_addr = err_source_struct + GHES_ERR_ST_ADDR_OFFSET;
> +    hest_read_ack_start_addr = err_source_struct + GHES_ACK_OFFSET;

s/hest_read_ack_start_addr/hest_read_ack_addr/

> +
> +    cpu_physical_memory_read(hest_err_block_addr, &error_block_addr,
> +                             sizeof(error_block_addr));
> +
> +    cpu_physical_memory_read(error_block_addr, cper_addr,
> +                             sizeof(*cper_addr));
> +
> +    cpu_physical_memory_read(hest_read_ack_start_addr, read_ack_start_addr,
> +                             sizeof(*read_ack_start_addr));
> +}
> +
>  void ghes_record_cper_errors(const void *cper, size_t len,
>                               uint16_t source_id, Error **errp)
>  {
> @@ -445,8 +513,13 @@ void ghes_record_cper_errors(const void *cper, size_t len,
>      }
>      ags = &acpi_ged_state->ghes_state;
>  
> -    get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
> -                     &cper_addr, &read_ack_register_addr);
> +    if (!ags->hest_addr_le) {
> +        get_ghes_offsets(le64_to_cpu(ags->hw_error_le),

should it be named get_hw_error_offsets

> +                         &cper_addr, &read_ack_register_addr);
> +    } else {
> +        get_hest_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
> +                         &cper_addr, &read_ack_register_addr, errp);
> +    }
>  
>      if (!cper_addr) {
>          error_setg(errp, "can not find Generic Error Status Block");

[PATCH RFC 3/5] acpi/generic_event_device: Update GHES migration to cover hest addr

[Mauro Carvalho Chehab]

The GHES migration logic at GED should now support HEST table
location too.

Increase migration version and change needed to check for both
ghes_addr_le and hest_addr_le.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 hw/acpi/generic_event_device.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index d4dbfb45e181..49ca1fb8e84a 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -369,6 +369,34 @@ static const VMStateDescription vmstate_ghes_state = {
     }
 };
 
+static const VMStateDescription vmstate_hest = {
+    .name = "acpi-ghes",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64(hest_addr_le, AcpiGhesState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static bool hest_needed(void *opaque)
+{
+    AcpiGedState *s = opaque;
+    return s->ghes_state.hest_addr_le;
+}
+
+static const VMStateDescription vmstate_hest_state = {
+    .name = "acpi-ged/ghes",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = hest_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_STRUCT(ghes_state, AcpiGedState, 1,
+                       vmstate_hest, AcpiGhesState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_acpi_ged = {
     .name = "acpi-ged",
     .version_id = 1,
@@ -380,6 +408,7 @@ static const VMStateDescription vmstate_acpi_ged = {
     .subsections = (const VMStateDescription * const []) {
         &vmstate_memhp_state,
         &vmstate_ghes_state,
+        &vmstate_hest_state,
         NULL
     }
 };
-- 
2.46.1

Re: [PATCH RFC 3/5] acpi/generic_event_device: Update GHES migration to cover hest addr

[Igor Mammedov]

On Tue,  1 Oct 2024 13:42:48 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The GHES migration logic at GED should now support HEST table
> location too.
> 
> Increase migration version and change needed to check for both
> ghes_addr_le and hest_addr_le.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

other than minor issues below, lgtm

> ---
>  hw/acpi/generic_event_device.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index d4dbfb45e181..49ca1fb8e84a 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -369,6 +369,34 @@ static const VMStateDescription vmstate_ghes_state = {
>      }
>  };
>  
> +static const VMStateDescription vmstate_hest = {
> +    .name = "acpi-ghes",
duplicate name for section, we use that already for hw_error address
I don't know ramification of (CCIng Peter)

Perhaps
s/ghes/hest/



> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT64(hest_addr_le, AcpiGhesState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static bool hest_needed(void *opaque)
> +{
> +    AcpiGedState *s = opaque;
> +    return s->ghes_state.hest_addr_le;
> +}
> +
> +static const VMStateDescription vmstate_hest_state = {
> +    .name = "acpi-ged/ghes",

ditto

> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = hest_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_STRUCT(ghes_state, AcpiGedState, 1,
> +                       vmstate_hest, AcpiGhesState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_acpi_ged = {
>      .name = "acpi-ged",
>      .version_id = 1,
> @@ -380,6 +408,7 @@ static const VMStateDescription vmstate_acpi_ged = {
>      .subsections = (const VMStateDescription * const []) {
>          &vmstate_memhp_state,
>          &vmstate_ghes_state,
> +        &vmstate_hest_state,
>          NULL
>      }
>  };

Re: [PATCH RFC 3/5] acpi/generic_event_device: Update GHES migration to cover hest addr

[Peter Xu]

On Wed, Oct 02, 2024 at 05:15:43PM +0200, Igor Mammedov wrote:
> On Tue,  1 Oct 2024 13:42:48 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > The GHES migration logic at GED should now support HEST table
> > location too.
> > 
> > Increase migration version and change needed to check for both
> > ghes_addr_le and hest_addr_le.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> other than minor issues below, lgtm
> 
> > ---
> >  hw/acpi/generic_event_device.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index d4dbfb45e181..49ca1fb8e84a 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -369,6 +369,34 @@ static const VMStateDescription vmstate_ghes_state = {
> >      }
> >  };
> >  
> > +static const VMStateDescription vmstate_hest = {
> > +    .name = "acpi-ghes",
> duplicate name for section, we use that already for hw_error address
> I don't know ramification of (CCIng Peter)

Currently the existing vmstate_ghes is embeded inside vmstate_ghes_state,
so maybe.. it's ok, as I remember QEMU only registers top level vmsds (via
vmstate_register_with_alias_id()).

We do have a sanity check in savevm_state_handler_insert() making sure no
duplicated entry will co-exist since commit caa91b3c44cd.

> 
> Perhaps
> s/ghes/hest/

Said that, perhaps it'll still be nice to try avoiding same names indeed if
possible, at least it could make debugging / reading easier sometimes.

> 
> 
> 
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (const VMStateField[]) {
> > +        VMSTATE_UINT64(hest_addr_le, AcpiGhesState),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> > +static bool hest_needed(void *opaque)
> > +{
> > +    AcpiGedState *s = opaque;
> > +    return s->ghes_state.hest_addr_le;
> > +}
> > +
> > +static const VMStateDescription vmstate_hest_state = {
> > +    .name = "acpi-ged/ghes",
> 
> ditto
> 
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = hest_needed,
> > +    .fields = (const VMStateField[]) {
> > +        VMSTATE_STRUCT(ghes_state, AcpiGedState, 1,
> > +                       vmstate_hest, AcpiGhesState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static const VMStateDescription vmstate_acpi_ged = {
> >      .name = "acpi-ged",
> >      .version_id = 1,
> > @@ -380,6 +408,7 @@ static const VMStateDescription vmstate_acpi_ged = {
> >      .subsections = (const VMStateDescription * const []) {
> >          &vmstate_memhp_state,
> >          &vmstate_ghes_state,
> > +        &vmstate_hest_state,
> >          NULL
> >      }
> >  };
> 

-- 
Peter Xu

[PATCH RFC 4/5] acpi/generic_event_device: add logic to detect if HEST addr is available

[Mauro Carvalho Chehab]

Create a new property (x-has-hest-addr) and use it to detect if
the GHES table offsets can be calculated from the HEST address
(qemu 9.2 and upper) or via the legacy way via an offset obtained
from the hardware_errors firmware file.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 hw/acpi/generic_event_device.c | 1 +
 hw/acpi/ghes.c                 | 2 +-
 hw/core/machine.c              | 4 +++-
 include/hw/acpi/ghes.h         | 1 +
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 49ca1fb8e84a..c4677c9ae6b4 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -318,6 +318,7 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
 
 static Property acpi_ged_properties[] = {
     DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0),
+    DEFINE_PROP_BOOL("x-has-hest-addr", AcpiGedState, ghes_state.hest_lookup, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 313a6e453af6..86bad865168c 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -513,7 +513,7 @@ void ghes_record_cper_errors(const void *cper, size_t len,
     }
     ags = &acpi_ged_state->ghes_state;
 
-    if (!ags->hest_addr_le) {
+    if (!ags->hest_lookup) {
         get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
                          &cper_addr, &read_ack_register_addr);
     } else {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index adaba17ebac1..b58afe48aa71 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,7 +34,9 @@
 #include "hw/virtio/virtio-iommu.h"
 #include "audio/audio.h"
 
-GlobalProperty hw_compat_9_1[] = {};
+GlobalProperty hw_compat_9_1[] = {
+    {"x-has-hest-addr", "false"},
+};
 const size_t hw_compat_9_1_len = G_N_ELEMENTS(hw_compat_9_1);
 
 GlobalProperty hw_compat_9_0[] = {
diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
index a07c30ef13b7..040d6ee366b2 100644
--- a/include/hw/acpi/ghes.h
+++ b/include/hw/acpi/ghes.h
@@ -61,6 +61,7 @@ typedef struct AcpiGhesState {
     uint64_t hest_addr_le;
     uint64_t hw_error_le;
     bool present; /* True if GHES is present at all on this board */
+    bool hest_lookup; /* True if HEST address is present */
 } AcpiGhesState;
 
 /*
-- 
2.46.1

Re: [PATCH RFC 4/5] acpi/generic_event_device: add logic to detect if HEST addr is available

[Igor Mammedov]

On Tue,  1 Oct 2024 13:42:49 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Create a new property (x-has-hest-addr) and use it to detect if
> the GHES table offsets can be calculated from the HEST address
> (qemu 9.2 and upper) or via the legacy way via an offset obtained
> from the hardware_errors firmware file.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  hw/acpi/generic_event_device.c | 1 +
>  hw/acpi/ghes.c                 | 2 +-
>  hw/core/machine.c              | 4 +++-
>  include/hw/acpi/ghes.h         | 1 +
>  4 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 49ca1fb8e84a..c4677c9ae6b4 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -318,6 +318,7 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>  
>  static Property acpi_ged_properties[] = {
>      DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0),
> +    DEFINE_PROP_BOOL("x-has-hest-addr", AcpiGedState, ghes_state.hest_lookup, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 313a6e453af6..86bad865168c 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -513,7 +513,7 @@ void ghes_record_cper_errors(const void *cper, size_t len,
>      }
>      ags = &acpi_ged_state->ghes_state;
>  
> -    if (!ags->hest_addr_le) {
> +    if (!ags->hest_lookup) {
>          get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
>                           &cper_addr, &read_ack_register_addr);

just fencing off lookup is not enough,
to be compatible with qemu-9.1 (virt-9.1) we also should not publish hest_addr fwcfg.

Also have assert (to be removed later) to make sure that we put only
1 (existing) error source in HEST.
When you switch to multiple sources, this will become a condition
to switch HEST generation between 9.1 and 9.2+ variants.


>      } else {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index adaba17ebac1..b58afe48aa71 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -34,7 +34,9 @@
>  #include "hw/virtio/virtio-iommu.h"
>  #include "audio/audio.h"
>  
> -GlobalProperty hw_compat_9_1[] = {};
> +GlobalProperty hw_compat_9_1[] = {
> +    {"x-has-hest-addr", "false"},
> +};
>  const size_t hw_compat_9_1_len = G_N_ELEMENTS(hw_compat_9_1);
>  
>  GlobalProperty hw_compat_9_0[] = {
> diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
> index a07c30ef13b7..040d6ee366b2 100644
> --- a/include/hw/acpi/ghes.h
> +++ b/include/hw/acpi/ghes.h
> @@ -61,6 +61,7 @@ typedef struct AcpiGhesState {
>      uint64_t hest_addr_le;
>      uint64_t hw_error_le;
>      bool present; /* True if GHES is present at all on this board */
> +    bool hest_lookup; /* True if HEST address is present */
>  } AcpiGhesState;
>  
>  /*

Re: [PATCH RFC 4/5] acpi/generic_event_device: add logic to detect if HEST addr is available

[Mauro Carvalho Chehab]

Em Thu, 3 Oct 2024 16:27:28 +0200
Igor Mammedov <imammedo@redhat.com> escreveu:

> > +++ b/hw/acpi/ghes.c
> > @@ -513,7 +513,7 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> >      }
> >      ags = &acpi_ged_state->ghes_state;
> >  
> > -    if (!ags->hest_addr_le) {
> > +    if (!ags->hest_lookup) {
> >          get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
> >                           &cper_addr, &read_ack_register_addr);  
> 
> just fencing off lookup is not enough,
> to be compatible with qemu-9.1 (virt-9.1) we also should not publish hest_addr fwcfg.

I tried this:

diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 201e72516608..6bb962d3c449 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -402,8 +402,10 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
     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);
 
-    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);
+    if (ags->hest_lookup) {
+        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);
+    }
 
     ags->present = true;
 }

But with such change, boot fails:

EFI stub: Booting Linux Kernel...
UpdateRegionMappingRecursive(0): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(1): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(2): C0000000 - 100000000 set 6000000000070C clr 0
UpdateRegionMappingRecursive(2): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(3): DF000000 - DF200000 set 6000000000070C clr 0
UpdateRegionMappingRecursive(3): DF100000 - DF200000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(3): E1A00000 - E1C00000 set 6000000000070C clr 0
UpdateRegionMappingRecursive(3): E1A00000 - E1B90000 set 400 clr FF9F000000000B3F
EFI stub: Generating empty DTB
EFI stub: Exiting boot services...
UpdateRegionMappingRecursive(0): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(1): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(2): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(3): 139A00000 - 139C00000 set 6000000000070C clr 0
UpdateRegionMappingRecursive(3): 139AC1000 - 139C00000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(3): 139C00000 - 139CD0000 set 400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(0): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(1): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(2): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
UpdateRegionMappingRecursive(3): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
SetUefiImageMemoryAttributes - 0x000000013FE60000 - 0x0000000000040000 (0x0000000000000008)
UpdateRegionMappingRecursive(0): 13FE60000 - 13FEA0000 set 70C clr 0
UpdateRegionMappingRecursive(1): 13FE60000 - 13FEA0000 set 70C clr 0
UpdateRegionMappingRecursive(2): 13FE60000 - 13FEA0000 set 70C clr 0
UpdateRegionMappingRecursive(3): 13FE60000 - 13FEA0000 set 70C clr 0
SetUefiImageMemoryAttributes - 0x000000013CAF0000 - 0x0000000000040000 (0x0000000000000008)
UpdateRegionMappingRecursive(0): 13CAF0000 - 13CB30000 set 70C clr 0
UpdateRegionMappingRecursive(1): 13CAF0000 - 13CB30000 set 70C clr 0
UpdateRegionMappingRecursive(2): 13CAF0000 - 13CB30000 set 70C clr 0
UpdateRegionMappingRecursive(3): 13CAF0000 - 13CB30000 set 70C clr 0
SetUefiImageMemoryAttributes - 0x000000013CAA0000 - 0x0000000000040000 (0x0000000000000008)
UpdateRegionMappingRecursive(0): 13CAA0000 - 13CAE0000 set 70C clr 0
UpdateRegionMappingRecursive(1): 13CAA0000 - 13CAE0000 set 70C clr 0
UpdateRegionMappingRecursive(2): 13CAA0000 - 13CAE0000 set 70C clr 0
UpdateRegionMappingRecursive(3): 13CAA0000 - 13CAE0000 set 70C clr 0
SetUefiImageMemoryAttributes - 0x000000013CA50000 - 0x0000000000040000 (0x0000000000000008)
UpdateRegionMappingRecursive(0): 13CA50000 - 13CA90000 set 70C clr 0
UpdateRegionMappingRecursive(1): 13CA50000 - 13CA90000 set 70C clr 0
UpdateRegionMappingRecursive(2): 13CA50000 - 13CA90000 set 70C clr 0
UpdateRegionMappingRecursive(3): 13CA50000 - 13CA90000 set 70C clr 0
SetUefiImageMemoryAttributes - 0x000000013C960000 - 0x0000000000040000 (0x0000000000000008)
UpdateRegionMappingRecursive(0): 13C960000 - 13C9A0000 set 70C clr 0
UpdateRegionMappingRecursive(1): 13C960000 - 13C9A0000 set 70C clr 0
UpdateRegionMappingRecursive(2): 13C960000 - 13C9A0000 set 70C clr 0
UpdateRegionMappingRecursive(3): 13C960000 - 13C9A0000 set 70C clr 0
SetUefiImageMemoryAttributes - 0x000000013FE20000 - 0x0000000000030000 (0x0000000000000008)
UpdateRegionMappingRecursive(0): 13FE20000 - 13FE50000 set 70C clr 0
UpdateRegionMappingRecursive(1): 13FE20000 - 13FE50000 set 70C clr 0
UpdateRegionMappingRecursive(2): 13FE20000 - 13FE50000 set 70C clr 0
UpdateRegionMappingRecursive(3): 13FE20000 - 13FE50000 set 70C clr 0
SetUefiImageMemoryAttributes - 0x000000013C7B0000 - 0x0000000000030000 (0x0000000000000008)
UpdateRegionMappingRecursive(0): 13C7B0000 - 13C7E0000 set 70C clr 0
UpdateRegionMappingRecursive(1): 13C7B0000 - 13C7E0000 set 70C clr 0
UpdateRegionMappingRecursive(2): 13C7B0000 - 13C7E0000 set 70C clr 0
UpdateRegionMappingRecursive(3): 13C7B0000 - 13C7E0000 set 70C clr 0
SetUefiImageMemoryAttributes - 0x000000013C770000 - 0x0000000000030000 (0x0000000000000008)
UpdateRegionMappingRecursive(0): 13C770000 - 13C7A0000 set 70C clr 0
UpdateRegionMappingRecursive(1): 13C770000 - 13C7A0000 set 70C clr 0
UpdateRegionMappingRecursive(2): 13C770000 - 13C7A0000 set 70C clr 0
UpdateRegionMappingRecursive(3): 13C770000 - 13C7A0000 set 70C clr 0

At this point, nothing else appears, and bios doesn't boot OSPM. 

(I'm using an arm64 BIOS with debug enabled)

Thanks,
Mauro

Re: [PATCH RFC 4/5] acpi/generic_event_device: add logic to detect if HEST addr is available

[Mauro Carvalho Chehab]

Em Tue, 12 Nov 2024 15:55:57 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Thu, 3 Oct 2024 16:27:28 +0200
> Igor Mammedov <imammedo@redhat.com> escreveu:
> 
> > > +++ b/hw/acpi/ghes.c
> > > @@ -513,7 +513,7 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> > >      }
> > >      ags = &acpi_ged_state->ghes_state;
> > >  
> > > -    if (!ags->hest_addr_le) {
> > > +    if (!ags->hest_lookup) {
> > >          get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
> > >                           &cper_addr, &read_ack_register_addr);    
> > 
> > just fencing off lookup is not enough,
> > to be compatible with qemu-9.1 (virt-9.1) we also should not publish hest_addr fwcfg.  
> 
> I tried this:
> 
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 201e72516608..6bb962d3c449 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -402,8 +402,10 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
>      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);
>  
> -    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);
> +    if (ags->hest_lookup) {
> +        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);
> +    }
>  
>      ags->present = true;
>  }
> 
> But with such change, boot fails:
> 
> EFI stub: Booting Linux Kernel...
> UpdateRegionMappingRecursive(0): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(1): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(2): C0000000 - 100000000 set 6000000000070C clr 0
> UpdateRegionMappingRecursive(2): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): DF000000 - DF200000 set 6000000000070C clr 0
> UpdateRegionMappingRecursive(3): DF100000 - DF200000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): E1A00000 - E1C00000 set 6000000000070C clr 0
> UpdateRegionMappingRecursive(3): E1A00000 - E1B90000 set 400 clr FF9F000000000B3F
> EFI stub: Generating empty DTB
> EFI stub: Exiting boot services...
> UpdateRegionMappingRecursive(0): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(1): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(2): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): 139A00000 - 139C00000 set 6000000000070C clr 0
> UpdateRegionMappingRecursive(3): 139AC1000 - 139C00000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): 139C00000 - 139CD0000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(0): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(1): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(2): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
> SetUefiImageMemoryAttributes - 0x000000013FE60000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13FE60000 - 13FEA0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13FE60000 - 13FEA0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13FE60000 - 13FEA0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13FE60000 - 13FEA0000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013CAF0000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13CAF0000 - 13CB30000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13CAF0000 - 13CB30000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13CAF0000 - 13CB30000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13CAF0000 - 13CB30000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013CAA0000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13CAA0000 - 13CAE0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13CAA0000 - 13CAE0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13CAA0000 - 13CAE0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13CAA0000 - 13CAE0000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013CA50000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13CA50000 - 13CA90000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13CA50000 - 13CA90000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13CA50000 - 13CA90000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13CA50000 - 13CA90000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013C960000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13C960000 - 13C9A0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13C960000 - 13C9A0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13C960000 - 13C9A0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13C960000 - 13C9A0000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013FE20000 - 0x0000000000030000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13FE20000 - 13FE50000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13FE20000 - 13FE50000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13FE20000 - 13FE50000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13FE20000 - 13FE50000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013C7B0000 - 0x0000000000030000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13C7B0000 - 13C7E0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13C7B0000 - 13C7E0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13C7B0000 - 13C7E0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13C7B0000 - 13C7E0000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013C770000 - 0x0000000000030000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13C770000 - 13C7A0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13C770000 - 13C7A0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13C770000 - 13C7A0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13C770000 - 13C7A0000 set 70C clr 0
> 
> At this point, nothing else appears, and bios doesn't boot OSPM. 
> 
> (I'm using an arm64 BIOS with debug enabled)
> 
> Thanks,
> Mauro

Got it. In order to be able to remove a call to 
fw_cfg_add_file_callback(), no calls to bios_linker_loader_write_pointer()
can happen.

That basically explains why we can't do:

	if (!ags->hest_lookup) {
	     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);
	}

We need the BIOS file callback to solve all the pointers that
were created between HEST table and the hardware error table.

This hunk worked:


diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 201e72516608..245efde75a8f 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -385,10 +385,12 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
      * tell firmware to write into GPA the address of HEST via fw_cfg,
      * once initialized.
      */
-    bios_linker_loader_write_pointer(linker,
-                                     ACPI_HEST_ADDR_FW_CFG_FILE, 0,
-                                     sizeof(uint64_t),
-                                     ACPI_BUILD_TABLE_FILE, hest_offset);
+    if (ags->hest_lookup) {
+        bios_linker_loader_write_pointer(linker,
+                                         ACPI_HEST_ADDR_FW_CFG_FILE, 0,
+                                         sizeof(uint64_t),
+                                         ACPI_BUILD_TABLE_FILE, hest_offset);
+    }
 }
 
 void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
@@ -402,8 +404,10 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
     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);
 
-    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);
+    if (ags->hest_lookup) {
+        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);
+    }
 
     ags->present = true;
 }

Thanks,
Mauro

[PATCH RFC 5/5] arm/virt-acpi-build: Properly handle virt-9.1

[Mauro Carvalho Chehab]

A virt-9.1 machine can have only one source ID.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 hw/arm/virt-acpi-build.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 476c365851c4..8036eb5953d0 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -894,6 +894,10 @@ static const AcpiNotificationSourceId hest_ghes_notify[] = {
     {ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA},
 };
 
+static const AcpiNotificationSourceId hest_ghes_notify_9_1[] = {
+    {ACPI_HEST_SRC_ID_QMP, ACPI_GHES_NOTIFY_GPIO},
+};
+
 static
 void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
 {
@@ -947,10 +951,28 @@ 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,
-                        hest_ghes_notify, ARRAY_SIZE(hest_ghes_notify),
-                        vms->oem_id, vms->oem_table_id);
+        AcpiGhesState *ags;
+        AcpiGedState *acpi_ged_state;
+
+        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);
+
+            if (!ags->hest_lookup) {
+                acpi_build_hest(tables_blob, tables->hardware_errors,
+                                tables->linker, hest_ghes_notify_9_1,
+                                ARRAY_SIZE(hest_ghes_notify_9_1),
+                                vms->oem_id, vms->oem_table_id);
+            } else {
+                acpi_build_hest(tables_blob, tables->hardware_errors,
+                                tables->linker, hest_ghes_notify,
+                                ARRAY_SIZE(hest_ghes_notify),
+                                vms->oem_id, vms->oem_table_id);
+            }
+        }
     }
 
     if (ms->numa_state->num_nodes > 0) {
-- 
2.46.1

Re: [PATCH RFC 5/5] arm/virt-acpi-build: Properly handle virt-9.1

[Igor Mammedov]

On Tue,  1 Oct 2024 13:42:50 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> A virt-9.1 machine can have only one source ID.

and here it is.
I'd just merge this into previous patch

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  hw/arm/virt-acpi-build.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 476c365851c4..8036eb5953d0 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -894,6 +894,10 @@ static const AcpiNotificationSourceId hest_ghes_notify[] = {
>      {ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA},
>  };
>  
> +static const AcpiNotificationSourceId hest_ghes_notify_9_1[] = {
> +    {ACPI_HEST_SRC_ID_QMP, ACPI_GHES_NOTIFY_GPIO},
> +};
> +
>  static
>  void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>  {
> @@ -947,10 +951,28 @@ 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,
> -                        hest_ghes_notify, ARRAY_SIZE(hest_ghes_notify),
> -                        vms->oem_id, vms->oem_table_id);
> +        AcpiGhesState *ags;
> +        AcpiGedState *acpi_ged_state;
> +
> +        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);
> +
> +            if (!ags->hest_lookup) {
> +                acpi_build_hest(tables_blob, tables->hardware_errors,
> +                                tables->linker, hest_ghes_notify_9_1,
> +                                ARRAY_SIZE(hest_ghes_notify_9_1),
> +                                vms->oem_id, vms->oem_table_id);
> +            } else {
> +                acpi_build_hest(tables_blob, tables->hardware_errors,
> +                                tables->linker, hest_ghes_notify,
> +                                ARRAY_SIZE(hest_ghes_notify),
> +                                vms->oem_id, vms->oem_table_id);
> +            }
> +        }
>      }
>  
>      if (ms->numa_state->num_nodes > 0) {

Re: [PATCH RFC 0/5] Change ghes driver to use HEST-based offsets

[Igor Mammedov]

On Tue,  1 Oct 2024 13:42:45 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> This RFC series was part of the previous PR to add generic error injection
> support on GHES.
> 
> It contains only the changes of the math used to calculate offsets at
> HEST table and hardware_error firmware file.
> 
> The first patch adds a new firmware file to store HEST address.
> The second patch makes use of it.
> The remaining ones add migration support.
> 
> PS.: I'm sending this as a RFC as using the proceudure defined at the
> pseudo-migration of:
> 
> 	https://www.linux-kvm.org/page/Migration
> 
> Didn't work. I tried to use two different QEMU versions to check a
> real life case and also to use just one QEMU and trying to load a
> virt-9.1 state on a virt-9.2 machine. 
> 
> For instance, trying to restore a virt-9.1 state on virt-9.2 gave me
> this error:
> 
> 	(qemu) qemu: Machine type received is 'virt-9.1' and local is 'virt-9.2'
> 	qemu: load of migration failed: Invalid argument

that's expected (idea is to keep machine type (virt-X) ABI stable so
it would work the same way on old and new QEMU)
migration is meant to move VM of the same machine type to a new/another QEMU instance.

i.e try migrate 

qemu-9.1 -M virt-9.1  => qemu-9.2 -M virt-9.1
and vice-versa
migration should succeed and memory error injection should still work
the old way in both instances (I don't recall anymore how to simulate SEA,
perhaps original author left a description how to do that somewhere on mail-list).

virt-9.2 is never meant to be 

> Yet, running virt-9.1 used the old math code (offsets from hardware_errors firmware
> file) while running virt-9.2 executed the new math code using HEST address.
> 
> Mauro Carvalho Chehab (5):
>   acpi/ghes: add a firmware file with HEST address
>   acpi/ghes: Use HEST table offsets when preparing GHES records
>   acpi/generic_event_device: Update GHES migration to cover hest addr
>   acpi/generic_event_device: add logic to detect if HEST addr is
>     available
>   arm/virt-acpi-build: Properly handle virt-9.1
> 
>  hw/acpi/generic_event_device.c |  30 +++++++++
>  hw/acpi/ghes.c                 | 108 ++++++++++++++++++++++++++++++---
>  hw/arm/virt-acpi-build.c       |  30 +++++++--
>  hw/core/machine.c              |   4 +-
>  include/hw/acpi/ghes.h         |   2 +
>  5 files changed, 159 insertions(+), 15 deletions(-)
> 

Re: [PATCH RFC 0/5] Change ghes driver to use HEST-based offsets

[Mauro Carvalho Chehab]

Em Wed, 2 Oct 2024 15:45:34 +0200
Igor Mammedov <imammedo@redhat.com> escreveu:

> On Tue,  1 Oct 2024 13:42:45 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > This RFC series was part of the previous PR to add generic error injection
> > support on GHES.
> > 
> > It contains only the changes of the math used to calculate offsets at
> > HEST table and hardware_error firmware file.
> > 
> > The first patch adds a new firmware file to store HEST address.
> > The second patch makes use of it.
> > The remaining ones add migration support.
> > 
> > PS.: I'm sending this as a RFC as using the proceudure defined at the
> > pseudo-migration of:
> > 
> > 	https://www.linux-kvm.org/page/Migration
> > 
> > Didn't work. I tried to use two different QEMU versions to check a
> > real life case and also to use just one QEMU and trying to load a
> > virt-9.1 state on a virt-9.2 machine. 
> > 
> > For instance, trying to restore a virt-9.1 state on virt-9.2 gave me
> > this error:
> > 
> > 	(qemu) qemu: Machine type received is 'virt-9.1' and local is 'virt-9.2'
> > 	qemu: load of migration failed: Invalid argument  
> 
> that's expected (idea is to keep machine type (virt-X) ABI stable so
> it would work the same way on old and new QEMU)
> migration is meant to move VM of the same machine type to a new/another QEMU instance.

I found a couple of issues and, after the fixes, it can successfully
migrate both virt-9.1 and virt-9.2 machines. 

> 
> i.e try migrate 
> 
> qemu-9.1 -M virt-9.1  => qemu-9.2 -M virt-9.1
> and vice-versa
> migration should succeed and memory error injection should still work
> the old way in both instances (I don't recall anymore how to simulate SEA,
> perhaps original author left a description how to do that somewhere on mail-list).

Those work as well, but I had to pass -cpu cortex-a57 to both 9.1
and 9.2, as using -cpu max caused qemu to refuse loading the guest.

I tested with both:

	qemu-9.1 -M virt-9.1 -cpu cortex-a57 => qemu-9.2 -M virt-9.1 -cpu cortex-a57
	qemu-9.2 -M virt-9.1 -cpu cortex-a57 => qemu-9.1 -M virt-9.1 -cpu cortex-a57

I'll address your other comments to the series and post a new version
today.

Thanks,
Mauro

Re: [PATCH RFC 0/5] Change ghes driver to use HEST-based offsets

[Mauro Carvalho Chehab]

Em Wed, 13 Nov 2024 07:54:18 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Wed, 2 Oct 2024 15:45:34 +0200
> Igor Mammedov <imammedo@redhat.com> escreveu:
> 
> > On Tue,  1 Oct 2024 13:42:45 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >   
> > > This RFC series was part of the previous PR to add generic error injection
> > > support on GHES.
> > > 
> > > It contains only the changes of the math used to calculate offsets at
> > > HEST table and hardware_error firmware file.
> > > 
> > > The first patch adds a new firmware file to store HEST address.
> > > The second patch makes use of it.
> > > The remaining ones add migration support.
> > > 
> > > PS.: I'm sending this as a RFC as using the proceudure defined at the
> > > pseudo-migration of:
> > > 
> > > 	https://www.linux-kvm.org/page/Migration
> > > 
> > > Didn't work. I tried to use two different QEMU versions to check a
> > > real life case and also to use just one QEMU and trying to load a
> > > virt-9.1 state on a virt-9.2 machine. 
> > > 
> > > For instance, trying to restore a virt-9.1 state on virt-9.2 gave me
> > > this error:
> > > 
> > > 	(qemu) qemu: Machine type received is 'virt-9.1' and local is 'virt-9.2'
> > > 	qemu: load of migration failed: Invalid argument    
> > 
> > that's expected (idea is to keep machine type (virt-X) ABI stable so
> > it would work the same way on old and new QEMU)
> > migration is meant to move VM of the same machine type to a new/another QEMU instance.  
> 
> I found a couple of issues and, after the fixes, it can successfully
> migrate both virt-9.1 and virt-9.2 machines. 
> 
> > 
> > i.e try migrate 
> > 
> > qemu-9.1 -M virt-9.1  => qemu-9.2 -M virt-9.1
> > and vice-versa
> > migration should succeed and memory error injection should still work
> > the old way in both instances (I don't recall anymore how to simulate SEA,
> > perhaps original author left a description how to do that somewhere on mail-list).  
> 
> Those work as well, but I had to pass -cpu cortex-a57 to both 9.1
> and 9.2, as using -cpu max caused qemu to refuse loading the guest.
> 
> I tested with both:
> 
> 	qemu-9.1 -M virt-9.1 -cpu cortex-a57 => qemu-9.2 -M virt-9.1 -cpu cortex-a57
> 	qemu-9.2 -M virt-9.1 -cpu cortex-a57 => qemu-9.1 -M virt-9.1 -cpu cortex-a57

Forgot to mention, but I modified qemu-9.1 to use GPIO instead of SEA, as
it is a lot easier to do the tests using the error injection logic.
Also, I don't know how to test SEA errors.

> I'll address your other comments to the series and post a new version
> today.
> 
> Thanks,
> Mauro



Thanks,
Mauro