Message-ID: <20240824021510.71451b57@sal.lan>
Date: Sat, 24 Aug 2024 02:15:10 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Igor Mammedov <imammedo@...hat.com>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Shiju Jose
 <shiju.jose@...wei.com>, "Michael S. Tsirkin" <mst@...hat.com>, Ani Sinha
 <anisinha@...hat.com>, Dongjiu Geng <gengdongjiu1@...il.com>,
 linux-kernel@...r.kernel.org, qemu-arm@...gnu.org, qemu-devel@...gnu.org
Subject: Re: [PATCH v8 13/13] acpi/ghes: check if the BIOS pointers for HEST
 are correct

Em Mon, 19 Aug 2024 16:07:33 +0200
Igor Mammedov <imammedo@...hat.com> escreveu:

> > +    err_source_struct = le64_to_cpu(ags->hest_addr_le) +
> > +                        source * HEST_GHES_V2_TABLE_SIZE;  
> 
> there is no guaranties that HEST table will contain only GHESv2 sources,
> and once such is added this place becomes broken.
> 
> we need to iterate over HEST taking that into account
> and find only ghesv2 structure with source id of interest.
> 
> This function (and acpi_ghes_record_errors() as well) taking source_id
> as input should be able to lookup pointers from HEST in guest RAM,
> very crude idea could look something like this:
> 
> typedef struct hest_source_type2len{
>    uint16_t type
>    int len
> } hest_structure_type2len
> 
> hest_structure_type2len supported_hest_sources[] = {
>     /* Table 18-344 Generic Hardware Error Source version 2 (GHESv2) Structure */
>     {.type = 10, .len = 92},
> }

Sounds interesting, but IMO it should be done only when other types besides
ghes would be added, as:

1. Right now, the file is acpi/ghes.c. Adding non-type 10 HEST structures
   there would be a little weird. It should likely be renamed to acpi/hest.c
   when such time comes.

2. ACPI 6.5 has made clear that the above will only work up to type 11,
   as, from type 12 and above, the length will be added to the error
   struct, according with:

   https://uefi.org/specs/ACPI/6.5/18_Platform_Error_Interfaces.html#error-source-structure-header-type-12-onward

3. some types have variable size. Starting from the beginning, type 0, as
   defined at:
   https://uefi.org/specs/ACPI/6.5/18_Platform_Error_Interfaces.html#hardware-errors-and-error-sources

   has:

   size = 40 + 24 * Number of Hardware banks

   So, a simple table like the above with fixed sizes won't work.

   The code would need instead a switch if types are <= 11.

   Adding proper support for all already defined 12 types sounds lots of 
   work, as the code would need to calculate the size depending on the
   size, and we don't really initialize the HEST table with other types
   but GHES.

Ok, we could still do something like this pseudo-code to get the
error source offset:

	#define ACPI_HEST_TYPE_GHESV2	11

	err_struct_offset = 0;
	for (i = 0; i < source_id_count; i++) {
		/* NOTE: Other types may have different sizes */
		assert(ghes[i].type == ACPI_HEST_TYPE_GHESV2);
		if (ghes[i].source_id == source_id)
			break;
		err_struct_offset += HEST_GHES_V2_TABLE_SIZE;
	}
	assert (i < source_id_count);

---

That's said, maybe this will just add unwanted complexity, as QEMU
is already setting those offsets via bios_linker_loader_add_pointer().

So, an alternative for that is to merge the code on patch 13 with the one
on patch 5, dropping the math calcus there and relying that QEMU will
always handle properly bios links.

See, the logic which constructs GHESv2 source IDs do this to create
the links between HEST ACPI table and etc/hardware_errors:

with:

Per-source ID logic at build_ghes_v2():

    address_offset = table_data->len;
    /* Error Status Address */
    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0,
                     4 /* QWord access */, 0);
    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
                                   address_offset + GAS_ADDR_OFFSET,
                                   sizeof(uint64_t),
                                   ACPI_HW_ERROR_FW_CFG_FILE,
                                   source_id * sizeof(uint64_t));
...
    /*
     * Read Ack Register
     * ACPI 6.1: 18.3.2.8 Generic Hardware Error Source
     * version 2 (GHESv2 - Type 10)
     */
    address_offset = table_data->len;
    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0,
                     4 /* QWord access */, 0);
    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
                                   address_offset + GAS_ADDR_OFFSET,
                                   sizeof(uint64_t),
                                   ACPI_HW_ERROR_FW_CFG_FILE,
                                   (ACPI_HEST_SRC_ID_COUNT + source_id) *
                                   sizeof(uint64_t));

HEST table creation logic inside build_ghes_error_table():

    for (i = 0; i < ACPI_HEST_SRC_ID_COUNT; i++) {
        /*
         * Tell firmware to patch error_block_address entries to point to
         * corresponding "Generic Error Status Block"
         */
        bios_linker_loader_add_pointer(linker,
            ACPI_HW_ERROR_FW_CFG_FILE, sizeof(uint64_t) * i,
            sizeof(uint64_t), ACPI_HW_ERROR_FW_CFG_FILE,
            error_status_block_offset + i * ACPI_GHES_MAX_RAW_DATA_LENGTH);
    }

Using those, the location of the CPER and ack addresses is easy and won't
require any math:

	/* GHESv2 CPER 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(error_block_addr));

	/* GHESv2 ack offset */
	cpu_physical_memory_read(hest_read_ack_start_addr, &read_ack_start_addr,
			         sizeof(read_ack_start_addr));


Regards,
Mauro

Powered by blists - more mailing lists