| Message ID | 556c19c1f3fa907c6cc13b62e060f6baa6faf2cf.1737560101.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Change ghes to use HEST-based offsets and add support for error inject | expand |
On Wed, 22 Jan 2025 16:46:22 +0100 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> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Wed, 22 Jan 2025 16:46:22 +0100 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 10.0 by now > 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 | 28 +++++++++++++++++++++------- > hw/arm/virt-acpi-build.c | 30 ++++++++++++++++++++++++++---- > hw/core/machine.c | 2 ++ > include/hw/acpi/ghes.h | 1 + > 5 files changed, 51 insertions(+), 11 deletions(-) > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > index 5346cae573b7..fe537ed05c66 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 const 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), s/hest_lookup/use_hest_addr/ > }; > > static const VMStateDescription vmstate_memhp_state = { > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > index b46b563bcaf8..86c97f60d6a0 100644 > --- a/hw/acpi/ghes.c > +++ b/hw/acpi/ghes.c > @@ -359,6 +359,8 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, > { > AcpiTable table = { .sig = "HEST", .rev = 1, > .oem_id = oem_id, .oem_table_id = oem_table_id }; > + AcpiGedState *acpi_ged_state; > + AcpiGhesState *ags = NULL; > int i; > > build_ghes_error_table(hardware_errors, linker, num_sources); > @@ -379,10 +381,20 @@ 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); > + > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, > + NULL)); the caller, already did lookup, just pass hest_lookup as an argument and use it here > + if (!acpi_ged_state) { > + return; > + } > + > + ags = &acpi_ged_state->ghes_state; > + 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, > @@ -396,8 +408,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); btw shouldn't we disable hw_error_le if hest_lookup is active? > > - 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 && ags->hest_lookup) { why bother with 'ags &&' if we don't do it hw_error_le? > + 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; > } > @@ -512,7 +526,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) { why? !ags->hest_addr_le is sufficient > 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 3d411787fc37..ada5d08cfbe7 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -897,6 +897,10 @@ static const AcpiNotificationSourceId hest_ghes_notify[] = { > { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA }, > }; > > +static const AcpiNotificationSourceId hest_ghes_notify_9_2[] = { > + { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA }, > +}; > + > static > void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > { > @@ -950,10 +954,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_2, > + ARRAY_SIZE(hest_ghes_notify_9_2), > + 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) { > diff --git a/hw/core/machine.c b/hw/core/machine.c > index c23b39949649..0d0cde481954 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -34,10 +34,12 @@ > #include "hw/virtio/virtio-pci.h" > #include "hw/virtio/virtio-net.h" > #include "hw/virtio/virtio-iommu.h" > +#include "hw/acpi/generic_event_device.h" > #include "audio/audio.h" > > GlobalProperty hw_compat_9_2[] = { > {"arm-cpu", "backcompat-pauth-default-use-qarma5", "true"}, > + { TYPE_ACPI_GED, "x-has-hest-addr", "false" }, > }; > const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2); > > diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h > index 237721fec0a2..164ed8b0f9a3 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 */ and perhaps reformulate this as well > + bool hest_lookup; /* True if HEST address is present */ if device should use HEST addr for error source lookup > } AcpiGhesState; > > /*
Em Fri, 24 Jan 2025 11:23:46 +0100 Igor Mammedov <imammedo@redhat.com> escreveu: > On Wed, 22 Jan 2025 16:46:22 +0100 > 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 > > 10.0 by now > > > 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 | 28 +++++++++++++++++++++------- > > hw/arm/virt-acpi-build.c | 30 ++++++++++++++++++++++++++---- > > hw/core/machine.c | 2 ++ > > include/hw/acpi/ghes.h | 1 + > > 5 files changed, 51 insertions(+), 11 deletions(-) > > > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > > index 5346cae573b7..fe537ed05c66 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 const 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), > > s/hest_lookup/use_hest_addr/ > > > }; > > > > static const VMStateDescription vmstate_memhp_state = { > > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > > index b46b563bcaf8..86c97f60d6a0 100644 > > --- a/hw/acpi/ghes.c > > +++ b/hw/acpi/ghes.c > > @@ -359,6 +359,8 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, > > { > > AcpiTable table = { .sig = "HEST", .rev = 1, > > .oem_id = oem_id, .oem_table_id = oem_table_id }; > > + AcpiGedState *acpi_ged_state; > > + AcpiGhesState *ags = NULL; > > int i; > > > > build_ghes_error_table(hardware_errors, linker, num_sources); > > @@ -379,10 +381,20 @@ 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); > > + > > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, > > + NULL)); > > the caller, already did lookup, > just pass hest_lookup as an argument and use it here for all the above: OK. > > + if (!acpi_ged_state) { > > + return; > > + } > > + > > + ags = &acpi_ged_state->ghes_state; > > + 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, > > @@ -396,8 +408,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); > > btw shouldn't we disable hw_error_le if hest_lookup is active? Despite not being used, we still need the fw_cfg logic to recalculate the table offsets, solving the bios_linker stuff. At the tests I did, not having a callback causes some fw_cfg issue when QEMU tries to load the firmware or tries to update it. > > > > - 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 && ags->hest_lookup) { > > why bother with 'ags &&' if we don't do it hw_error_le? Legacy stuff. I'll drop "ags &&". > > > > + 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; > > } > > @@ -512,7 +526,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) { > why? !ags->hest_addr_le is sufficient Either checking for "hest_addr_le" or for "use_hest_addr" would equally work, assuming that address == 0 is invalid. I opted to use the latest one because you requested on a previous review, and also because it makes clearer that this comes from the migration logic, which dictates what kind of lookup should be done. From my side, either way works fine. What do you prefer? > > > 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 3d411787fc37..ada5d08cfbe7 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -897,6 +897,10 @@ static const AcpiNotificationSourceId hest_ghes_notify[] = { > > { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA }, > > }; > > > > +static const AcpiNotificationSourceId hest_ghes_notify_9_2[] = { > > + { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA }, > > +}; > > + > > static > > void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > > { > > @@ -950,10 +954,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_2, > > + ARRAY_SIZE(hest_ghes_notify_9_2), > > + 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) { > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index c23b39949649..0d0cde481954 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -34,10 +34,12 @@ > > #include "hw/virtio/virtio-pci.h" > > #include "hw/virtio/virtio-net.h" > > #include "hw/virtio/virtio-iommu.h" > > +#include "hw/acpi/generic_event_device.h" > > #include "audio/audio.h" > > > > GlobalProperty hw_compat_9_2[] = { > > {"arm-cpu", "backcompat-pauth-default-use-qarma5", "true"}, > > + { TYPE_ACPI_GED, "x-has-hest-addr", "false" }, > > }; > > const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2); > > > > diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h > > index 237721fec0a2..164ed8b0f9a3 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 */ > and perhaps reformulate this as well > > > + bool hest_lookup; /* True if HEST address is present */ > if device should use HEST addr for error source lookup > > > } AcpiGhesState; > > > > /* > Thanks, Mauro
Em Tue, 28 Jan 2025 12:29:51 +0100 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > Em Fri, 24 Jan 2025 11:23:46 +0100 > Igor Mammedov <imammedo@redhat.com> escreveu: > > > On Wed, 22 Jan 2025 16:46:22 +0100 > > 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 > > > > 10.0 by now > > > > > 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 | 28 +++++++++++++++++++++------- > > > hw/arm/virt-acpi-build.c | 30 ++++++++++++++++++++++++++---- > > > hw/core/machine.c | 2 ++ > > > include/hw/acpi/ghes.h | 1 + > > > 5 files changed, 51 insertions(+), 11 deletions(-) > > > > > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > > > index 5346cae573b7..fe537ed05c66 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 const 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), > > > > s/hest_lookup/use_hest_addr/ > > > > > }; > > > > > > static const VMStateDescription vmstate_memhp_state = { > > > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > > > index b46b563bcaf8..86c97f60d6a0 100644 > > > --- a/hw/acpi/ghes.c > > > +++ b/hw/acpi/ghes.c > > > @@ -359,6 +359,8 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, > > > { > > > AcpiTable table = { .sig = "HEST", .rev = 1, > > > .oem_id = oem_id, .oem_table_id = oem_table_id }; > > > + AcpiGedState *acpi_ged_state; > > > + AcpiGhesState *ags = NULL; > > > int i; > > > > > > build_ghes_error_table(hardware_errors, linker, num_sources); > > > @@ -379,10 +381,20 @@ 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); > > > + > > > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, > > > + NULL)); > > > > the caller, already did lookup, > > just pass hest_lookup as an argument and use it here > > for all the above: OK. > > > > + if (!acpi_ged_state) { > > > + return; > > > + } > > > + > > > + ags = &acpi_ged_state->ghes_state; > > > + 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, > > > @@ -396,8 +408,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); > > > > btw shouldn't we disable hw_error_le if hest_lookup is active? > > Despite not being used, we still need the fw_cfg logic to recalculate the > table offsets, solving the bios_linker stuff. > > At the tests I did, not having a callback causes some fw_cfg issue when QEMU > tries to load the firmware or tries to update it. > > > > > > > - 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 && ags->hest_lookup) { > > > > why bother with 'ags &&' if we don't do it hw_error_le? > > Legacy stuff. I'll drop "ags &&". > > > > > > > > + 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; > > > } > > > @@ -512,7 +526,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) { > > why? !ags->hest_addr_le is sufficient > > Either checking for "hest_addr_le" or for "use_hest_addr" would > equally work, assuming that address == 0 is invalid. I opted to use > the latest one because you requested on a previous review, and also > because it makes clearer that this comes from the migration logic, > which dictates what kind of lookup should be done. > > From my side, either way works fine. What do you prefer? After sleeping on it, IMO checking for !ags->hest_addr_le is better here and will align with the new code from this patch: acpi/ghes: Cleanup the code which gets ghes ged state that will remove ags->present in favor of checking for both hw_error_le and hest_addr_le: AcpiGhesState *acpi_ghes_get_state(void) { AcpiGedState *acpi_ged_state; AcpiGhesState *ags; if (!acpi_ged_state) { return NULL; } ags = &acpi_ged_state->ghes_state; if (!ags->hw_error_le && !ags->hest_addr_le) { return NULL; } return ags; } So, I'll drop this hunk. > > > > > > 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 3d411787fc37..ada5d08cfbe7 100644 > > > --- a/hw/arm/virt-acpi-build.c > > > +++ b/hw/arm/virt-acpi-build.c > > > @@ -897,6 +897,10 @@ static const AcpiNotificationSourceId hest_ghes_notify[] = { > > > { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA }, > > > }; > > > > > > +static const AcpiNotificationSourceId hest_ghes_notify_9_2[] = { > > > + { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA }, > > > +}; > > > + > > > static > > > void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > > > { > > > @@ -950,10 +954,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_2, > > > + ARRAY_SIZE(hest_ghes_notify_9_2), > > > + 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) { > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > > index c23b39949649..0d0cde481954 100644 > > > --- a/hw/core/machine.c > > > +++ b/hw/core/machine.c > > > @@ -34,10 +34,12 @@ > > > #include "hw/virtio/virtio-pci.h" > > > #include "hw/virtio/virtio-net.h" > > > #include "hw/virtio/virtio-iommu.h" > > > +#include "hw/acpi/generic_event_device.h" > > > #include "audio/audio.h" > > > > > > GlobalProperty hw_compat_9_2[] = { > > > {"arm-cpu", "backcompat-pauth-default-use-qarma5", "true"}, > > > + { TYPE_ACPI_GED, "x-has-hest-addr", "false" }, > > > }; > > > const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2); > > > > > > diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h > > > index 237721fec0a2..164ed8b0f9a3 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 */ > > and perhaps reformulate this as well > > > > > + bool hest_lookup; /* True if HEST address is present */ > > if device should use HEST addr for error source lookup > > > > > } AcpiGhesState; > > > > > > /* > > > > > > Thanks, > Mauro Thanks, Mauro
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c index 5346cae573b7..fe537ed05c66 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 const 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), }; static const VMStateDescription vmstate_memhp_state = { diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c index b46b563bcaf8..86c97f60d6a0 100644 --- a/hw/acpi/ghes.c +++ b/hw/acpi/ghes.c @@ -359,6 +359,8 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, { AcpiTable table = { .sig = "HEST", .rev = 1, .oem_id = oem_id, .oem_table_id = oem_table_id }; + AcpiGedState *acpi_ged_state; + AcpiGhesState *ags = NULL; int i; build_ghes_error_table(hardware_errors, linker, num_sources); @@ -379,10 +381,20 @@ 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); + + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, + NULL)); + if (!acpi_ged_state) { + return; + } + + ags = &acpi_ged_state->ghes_state; + 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, @@ -396,8 +408,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 && 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; } @@ -512,7 +526,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_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 3d411787fc37..ada5d08cfbe7 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -897,6 +897,10 @@ static const AcpiNotificationSourceId hest_ghes_notify[] = { { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA }, }; +static const AcpiNotificationSourceId hest_ghes_notify_9_2[] = { + { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA }, +}; + static void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) { @@ -950,10 +954,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_2, + ARRAY_SIZE(hest_ghes_notify_9_2), + 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) { diff --git a/hw/core/machine.c b/hw/core/machine.c index c23b39949649..0d0cde481954 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -34,10 +34,12 @@ #include "hw/virtio/virtio-pci.h" #include "hw/virtio/virtio-net.h" #include "hw/virtio/virtio-iommu.h" +#include "hw/acpi/generic_event_device.h" #include "audio/audio.h" GlobalProperty hw_compat_9_2[] = { {"arm-cpu", "backcompat-pauth-default-use-qarma5", "true"}, + { TYPE_ACPI_GED, "x-has-hest-addr", "false" }, }; const size_t hw_compat_9_2_len = G_N_ELEMENTS(hw_compat_9_2); diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h index 237721fec0a2..164ed8b0f9a3 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; /*
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 | 28 +++++++++++++++++++++------- hw/arm/virt-acpi-build.c | 30 ++++++++++++++++++++++++++---- hw/core/machine.c | 2 ++ include/hw/acpi/ghes.h | 1 + 5 files changed, 51 insertions(+), 11 deletions(-)