| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241125120830.000025bf@huawei.com>
Date: Mon, 25 Nov 2024 12:08:30 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
CC: Shiju Jose <shiju.jose@...wei.com>, "Michael S. Tsirkin" <mst@...hat.com>,
Philippe Mathieu-Daudé <philmd@...aro.org>, Ani Sinha
<anisinha@...hat.com>, Dongjiu Geng <gengdongjiu1@...il.com>, Eduardo Habkost
<eduardo@...kost.net>, "Igor Mammedov" <imammedo@...hat.com>, Marcel
Apfelbaum <marcel.apfelbaum@...il.com>, Peter Maydell
<peter.maydell@...aro.org>, Shannon Zhao <shannon.zhaosl@...il.com>, Yanan
Wang <wangyanan55@...wei.com>, Zhao Liu <zhao1.liu@...el.com>,
<linux-kernel@...r.kernel.org>, <qemu-arm@...gnu.org>,
<qemu-devel@...gnu.org>
Subject: Re: [PATCH v2 5/5] acpi/generic_event_device: add logic to detect
if HEST addr is available
On Fri, 22 Nov 2024 14:14:15 +0100
Mauro Carvalho Chehab <mchehab+huawei@...nel.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@...nel.org>
Hi Mauro,
I think we still have a few inconsistencies in here on what can be
NULL and what can't. See inline.
With those tidied up.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index b5e3e2891445..a10f7e266a6a 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;
I don't think you need to set this any more.
> 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;
if (!ags) which the above check implies can happen, then
boom. So I think you can just drop the check on ags above.
The current caller can never pass that in as NULL anyway and it would
make no sense to call this function with it as NULL.
> }
> @@ -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 bd5582bc75f8..46ce3f3bb07a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -893,6 +893,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_SYNC, ACPI_GHES_NOTIFY_SEA },
> +};
> +
> static
> void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> {
> @@ -946,10 +950,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;
Could push down to the scope of if (acpi_ged_state)
I don't think it really matters though.
> + 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) {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a35c4a8faecb..00521a1963ba 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_1[] = {
> { TYPE_PCI_DEVICE, "x-pcie-ext-tag", "false" },
> + { TYPE_ACPI_GED, "x-has-hest-addr", "false" },
> };
> const size_t hw_compat_9_1_len = G_N_ELEMENTS(hw_compat_9_1);
>
> 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;
>
> /*
Powered by blists - more mailing lists