On Thu, 28 Dec 2017 13:54:11 +0800 Dongjiu Geng <gengdongjiu@xxxxxxxxxx> wrote: > This implements APEI GHES Table generation and record CPER in > runtime via fw_cfg blobs.Now we only support two types of GHESv2, > which are GPIO-Signal and ARMv8 SEA. Afterwards, we can extend the > supported type if needed. For the CPER section type, currently it s/type/types/ > is memory section because kernel manly wants userspace to handle s/manly/mainly/ > the memory errors. >In order to simulation, we hard code the error > type to Multi-bit ECC. Not sure what this is about, care to elaborate? > For GHESv2 error source, the OSPM must acknowledges the error via > Read ACK register. So user space must check the ACK value before > recording a new CPER to avoid read-write race condition. > > Suggested-by: Laszlo Ersek <lersek@xxxxxxxxxx> > Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx> > --- > The basic solution is suggested by Laszlo in [1] > [1]: https://lkml.org/lkml/2017/3/29/342 patch is too big, it would be better if it were split in several parts. > --- > hw/acpi/aml-build.c | 2 + > hw/acpi/hest_ghes.c | 390 ++++++++++++++++++++++++++++++++++++++++++++ > hw/arm/virt-acpi-build.c | 8 + > include/hw/acpi/aml-build.h | 1 + > include/hw/acpi/hest_ghes.h | 82 ++++++++++ > 5 files changed, 483 insertions(+) > create mode 100644 hw/acpi/hest_ghes.c > create mode 100644 include/hw/acpi/hest_ghes.h > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index 36a6cc4..6849e5f 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -1561,6 +1561,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables) > tables->table_data = g_array_new(false, true /* clear */, 1); > tables->tcpalog = g_array_new(false, true /* clear */, 1); > tables->vmgenid = g_array_new(false, true /* clear */, 1); > + tables->hardware_errors = g_array_new(false, true /* clear */, 1); > tables->linker = bios_linker_loader_init(); > } > > @@ -1571,6 +1572,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre) > g_array_free(tables->table_data, true); > g_array_free(tables->tcpalog, mfre); > g_array_free(tables->vmgenid, mfre); > + g_array_free(tables->hardware_errors, mfre); > } > > /* Build rsdt table */ > diff --git a/hw/acpi/hest_ghes.c b/hw/acpi/hest_ghes.c > new file mode 100644 > index 0000000..86ec99e > --- /dev/null > +++ b/hw/acpi/hest_ghes.c > @@ -0,0 +1,390 @@ > +/* Support for generating APEI tables and record CPER for Guests > + * > + * Copyright (C) 2017 HuaWei Corporation. > + * > + * Author: Dongjiu Geng <gengdongjiu@xxxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include "qemu/osdep.h" > +#include "hw/acpi/acpi.h" > +#include "hw/acpi/aml-build.h" > +#include "hw/acpi/hest_ghes.h" > +#include "hw/nvram/fw_cfg.h" > +#include "sysemu/sysemu.h" > +#include "qemu/error-report.h" > + > +/* Generic Address Structure (GAS) > + * ACPI 2.0/3.0: 5.2.3.1 Generic Address Structure > + * 2.0 compat note: > + * @access_width must be 0, see ACPI 2.0:Table 5-1 > + */ > +static void build_append_gas(GArray *table, AmlRegionSpace as, > + uint8_t bit_width, uint8_t bit_offset, > + uint8_t access_width, uint64_t address) > +{ > + build_append_int_noprefix(table, as, 1); > + build_append_int_noprefix(table, bit_width, 1); > + build_append_int_noprefix(table, bit_offset, 1); > + build_append_int_noprefix(table, access_width, 1); > + build_append_int_noprefix(table, address, 8); > +} all build_append_foo() primitives should go into aml-build.c you can reuse take following patches for build_append_gas() from my github repo https://github.com/imammedo/qemu/commit/ff35b4ae1ec286f5f11830d504467f5ad243995b https://github.com/imammedo/qemu/commit/3d2fd6d13a3ea298d2ee814835495ce6241d085c > +/* Hardware Error Notification > + * ACPI 4.0: 17.3.2.7 Hardware Error Notification > + */ > +static void build_append_notify(GArray *table, const uint8_t type, > + uint8_t length) > +{ > + build_append_int_noprefix(table, type, 1); /* type */ > + build_append_int_noprefix(table, length, 1); > + build_append_int_noprefix(table, 0, 26); > +} split all build_append_foo() into separate patches /a patch per function/, probably for GHES related primitives it would be better use 'ghes' domain prefix in function names, like s/build_append_notify/build_append_ghes_notify/ the same applies to other build_append_foo() below > +/* Generic Error Status Block > + * ACPI 4.0: 17.3.2.6.1 Generic Error Data > + */ > +static void build_append_gesb_header(GArray *table, uint32_t block_status, > + uint32_t raw_data_offset, uint32_t raw_data_length, > + uint32_t data_length, uint32_t error_severity) > +{ > + build_append_int_noprefix(table, block_status, 4); > + build_append_int_noprefix(table, raw_data_offset, 4); > + build_append_int_noprefix(table, raw_data_length, 4); > + build_append_int_noprefix(table, data_length, 4); > + build_append_int_noprefix(table, error_severity, 4); > +} > + > +/* Generic Error Data Entry > + * ACPI 4.0: 17.3.2.6.1 Generic Error Data > + */ > +static void build_append_gede_header(GArray *table, const char *section_type, > + const uint32_t error_severity, const uint16_t revision, > + const uint32_t error_data_length) > +{ > + int i; > + > + for (i = 0; i < 16; i++) { > + build_append_int_noprefix(table, section_type[i], 1); > + } > + > + build_append_int_noprefix(table, error_severity, 4); > + build_append_int_noprefix(table, revision, 2); > + build_append_int_noprefix(table, 0, 2); > + build_append_int_noprefix(table, error_data_length, 4); > + build_append_int_noprefix(table, 0, 44); > +} > + > +/* Memory section CPER record */ comment missing reference to related spec item > +static void build_append_mem_cper(GArray *table, uint64_t error_physical_addr) > +{ > + /* > + * Memory Error Record > + */ > + build_append_int_noprefix(table, > + (1UL << 14) | /* Type Valid */ > + (1UL << 1) /* Physical Address Valid */, > + 8); > + /* Memory error status information */ > + build_append_int_noprefix(table, 0, 8); > + /* The physical address at which the memory error occurred */ > + build_append_int_noprefix(table, error_physical_addr, 8); > + build_append_int_noprefix(table, 0, 48); > + /* Hard code to Multi-bit ECC error */ > + build_append_int_noprefix(table, 3 /* Multi-bit ECC */, 1); > + build_append_int_noprefix(table, 0, 7); > +} > + > +static int ghes_record_mem_error(uint64_t error_block_address, > + uint64_t error_physical_addr) probably function should be part of 9/9 patch, as the others that's called only from ghes_record_errors(). i.e. split this patch into acpi tables generation and actual error generation patches. > +{ > + GArray *block; > + uint64_t current_block_length; > + uint32_t data_length; > + /* Memory section */ > + char mem_section_id_le[] = {0x14, 0x11, 0xBC, 0xA5, 0x64, 0x6F, 0xDE, > + 0x4E, 0xB8, 0x63, 0x3E, 0x83, 0xED, 0x7C, > + 0x83, 0xB1}; > + > + block = g_array_new(false, true /* clear */, 1); > + > + /* Read the current length in bytes of the generic error data */ > + cpu_physical_memory_read(error_block_address + > + offsetof(AcpiGenericErrorStatus, data_length), &data_length, 4); > + > + /* The current whole length in bytes of the generic error status block */ > + current_block_length = sizeof(AcpiGenericErrorStatus) + data_length; missing le32_to_cpu() here also check other sites where you call cpu_physical_memory_foo() it would be good to have a 'make check' test case included in this series, then when you can spot these errors during test on BE host, before posting patches. > + > + /* Add a new generic error data entry*/ > + data_length += GHES_DATA_LENGTH; > + data_length += GHES_CPER_LENGTH; > + > + /* Check whether it will run out of the preallocated memory if adding a new > + * generic error data entry > + */ > + if ((data_length + sizeof(AcpiGenericErrorStatus)) > GHES_MAX_RAW_DATA_LENGTH) { > + error_report("Record CPER out of boundary!!!"); > + return GHES_CPER_FAIL; you return values here and from ghes_record_errors() but not actually handle them, so it's rather pointless thing to do, more over it's called on nofail path so one should either abort on error o ignore it > + } > + /* Build the new generic error status block header */ > + build_append_gesb_header(block, cpu_to_le32(ACPI_GEBS_UNCORRECTABLE), 0, 0, > + cpu_to_le32(data_length), cpu_to_le32(ACPI_CPER_SEV_RECOVERABLE)); > + > + /* Write back above generic error status block header to guest memory */ > + cpu_physical_memory_write(error_block_address, block->data, > + block->len); > + > + /* Build the generic error data entries */ > + > + data_length = block->len; > + /* Build the new generic error data entry header */ > + build_append_gede_header(block, mem_section_id_le, > + cpu_to_le32(ACPI_CPER_SEV_RECOVERABLE), cpu_to_le32(0x300), > + cpu_to_le32(80)/* the total size of Memory Error Record */); > + > + /* Build the memory section CPER */ > + build_append_mem_cper(block, error_physical_addr); > + > + /* Write back above whole new generic error data entry to guest memory */ > + cpu_physical_memory_write(error_block_address + current_block_length, > + block->data + data_length, block->len - data_length); > + > + g_array_free(block, true); > + > + return GHES_CPER_OK; > +} [...] I'll try to make another round of review on this once patch is split
