| 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: <20250226164851.316aa095@imammedo.users.ipa.redhat.com>
Date: Wed, 26 Feb 2025 16:48:51 +0100
From: Igor Mammedov <imammedo@...hat.com>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: "Michael S . Tsirkin" <mst@...hat.com>, Jonathan Cameron
<Jonathan.Cameron@...wei.com>, Shiju Jose <shiju.jose@...wei.com>,
qemu-arm@...gnu.org, qemu-devel@...gnu.org, Ani Sinha
<anisinha@...hat.com>, Dongjiu Geng <gengdongjiu1@...il.com>, Peter Maydell
<peter.maydell@...aro.org>, Shannon Zhao <shannon.zhaosl@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 04/14] acpi/ghes: don't hard-code the number of
sources for HEST table
On Fri, 21 Feb 2025 15:35:13 +0100
Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:
> The current code is actually dependent on having just one error
> structure with a single source, as any change there would cause
> migration issues.
>
> As the number of sources should be arch-dependent, as it will depend on
> what kind of notifications will exist, and how many errors can be
> reported at the same time, change the logic to be more flexible,
> allowing the number of sources to be defined when building the
> HEST table by the caller.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Reviewed-by: Igor Mammedov <imammedo@...hat.com>
> ---
> hw/acpi/ghes.c | 38 +++++++++++++++++++++-----------------
> hw/arm/virt-acpi-build.c | 8 +++++++-
> include/hw/acpi/ghes.h | 17 ++++++++++++-----
> 3 files changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 7efea519f766..4a4ea8f4be90 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -238,17 +238,17 @@ ghes_gen_err_data_uncorrectable_recoverable(GArray *block,
> * See docs/specs/acpi_hest_ghes.rst for blobs format.
> */
> static void build_ghes_error_table(AcpiGhesState *ags, GArray *hardware_errors,
> - BIOSLinker *linker)
> + BIOSLinker *linker, int num_sources)
> {
> int i, error_status_block_offset;
>
> /* Build error_block_address */
> - for (i = 0; i < ACPI_GHES_ERROR_SOURCE_COUNT; i++) {
> + for (i = 0; i < num_sources; i++) {
> build_append_int_noprefix(hardware_errors, 0, sizeof(uint64_t));
> }
>
> /* Build read_ack_register */
> - for (i = 0; i < ACPI_GHES_ERROR_SOURCE_COUNT; i++) {
> + for (i = 0; i < num_sources; i++) {
> /*
> * Initialize the value of read_ack_register to 1, so GHES can be
> * writable after (re)boot.
> @@ -263,13 +263,13 @@ static void build_ghes_error_table(AcpiGhesState *ags, GArray *hardware_errors,
>
> /* Reserve space for Error Status Data Block */
> acpi_data_push(hardware_errors,
> - ACPI_GHES_MAX_RAW_DATA_LENGTH * ACPI_GHES_ERROR_SOURCE_COUNT);
> + ACPI_GHES_MAX_RAW_DATA_LENGTH * num_sources);
>
> /* Tell guest firmware to place hardware_errors blob into RAM */
> bios_linker_loader_alloc(linker, ACPI_HW_ERROR_FW_CFG_FILE,
> hardware_errors, sizeof(uint64_t), false);
>
> - for (i = 0; i < ACPI_GHES_ERROR_SOURCE_COUNT; i++) {
> + for (i = 0; i < num_sources; i++) {
> /*
> * Tell firmware to patch error_block_address entries to point to
> * corresponding "Generic Error Status Block"
> @@ -295,12 +295,14 @@ static void build_ghes_error_table(AcpiGhesState *ags, GArray *hardware_errors,
> }
>
> /* Build Generic Hardware Error Source version 2 (GHESv2) */
> -static void build_ghes_v2(GArray *table_data,
> - BIOSLinker *linker,
> - enum AcpiGhesNotifyType notify,
> - uint16_t source_id)
> +static void build_ghes_v2_entry(GArray *table_data,
> + BIOSLinker *linker,
> + const AcpiNotificationSourceId *notif_src,
> + uint16_t index, int num_sources)
> {
> uint64_t address_offset;
> + const uint16_t notify = notif_src->notify;
> + const uint16_t source_id = notif_src->source_id;
>
> /*
> * Type:
> @@ -331,7 +333,7 @@ static void build_ghes_v2(GArray *table_data,
> address_offset + GAS_ADDR_OFFSET,
> sizeof(uint64_t),
> ACPI_HW_ERROR_FW_CFG_FILE,
> - source_id * sizeof(uint64_t));
> + index * sizeof(uint64_t));
>
> /* Notification Structure */
> build_ghes_hw_error_notification(table_data, notify);
> @@ -351,8 +353,7 @@ static void build_ghes_v2(GArray *table_data,
> address_offset + GAS_ADDR_OFFSET,
> sizeof(uint64_t),
> ACPI_HW_ERROR_FW_CFG_FILE,
> - (ACPI_GHES_ERROR_SOURCE_COUNT + source_id)
> - * sizeof(uint64_t));
> + (num_sources + index) * sizeof(uint64_t));
>
> /*
> * Read Ack Preserve field
> @@ -368,22 +369,26 @@ static void build_ghes_v2(GArray *table_data,
> void acpi_build_hest(AcpiGhesState *ags, GArray *table_data,
> GArray *hardware_errors,
> BIOSLinker *linker,
> + const AcpiNotificationSourceId *notif_source,
> + int num_sources,
> const char *oem_id, const char *oem_table_id)
> {
> AcpiTable table = { .sig = "HEST", .rev = 1,
> .oem_id = oem_id, .oem_table_id = oem_table_id };
> uint32_t hest_offset;
> + int i;
>
> hest_offset = table_data->len;
>
> - build_ghes_error_table(ags, hardware_errors, linker);
> + build_ghes_error_table(ags, hardware_errors, linker, num_sources);
>
> acpi_table_begin(&table, table_data);
>
> /* Error Source Count */
> - build_append_int_noprefix(table_data, ACPI_GHES_ERROR_SOURCE_COUNT, 4);
> - build_ghes_v2(table_data, linker,
> - ACPI_GHES_NOTIFY_SEA, ACPI_HEST_SRC_ID_SEA);
> + build_append_int_noprefix(table_data, num_sources, 4);
> + for (i = 0; i < num_sources; i++) {
> + build_ghes_v2_entry(table_data, linker, ¬if_source[i], i, num_sources);
> + }
>
> acpi_table_end(linker, &table);
>
> @@ -529,7 +534,6 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> }
> ags = &acpi_ged_state->ghes_state;
>
> - assert(ACPI_GHES_ERROR_SOURCE_COUNT == 1);
I'd also remove one blank line here
>
> if (!ags->use_hest_addr) {
> get_hw_error_offsets(le64_to_cpu(ags->hw_error_le),
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 8ab8d11b6536..4439252e1a75 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -893,6 +893,10 @@ static void acpi_align_size(GArray *blob, unsigned align)
> g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align));
> }
>
> +static const AcpiNotificationSourceId hest_ghes_notify[] = {
> + { ACPI_HEST_SRC_ID_SYNC, ACPI_GHES_NOTIFY_SEA },
> +};
> +
> static
> void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> {
> @@ -956,7 +960,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>
> acpi_add_table(table_offsets, tables_blob);
> acpi_build_hest(ags, tables_blob, tables->hardware_errors,
> - tables->linker, vms->oem_id, vms->oem_table_id);
> + tables->linker, hest_ghes_notify,
> + ARRAY_SIZE(hest_ghes_notify),
> + vms->oem_id, vms->oem_table_id);
> }
> }
>
> diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
> index 2f06e433ce04..51c6b6b33327 100644
> --- a/include/hw/acpi/ghes.h
> +++ b/include/hw/acpi/ghes.h
> @@ -57,13 +57,18 @@ enum AcpiGhesNotifyType {
> ACPI_GHES_NOTIFY_RESERVED = 12
> };
>
> -enum {
> - ACPI_HEST_SRC_ID_SEA = 0,
> - /* future ids go here */
> -
> - ACPI_GHES_ERROR_SOURCE_COUNT
> +/*
> + * ID numbers used to fill HEST source ID field
> + */
> +enum AcpiGhesSourceID {
> + ACPI_HEST_SRC_ID_SYNC,
> };
>
> +typedef struct AcpiNotificationSourceId {
> + enum AcpiGhesSourceID source_id;
> + enum AcpiGhesNotifyType notify;
> +} AcpiNotificationSourceId;
> +
> /*
> * AcpiGhesState stores an offset that will be used to fill HEST entries.
> *
> @@ -86,6 +91,8 @@ typedef struct AcpiGhesState {
> void acpi_build_hest(AcpiGhesState *ags, GArray *table_data,
> GArray *hardware_errors,
> BIOSLinker *linker,
> + const AcpiNotificationSourceId * const notif_source,
> + int num_sources,
> const char *oem_id, const char *oem_table_id);
> void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s,
> GArray *hardware_errors);
Powered by blists - more mailing lists