Em Tue, 28 Jan 2025 12:29:51 +0100
Mauro Carvalho Chehab <mchehab+hua...@kernel.org> escreveu:
> Em Fri, 24 Jan 2025 11:23:46 +0100
> Igor Mammedov <imamm...@redhat.com> escreveu:
>
> > On Wed, 22 Jan 2025 16:46:22 +0100
> > Mauro Carvalho Chehab <mchehab+hua...@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+hua...@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
