* [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
@ 2017-04-30 5:35 Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 2/4] target-arm: kvm64: detect guest RAS EXTENSION feature Dongjiu Geng
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Dongjiu Geng @ 2017-04-30 5:35 UTC (permalink / raw)
To: james.morse, marc.zyngier, christoffer.dall, rkrcmar, linux,
tbaicar, imammedo, zhaoshenglong, peter.maydell, pbonzini,
qemu-devel, qemu-arm, lersek, ard.biesheuvel, mtsirkin, drjones,
ben, kvm, linux-arm-kernel, kvmarm
Cc: xiexiuqi, wangxiongfeng2, songwenjun, wuquanming, huangshaoyu,
gengdongjiu
This implements APEI GHES Table by passing the error cper info
to the guest via a fw_cfg_blob. After a CPER info is added, an
SEA/SEI exception will be injected into the guest OS.
Below is the table layout, the max number of error soure is 11,
which is classified by notification type.
etc/acpi/tables etc/hardware_errors
================ ==========================================
+-----------+
+--------------+ | address | +-> +--------------+
| HEST + | registers | | | Error Status |
+ +------------+ | +---------+ | | Data Block 1 |
| | GHES1 | --> | |address1 | --------+ | +------------+
| | GHES2 | --> | |address2 | ------+ | | CPER |
| | GHES3 | --> | |address3 | ----+ | | | CPER |
| | .... | --> | | ....... | | | | | CPER |
| | GHES10 | --> | |address10| -+ | | | | CPER |
+-+------------+ +-+---------+ | | | +-+------------+
| | |
| | +---> +--------------+
| | | Error Status |
| | | Data Block 2 |
| | | +------------+
| | | | CPER |
| | | | CPER |
| | +-+------------+
| |
| +-----> +--------------+
| | Error Status |
| | Data Block 3 |
| | +------------+
| | | CPER |
| +-+------------+
| ...........
+--------> +--------------+
| Error Status |
| Data Block 10|
| +------------+
| | CPER |
| | CPER |
| | CPER |
+-+------------+
Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
default-configs/arm-softmmu.mak | 1 +
hw/acpi/Makefile.objs | 1 +
hw/acpi/aml-build.c | 2 +
hw/acpi/hest_ghes.c | 203 +++++++++++++++++++++++++++++++++++
hw/arm/virt-acpi-build.c | 6 ++
include/hw/acpi/acpi-defs.h | 227 ++++++++++++++++++++++++++++++++++++++++
include/hw/acpi/aml-build.h | 1 +
include/hw/acpi/hest_ghes.h | 43 ++++++++
8 files changed, 484 insertions(+)
create mode 100644 hw/acpi/hest_ghes.c
create mode 100644 include/hw/acpi/hest_ghes.h
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 1e3bd2b..d5f1552 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -121,3 +121,4 @@ CONFIG_ACPI=y
CONFIG_SMBIOS=y
CONFIG_ASPEED_SOC=y
CONFIG_GPIO_KEY=y
+CONFIG_ACPI_APEI_GENERATION=y
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 11c35bc..776b46e 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
+common-obj-$(CONFIG_ACPI_APEI_GENERATION) += hest_ghes.o
common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
common-obj-y += acpi_interface.o
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index c6f2032..802b98d 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1560,6 +1560,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();
}
@@ -1570,6 +1571,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..91d382e
--- /dev/null
+++ b/hw/acpi/hest_ghes.c
@@ -0,0 +1,203 @@
+/*
+ * APEI GHES table Generation
+ *
+ * Copyright (C) 2017 huawei.
+ *
+ * Author: Dongjiu Geng <gengdongjiu@huawei.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qmp-commands.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"
+
+static int ghes_generate_cper_record(uint64_t block_error_address,
+ uint64_t error_physical_addr)
+{
+ AcpiGenericErrorStatus block;
+ AcpiGenericErrorData *gdata;
+ struct cper_sec_mem_err *mem_err;
+ uint64_t block_data_length;
+ unsigned char *buffer;
+
+ cpu_physical_memory_read(block_error_address, &block,
+ sizeof(AcpiGenericErrorStatus));
+
+ block_data_length = sizeof(AcpiGenericErrorStatus) + block.data_length;
+
+ /* If the Generic Error Status Block is NULL, update
+ * the block header
+ */
+ if (!block.block_status) {
+ block.block_status = ACPI_BERT_UNCORRECTABLE;
+ block.error_severity = CPER_SEV_FATAL;
+ }
+
+ block.data_length += sizeof(AcpiGenericErrorData);
+ block.data_length += sizeof(struct cper_sec_mem_err);
+
+ /* Write back the Generic Error Status Block to guest memory */
+ cpu_physical_memory_write(block_error_address, &block,
+ sizeof(AcpiGenericErrorStatus));
+
+ /* Fill in Generic Error Data Entry */
+ buffer = g_malloc(sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
+ memset(buffer, 0, sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
+ gdata = (AcpiGenericErrorData *)buffer;
+
+ memcpy(gdata->section_type, (void *) &CPER_SEC_PLATFORM_MEM,
+ sizeof(uuid_le));
+ gdata->error_data_length = sizeof(struct cper_sec_mem_err);
+
+ mem_err = (struct cper_sec_mem_err *) (gdata + 1);
+
+ /* In order to simplify simulation, hardcode the CPER section to memory
+ * section.
+ */
+ mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
+ mem_err->error_type = 3;
+
+ mem_err->validation_bits |= CPER_MEM_VALID_PA;
+ mem_err->physical_addr = error_physical_addr;
+
+ mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
+ CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
+ CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
+ mem_err->card = 1;
+ mem_err->module = 2;
+ mem_err->bank = 3;
+ mem_err->row = 1;
+ mem_err->column = 2;
+ mem_err->bit_pos = 5;
+
+ mem_err->validation_bits |= CPER_MEM_VALID_ERROR_STATUS;
+ mem_err->error_status = 4 << 8;
+
+ /* Write back the Generic Error Data Entry to guest memory */
+ cpu_physical_memory_write(block_error_address + block_data_length, buffer,
+ sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
+
+ g_free(buffer);
+ return BFAPEI_OK;
+}
+
+void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
+ BIOSLinker *linker)
+{
+ Aml *hest;
+ uint32_t address_registers_offset;
+ AcpiTableHeader *header;
+ AcpiGenericHardwareErrorSource *error_source;
+ int i;
+
+ int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
+
+ /* New address register and Error status block table size*/
+ g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6
+ * block_reqr_size);
+
+ /* Put this in a HEST table */
+ hest = init_aml_allocator();
+ address_registers_offset = table_data->len
+ + sizeof(AcpiHardwareErrorSourceTable)
+ + ERROR_STATUS_ADDRESS_OFFSET
+ + GAS_ADDRESS_OFFSET;
+ /* Reserve space for HEST table size*/
+ acpi_data_push(hest->buf, sizeof(AcpiHardwareErrorSourceTable)
+ + MAX_ERROR_SOURCE_COUNT_V6
+ * sizeof(AcpiGenericHardwareErrorSource));
+
+ g_array_append_vals(table_data, hest->buf->data, hest->buf->len);
+ /* Allocate guest memory for the Data fw_cfg blob */
+ bios_linker_loader_alloc(linker, GHES_ERRORS_FW_CFG_FILE,
+ hardware_error, 4096,
+ false /* page boundary, high memory */);
+ header = (AcpiTableHeader *)(table_data->data
+ + table_data->len - hest->buf->len);
+ *(uint32_t *)(header + 1) = MAX_ERROR_SOURCE_COUNT_V6;
+ error_source = (AcpiGenericHardwareErrorSource *)((char *)header
+ + sizeof(AcpiHardwareErrorSourceTable));
+
+ for (i = 0; i < MAX_ERROR_SOURCE_COUNT_V6; i++) {
+ error_source->type = ACPI_HEST_TYPE_GENERIC_ERROR;
+ error_source->source_id = 0;
+ error_source->related_source_id = 0xffff;
+ error_source->flags = 0;
+ error_source->enabled = 1;
+ error_source->number_of_records = 1;
+ error_source->max_sections_per_record = 1;
+ error_source->max_raw_data_length = MAX_RAW_DATA_LENGTH;
+ error_source->error_status_address.space_id =
+ ACPI_ADR_SPACE_SYSTEM_MEMORY;
+ error_source->error_status_address.bit_width = 64;
+ error_source->error_status_address.bit_offset = 0;
+ error_source->error_status_address.access_width = 4;
+ error_source->notify.type = i;
+ error_source->notify.length = sizeof(AcpiGenericHardwareErrorSource);
+
+ bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
+ sizeof(uint64_t) * i, sizeof(uint64_t),
+ GHES_ERRORS_FW_CFG_FILE,
+ MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
+ i * MAX_RAW_DATA_LENGTH);
+ bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
+ address_registers_offset
+ + i * sizeof(AcpiGenericHardwareErrorSource),
+ sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
+ i * sizeof(uint64_t));
+
+ bios_linker_loader_write_pointer(linker, GHES_DATA_ADDR_FW_CFG_FILE,
+ i * sizeof(uint64_t), sizeof(uint64_t),
+ GHES_ERRORS_FW_CFG_FILE,
+ MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
+ i * MAX_RAW_DATA_LENGTH);
+ error_source++;
+ }
+
+ build_header(linker, table_data,
+ (void *)header, "HEST", hest->buf->len, 1, NULL, "GHES");
+
+ free_aml_allocator();
+}
+
+static GhesErrorState ges;
+void ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_error)
+{
+
+ int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
+ int size = MAX_ERROR_SOURCE_COUNT_V6 * block_reqr_size;
+
+ /* Create a read-only fw_cfg file for GHES */
+ fw_cfg_add_file(s, GHES_ERRORS_FW_CFG_FILE, hardware_error->data,
+ size);
+ /* Create a read-write fw_cfg file for Address */
+ fw_cfg_add_file_callback(s, GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL,
+ &(ges.ghes_addr_le[0]),
+ sizeof(uint64_t) * MAX_ERROR_SOURCE_COUNT_V6,
+ false);
+}
+
+void ghes_update_guest(uint32_t notify, uint64_t physical_address)
+{
+ uint64_t block_error_addr;
+
+ if (physical_address) {
+ ges.physical_addr = physical_address;
+ block_error_addr = ges.ghes_addr_le[notify];
+ block_error_addr = le32_to_cpu(block_error_addr);
+
+ /* A zero value in ghes_addr means that BIOS has not yet written
+ * the address
+ */
+ if (block_error_addr) {
+ ghes_generate_cper_record(block_error_addr, physical_address);
+ }
+ }
+}
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 0835e59..e7ab5dc 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -45,6 +45,8 @@
#include "hw/arm/virt.h"
#include "sysemu/numa.h"
#include "kvm_arm.h"
+#include "hw/acpi/vmgenid.h"
+#include "hw/acpi/hest_ghes.h"
#define ARM_SPI_BASE 32
#define ACPI_POWER_BUTTON_DEVICE "PWRB"
@@ -778,6 +780,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_spcr(tables_blob, tables->linker, vms);
+ acpi_add_table(table_offsets, tables_blob);
+ ghes_build_acpi(tables_blob, tables->hardware_errors, tables->linker);
+
if (nb_numa_nodes > 0) {
acpi_add_table(table_offsets, tables_blob);
build_srat(tables_blob, tables->linker, vms);
@@ -892,6 +897,7 @@ void virt_acpi_setup(VirtMachineState *vms)
build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
ACPI_BUILD_RSDP_FILE, 0);
+ ghes_add_fw_cfg(vms->fw_cfg, tables.hardware_errors);
qemu_register_reset(virt_acpi_build_reset, build_state);
virt_acpi_build_reset(build_state);
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 4cc3630..27adede 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -295,6 +295,58 @@ typedef struct AcpiMultipleApicTable AcpiMultipleApicTable;
#define ACPI_APIC_GENERIC_TRANSLATOR 15
#define ACPI_APIC_RESERVED 16 /* 16 and greater are reserved */
+#define CPER_MEM_VALID_ERROR_STATUS 0x0001
+#define CPER_MEM_VALID_PA 0x0002
+#define CPER_MEM_VALID_PA_MASK 0x0004
+#define CPER_MEM_VALID_NODE 0x0008
+#define CPER_MEM_VALID_CARD 0x0010
+#define CPER_MEM_VALID_MODULE 0x0020
+#define CPER_MEM_VALID_BANK 0x0040
+#define CPER_MEM_VALID_DEVICE 0x0080
+#define CPER_MEM_VALID_ROW 0x0100
+#define CPER_MEM_VALID_COLUMN 0x0200
+#define CPER_MEM_VALID_BIT_POSITION 0x0400
+#define CPER_MEM_VALID_REQUESTOR_ID 0x0800
+#define CPER_MEM_VALID_RESPONDER_ID 0x1000
+#define CPER_MEM_VALID_TARGET_ID 0x2000
+#define CPER_MEM_VALID_ERROR_TYPE 0x4000
+#define CPER_MEM_VALID_RANK_NUMBER 0x8000
+#define CPER_MEM_VALID_CARD_HANDLE 0x10000
+#define CPER_MEM_VALID_MODULE_HANDLE 0x20000
+
+typedef struct {
+ uint8_t b[16];
+} uuid_le;
+
+#define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
+((uuid_le) \
+{{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
+ (b) & 0xff, ((b) >> 8) & 0xff, \
+ (c) & 0xff, ((c) >> 8) & 0xff, \
+ (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) } })
+
+/* Platform Memory */
+#define CPER_SEC_PLATFORM_MEM \
+ UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \
+ 0xED, 0x7C, 0x83, 0xB1)
+
+/* Values for Notify Type field above */
+
+enum acpi_hest_notify_types {
+ ACPI_HEST_NOTIFY_POLLED = 0,
+ ACPI_HEST_NOTIFY_EXTERNAL = 1,
+ ACPI_HEST_NOTIFY_LOCAL = 2,
+ ACPI_HEST_NOTIFY_SCI = 3,
+ ACPI_HEST_NOTIFY_NMI = 4,
+ ACPI_HEST_NOTIFY_CMCI = 5, /* ACPI 5.0 */
+ ACPI_HEST_NOTIFY_MCE = 6, /* ACPI 5.0 */
+ ACPI_HEST_NOTIFY_GPIO = 7, /* ACPI 6.0 */
+ ACPI_HEST_NOTIFY_SEA = 8, /* ACPI 6.1 */
+ ACPI_HEST_NOTIFY_SEI = 9, /* ACPI 6.1 */
+ ACPI_HEST_NOTIFY_GSIV = 10, /* ACPI 6.1 */
+ ACPI_HEST_NOTIFY_RESERVED = 11 /* 11 and greater are reserved */
+};
+
/*
* MADT sub-structures (Follow MULTIPLE_APIC_DESCRIPTION_TABLE)
*/
@@ -475,6 +527,181 @@ struct AcpiSystemResourceAffinityTable
} QEMU_PACKED;
typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
+#define ACPI_ADR_SPACE_SYSTEM_MEMORY (uint8_t) 0
+#define ACPI_ADR_SPACE_SYSTEM_IO (uint8_t) 1
+#define ACPI_ADR_SPACE_PCI_CONFIG (uint8_t) 2
+#define ACPI_ADR_SPACE_EC (uint8_t) 3
+#define ACPI_ADR_SPACE_SMBUS (uint8_t) 4
+#define ACPI_ADR_SPACE_CMOS (uint8_t) 5
+#define ACPI_ADR_SPACE_PCI_BAR_TARGET (uint8_t) 6
+#define ACPI_ADR_SPACE_IPMI (uint8_t) 7
+#define ACPI_ADR_SPACE_GPIO (uint8_t) 8
+#define ACPI_ADR_SPACE_GSBUS (uint8_t) 9
+#define ACPI_ADR_SPACE_PLATFORM_COMM (uint8_t) 10
+
+/* GAS - Generic Address Structure */
+struct acpi_generic_address {
+ uint8_t space_id; /* Address space where
+ *struct or register exists
+ */
+ uint8_t bit_width; /* Size in bits of given register */
+ uint8_t bit_offset; /* Bit offset within the register */
+ uint8_t access_width; /* Minimum Access size (ACPI 3.0) */
+ uint64_t address; /* 64-bit address of struct or register */
+} __attribute__ ((packed));
+
+/* Hardware Error Notification */
+struct acpi_hest_notify {
+ uint8_t type;
+ uint8_t length;
+ uint16_t config_write_enable;
+ uint32_t poll_interval;
+ uint32_t vector;
+ uint32_t polling_threshold_value;
+ uint32_t polling_threshold_window;
+ uint32_t error_threshold_value;
+ uint32_t error_threshold_window;
+};
+
+enum acpi_hest_types {
+ ACPI_HEST_TYPE_IA32_CHECK = 0,
+ ACPI_HEST_TYPE_IA32_CORRECTED_CHECK = 1,
+ ACPI_HEST_TYPE_IA32_NMI = 2,
+ ACPI_HEST_TYPE_NOT_USED3 = 3,
+ ACPI_HEST_TYPE_NOT_USED4 = 4,
+ ACPI_HEST_TYPE_NOT_USED5 = 5,
+ ACPI_HEST_TYPE_AER_ROOT_PORT = 6,
+ ACPI_HEST_TYPE_AER_ENDPOINT = 7,
+ ACPI_HEST_TYPE_AER_BRIDGE = 8,
+ ACPI_HEST_TYPE_GENERIC_ERROR = 9,
+ ACPI_HEST_TYPE_GENERIC_ERROR_V2 = 10,
+ ACPI_HEST_TYPE_RESERVED = 11 /* 11 and greater are reserved */
+};
+
+/* Values for block_status flags above */
+#define ACPI_BERT_UNCORRECTABLE (1)
+#define ACPI_BERT_CORRECTABLE (1 << 1)
+#define ACPI_BERT_MULTIPLE_UNCORRECTABLE (1 << 2)
+#define ACPI_BERT_MULTIPLE_CORRECTABLE (1 << 3)
+/* 8 bits, error count */
+#define ACPI_BERT_ERROR_ENTRY_COUNT (0xFF << 4)
+
+/* Generic Hardware Error Source Structure */
+struct AcpiGenericHardwareErrorSource {
+ uint16_t type;
+ uint16_t source_id;
+ uint16_t related_source_id;
+ uint8_t flags;
+ uint8_t enabled;
+ uint32_t number_of_records;
+ uint32_t max_sections_per_record;
+ uint32_t max_raw_data_length;
+ struct acpi_generic_address error_status_address;
+ struct acpi_hest_notify notify;
+ uint32_t error_status_block_length;
+} QEMU_PACKED;
+typedef struct AcpiGenericHardwareErrorSource AcpiGenericHardwareErrorSource;
+
+/* Generic Hardware Error Source , version 2 */
+struct AcpiGenericHardwareErrorSourceV2 {
+ uint16_t type;
+ uint16_t source_id;
+ uint16_t related_source_id;
+ uint8_t flags;
+ uint8_t enabled;
+ uint32_t number_of_records;
+ uint32_t max_sections_per_record;
+ uint32_t max_raw_data_length;
+ struct acpi_generic_address error_status_address;
+ struct acpi_hest_notify notify;
+ uint32_t error_status_block_length;
+ struct acpi_generic_address read_ack_register;
+ uint64_t read_ack_preserve;
+ uint64_t read_ack_write;
+} QEMU_PACKED;
+typedef struct AcpiGenericHardwareErrorSourceV2
+ AcpiGenericHardwareErrorSourceV2;
+
+/* Generic Error Status block */
+
+struct AcpiGenericErrorStatus {
+ uint32_t block_status;
+ uint32_t raw_data_offset;
+ uint32_t raw_data_length;
+ uint32_t data_length;
+ uint32_t error_severity;
+};
+typedef struct AcpiGenericErrorStatus AcpiGenericErrorStatus;
+
+/* Generic Error Data entry */
+
+struct AcpiGenericErrorData {
+ uint8_t section_type[16];
+ uint32_t error_severity;
+ uint16_t revision;
+ uint8_t validation_bits;
+ uint8_t flags;
+ uint32_t error_data_length;
+ uint8_t fru_id[16];
+ uint8_t fru_text[20];
+};
+typedef struct AcpiGenericErrorData AcpiGenericErrorData;
+
+/* Extension for revision 0x0300 */
+struct AcpiGenericErrorDataV300 {
+ uint8_t section_type[16];
+ uint32_t error_severity;
+ uint16_t revision;
+ uint8_t validation_bits;
+ uint8_t flags;
+ uint32_t error_data_length;
+ uint8_t fru_id[16];
+ uint8_t fru_text[20];
+ uint64_t time_stamp;
+};
+typedef struct AcpiGenericErrorDataV300 AcpiGenericErrorDataV300;
+
+enum {
+ CPER_SEV_RECOVERABLE,
+ CPER_SEV_FATAL,
+ CPER_SEV_CORRECTED,
+ CPER_SEV_INFORMATIONAL,
+};
+
+/* Memory Error Section */
+struct cper_sec_mem_err {
+ uint64_t validation_bits;
+ uint64_t error_status;
+ uint64_t physical_addr;
+ uint64_t physical_addr_mask;
+ uint16_t node;
+ uint16_t card;
+ uint16_t module;
+ uint16_t bank;
+ uint16_t device;
+ uint16_t row;
+ uint16_t column;
+ uint16_t bit_pos;
+ uint64_t requestor_id;
+ uint64_t responder_id;
+ uint64_t target_id;
+ uint8_t error_type;
+ uint8_t reserved;
+ uint16_t rank;
+ uint16_t mem_array_handle; /* card handle in UEFI 2.4 */
+ uint16_t mem_dev_handle; /* module handle in UEFI 2.4 */
+};
+ typedef struct cper_sec_mem_err cper_sec_mem_err;
+
+/*
+ * HEST Description Table
+ */
+struct AcpiHardwareErrorSourceTable {
+ ACPI_TABLE_HEADER_DEF /* ACPI common table header */
+ uint32_t error_source_count;
+} QEMU_PACKED;
+typedef struct AcpiHardwareErrorSourceTable AcpiHardwareErrorSourceTable;
+
#define ACPI_SRAT_PROCESSOR_APIC 0
#define ACPI_SRAT_MEMORY 1
#define ACPI_SRAT_PROCESSOR_x2APIC 2
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 00c21f1..c1d15b3 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -211,6 +211,7 @@ struct AcpiBuildTables {
GArray *rsdp;
GArray *tcpalog;
GArray *vmgenid;
+ GArray *hardware_errors;
BIOSLinker *linker;
} AcpiBuildTables;
diff --git a/include/hw/acpi/hest_ghes.h b/include/hw/acpi/hest_ghes.h
new file mode 100644
index 0000000..0cadc2b
--- /dev/null
+++ b/include/hw/acpi/hest_ghes.h
@@ -0,0 +1,43 @@
+#ifndef ACPI_GHES_H
+#define ACPI_GHES_H
+
+#include "hw/acpi/bios-linker-loader.h"
+
+#define GHES_ERRORS_FW_CFG_FILE "etc/hardware_errors"
+#define GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
+
+#define GAS_ADDRESS_OFFSET 4
+#define ERROR_STATUS_ADDRESS_OFFSET 20
+#define NOTIFICATION_STRUCTURE 32
+
+#define BFAPEI_OK 0
+#define BFAPEI_FAIL 1
+
+/* The max number of error source, the error sources
+ * are classified by notification type, below is the definition
+ * 0 - Polled
+ * 1 - External Interrupt
+ * 2 - Local Interrupt
+ * 3 - SCI
+ * 4 - NMI
+ * 5 - CMCI
+ * 6 - MCE
+ * 7 - GPIO-Signal
+ * 8 - ARMv8 SEA
+ * 9 - ARMv8 SEI
+ * 10 - External Interrupt - GSIV
+ */
+#define MAX_ERROR_SOURCE_COUNT_V6 11
+/* The max size in Bytes for one error block */
+#define MAX_RAW_DATA_LENGTH 0x1000
+
+typedef struct GhesErrorState {
+ uint64_t physical_addr;
+ uint64_t ghes_addr_le[8];
+} GhesErrorState;
+
+void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
+ BIOSLinker *linker);
+void ghes_add_fw_cfg(FWCfgState *s, GArray *guid);
+void ghes_update_guest(uint32_t notify, uint64_t physical_address);
+#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v3 2/4] target-arm: kvm64: detect guest RAS EXTENSION feature
2017-04-30 5:35 [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Dongjiu Geng
@ 2017-04-30 5:35 ` Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 3/4] target-arm: kvm64: handle SIGBUS signal for synchronous External Abort Dongjiu Geng
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Dongjiu Geng @ 2017-04-30 5:35 UTC (permalink / raw)
To: james.morse, marc.zyngier, christoffer.dall, rkrcmar, linux,
tbaicar, imammedo, zhaoshenglong, peter.maydell, pbonzini,
qemu-devel, qemu-arm, lersek, ard.biesheuvel, mtsirkin, drjones,
ben, kvm, linux-arm-kernel, kvmarm
Cc: xiexiuqi, wangxiongfeng2, songwenjun, wuquanming, huangshaoyu,
gengdongjiu
check if kvm supports guest RAS EXTENSION. if so, set
corresponding feature bit for vcpu.
Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
| 1 +
| 1 +
target/arm/cpu.h | 3 +++
target/arm/kvm64.c | 8 ++++++++
4 files changed, 13 insertions(+)
--git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index 651ec30..acb5c4d 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -97,6 +97,7 @@ struct kvm_regs {
#define KVM_ARM_VCPU_EL1_32BIT 1 /* CPU running a 32bit VM */
#define KVM_ARM_VCPU_PSCI_0_2 2 /* CPU uses PSCI v0.2 */
#define KVM_ARM_VCPU_PMU_V3 3 /* Support guest PMUv3 */
+#define KVM_ARM_VCPU_RAS_EXTENSION 4
struct kvm_vcpu_init {
__u32 target;
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 4e082a8..e0fa5b8 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_PPC_MMU_RADIX 134
#define KVM_CAP_PPC_MMU_HASH_V3 135
#define KVM_CAP_IMMEDIATE_EXIT 136
+#define KVM_CAP_ARM_RAS_EXTENSION 137
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a8aabce..1cf4a5b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -610,6 +610,8 @@ struct ARMCPU {
/* CPU has memory protection unit */
bool has_mpu;
+ /* CPU has ras extension unit */
+ bool has_ras_extension;
/* PMSAv7 MPU number of supported regions */
uint32_t pmsav7_dregion;
@@ -1216,6 +1218,7 @@ enum arm_features {
ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
ARM_FEATURE_PMU, /* has PMU support */
ARM_FEATURE_VBAR, /* has cp15 VBAR */
+ ARM_FEATURE_RAS_EXTENSION, /*has RAS extension support */
};
static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 6111109..fd30a5a 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -518,6 +518,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
unset_feature(&env->features, ARM_FEATURE_PMU);
}
+ if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_RAS_EXTENSION)) {
+ cpu->has_ras_extension = true;
+ cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_RAS_EXTENSION;
+ } else {
+ cpu->has_ras_extension = false;
+ unset_feature(&env->features, ARM_FEATURE_RAS_EXTENSION);
+ }
+
/* Do KVM_ARM_VCPU_INIT ioctl */
ret = kvm_arm_vcpu_init(cs);
if (ret) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v3 3/4] target-arm: kvm64: handle SIGBUS signal for synchronous External Abort
2017-04-30 5:35 [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 2/4] target-arm: kvm64: detect guest RAS EXTENSION feature Dongjiu Geng
@ 2017-04-30 5:35 ` Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 4/4] target-arm: kvm64: handle SError interrupt for RAS extension Dongjiu Geng
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Dongjiu Geng @ 2017-04-30 5:35 UTC (permalink / raw)
To: james.morse, marc.zyngier, christoffer.dall, rkrcmar, linux,
tbaicar, imammedo, zhaoshenglong, peter.maydell, pbonzini,
qemu-devel, qemu-arm, lersek, ard.biesheuvel, mtsirkin, drjones,
ben, kvm, linux-arm-kernel, kvmarm
Cc: xiexiuqi, wangxiongfeng2, songwenjun, wuquanming, huangshaoyu,
gengdongjiu
If RAS extension feature exists, when guest OS happen synchronous
External Abort, a SIGBUS signal will be delivered from the kvm.
so handle this signal, generate GHES record and inject this
abort to notify guest. then guest can handle this abort
The Synchronous External Abort mainly includes below two types:
1. Error on memory access, such as hwpoison. the SIGBUS si_code is
BUS_MCEERR_AR or BUS_MCEERR_OR
2. Error on translation table walk or hardware update of translation table.
The SIGBUS si_code is 0
Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
include/sysemu/kvm.h | 2 +-
| 3 +++
target/arm/kvm.c | 34 +++++++++++++++++++++++++++
target/arm/kvm64.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
target/arm/kvm_arm.h | 1 +
5 files changed, 99 insertions(+), 1 deletion(-)
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 24281fc..feef990 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -354,7 +354,7 @@ bool kvm_vcpu_id_is_valid(int vcpu_id);
/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
unsigned long kvm_arch_vcpu_id(CPUState *cpu);
-#ifdef TARGET_I386
+#if defined(TARGET_I386) || defined(TARGET_AARCH64)
#define KVM_HAVE_MCE_INJECTION 1
void kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
#endif
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index e0fa5b8..c985e10 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1301,6 +1301,9 @@ struct kvm_s390_ucas_mapping {
#define KVM_S390_GET_IRQ_STATE _IOW(KVMIO, 0xb6, struct kvm_s390_irq_state)
/* Available with KVM_CAP_X86_SMM */
#define KVM_SMI _IO(KVMIO, 0xb7)
+/* Available with KVM_CAP_ARM_RAS_EXTENSION */
+#define KVM_ARM_SEA _IO(KVMIO, 0xb8)
+
#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
#define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 4555468..bf6ee4c 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -129,6 +129,39 @@ void kvm_arm_destroy_scratch_host_vcpu(int *fdarray)
}
}
+typedef struct HWPoisonPage {
+ ram_addr_t ram_addr;
+ QLIST_ENTRY(HWPoisonPage) list;
+} HWPoisonPage;
+
+static QLIST_HEAD(, HWPoisonPage) hwpoison_page_list =
+ QLIST_HEAD_INITIALIZER(hwpoison_page_list);
+
+static void kvm_unpoison_all(void *param)
+{
+ HWPoisonPage *page, *next_page;
+
+ QLIST_FOREACH_SAFE(page, &hwpoison_page_list, list, next_page) {
+ QLIST_REMOVE(page, list);
+ qemu_ram_remap(page->ram_addr, TARGET_PAGE_SIZE);
+ g_free(page);
+ }
+}
+
+void kvm_hwpoison_page_add(ram_addr_t ram_addr)
+{
+ HWPoisonPage *page;
+
+ QLIST_FOREACH(page, &hwpoison_page_list, list) {
+ if (page->ram_addr == ram_addr) {
+ return;
+ }
+ }
+ page = g_new(HWPoisonPage, 1);
+ page->ram_addr = ram_addr;
+ QLIST_INSERT_HEAD(&hwpoison_page_list, page, list);
+}
+
static void kvm_arm_host_cpu_class_init(ObjectClass *oc, void *data)
{
ARMHostCPUClass *ahcc = ARM_HOST_CPU_CLASS(oc);
@@ -176,6 +209,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);
+ qemu_register_reset(kvm_unpoison_all, NULL);
type_register_static(&host_arm_cpu_type_info);
return 0;
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index fd30a5a..5eee6f4 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -27,6 +27,8 @@
#include "kvm_arm.h"
#include "internals.h"
#include "hw/arm/arm.h"
+#include "hw/acpi/acpi-defs.h"
+#include "hw/acpi/hest_ghes.h"
static bool have_guest_debug;
@@ -590,6 +592,21 @@ int kvm_arm_cpreg_level(uint64_t regidx)
return KVM_PUT_RUNTIME_STATE;
}
+static int kvm_inject_arm_sea(ARMCPU *cpu)
+{
+ CPUState *cs = CPU(cpu);
+ if (cs->exception_index == EXCP_DATA_ABORT) {
+ cs->exception_index = -1;
+ /* Here use a new ioctl KVM_ARM_SEA instead of KVM_SET_ONE_REG,
+ * Becuase kernel has direct injection interface, so simplify
+ * the logic to use kernel interface
+ */
+ return kvm_vcpu_ioctl(CPU(cpu), KVM_ARM_SEA);
+ }
+
+ return 0;
+}
+
#define AARCH64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
@@ -735,6 +752,13 @@ int kvm_arch_put_registers(CPUState *cs, int level)
return EINVAL;
}
+ if (arm_feature(env, ARM_FEATURE_RAS_EXTENSION)) {
+ ret = kvm_inject_arm_sea(cpu);
+ if (ret < 0) {
+ return ret;
+ }
+ }
+
kvm_arm_sync_mpstate_to_kvm(cpu);
return ret;
@@ -887,6 +911,42 @@ int kvm_arch_get_registers(CPUState *cs)
return ret;
}
+void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
+{
+ ARMCPU *cpu = ARM_CPU(c);
+ CPUARMState *env = &cpu->env;
+
+ ram_addr_t ram_addr;
+ hwaddr paddr;
+
+ if (arm_feature(env, ARM_FEATURE_RAS_EXTENSION) && addr) {
+ ram_addr = qemu_ram_addr_from_host(addr);
+ if (ram_addr != RAM_ADDR_INVALID &&
+ kvm_physical_memory_addr_from_host(c->kvm_state, addr, &paddr)) {
+ /* For the hardware error, some are hwpoison errors, some
+ * are other errors, such as translation table walk or hardware
+ * update of translation table. For the hwpoison, the si_code
+ * is BUS_MCEERR_AR or BUS_MCEERR_AO, other errors are 0
+ */
+ if (code == BUS_MCEERR_AR || code == BUS_MCEERR_AO) {
+ kvm_hwpoison_page_add(ram_addr);
+ }
+ ghes_update_guest(ACPI_HEST_NOTIFY_SEA, paddr);
+ c->exception_index = EXCP_DATA_ABORT;
+ kvm_cpu_synchronize_state(c);
+ return;
+ }
+
+ fprintf(stderr, "Hardware memory error for memory used by "
+ "QEMU itself instead of guest system!\n");
+ }
+
+ if (code == BUS_MCEERR_AR) {
+ fprintf(stderr, "Hardware memory error!\n");
+ exit(1);
+ }
+}
+
/* C6.6.29 BRK instruction */
static const uint32_t brk_insn = 0xd4200000;
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 633d088..7cdde97 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -288,4 +288,5 @@ static inline const char *its_class_name(void)
}
}
+void kvm_hwpoison_page_add(ram_addr_t ram_addr);
#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v3 4/4] target-arm: kvm64: handle SError interrupt for RAS extension
2017-04-30 5:35 [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 2/4] target-arm: kvm64: detect guest RAS EXTENSION feature Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 3/4] target-arm: kvm64: handle SIGBUS signal for synchronous External Abort Dongjiu Geng
@ 2017-04-30 5:35 ` Dongjiu Geng
2017-05-12 21:00 ` [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Laszlo Ersek
2017-05-12 23:59 ` Michael S. Tsirkin
4 siblings, 0 replies; 17+ messages in thread
From: Dongjiu Geng @ 2017-04-30 5:35 UTC (permalink / raw)
To: james.morse, marc.zyngier, christoffer.dall, rkrcmar, linux,
tbaicar, imammedo, zhaoshenglong, peter.maydell, pbonzini,
qemu-devel, qemu-arm, lersek, ard.biesheuvel, mtsirkin, drjones,
ben, kvm, linux-arm-kernel, kvmarm
Cc: xiexiuqi, wangxiongfeng2, songwenjun, wuquanming, huangshaoyu,
gengdongjiu
Record this CPER to GHES, and raise a virtual SError interrupt
with specified syndrome information, the virtual syndrome
is delivered by vsesr_el2. the reason that record the error to HEST
table and inject the SError to guest is due to guest can
handle some errores to avoid directly panic, such as the
application error defered by ESB.
The steps are shown below:
1. syndrome_info and host error VA is delivered by KVM, qemu parse it.
2. translate the host VA to PA and record this error to HEST table.
3. Raise a virtual SError interrupt by set the hcr_el2.vse and pass
the virtual syndrome
Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
| 5 +++++
target/arm/cpu.h | 5 +++++
target/arm/helper.c | 20 ++++++++++++++++++++
target/arm/internals.h | 1 +
target/arm/kvm.c | 3 +++
target/arm/kvm64.c | 31 +++++++++++++++++++++++++++++++
target/arm/kvm_arm.h | 14 ++++++++++++++
7 files changed, 79 insertions(+)
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index c985e10..7951b91 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -360,6 +360,11 @@ struct kvm_run {
struct {
__u8 vector;
} eoi;
+ /* KVM_EXIT_INTR */
+ struct {
+ __u32 syndrome_info;
+ __u64 address;
+ } intr;
/* KVM_EXIT_HYPERV */
struct kvm_hyperv_exit hyperv;
/* Fix the size of the union. */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 1cf4a5b..a2e6348 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -58,6 +58,8 @@
#define EXCP_SEMIHOST 16 /* semihosting call */
#define EXCP_NOCP 17 /* v7M NOCP UsageFault */
#define EXCP_INVSTATE 18 /* v7M INVSTATE UsageFault */
+#define EXCP_SERROR 19 /* v8 System Error or Asynchronous Abort */
+
#define ARMV7M_EXCP_RESET 1
#define ARMV7M_EXCP_NMI 2
@@ -273,6 +275,7 @@ typedef struct CPUARMState {
};
uint64_t esr_el[4];
};
+ uint64_t vsesr_el2;
uint32_t c6_region[8]; /* MPU base/size registers. */
union { /* Fault address registers. */
struct {
@@ -922,6 +925,8 @@ void pmccntr_sync(CPUARMState *env);
#define PSTATE_MODE_EL1t 4
#define PSTATE_MODE_EL0t 0
+#define VSESR_ELx_IDS_ISS_MASK ((1UL << 25) - 1)
+
/* Map EL and handler into a PSTATE_MODE. */
static inline unsigned int aarch64_pstate_mode(unsigned int el, bool handler)
{
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8cb7a94..bdfaba2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4852,6 +4852,15 @@ void register_cp_regs_for_features(ARMCPU *cpu)
}
define_arm_cp_regs(cpu, v8_idregs);
define_arm_cp_regs(cpu, v8_cp_reginfo);
+ if (arm_feature(env, ARM_FEATURE_RAS_EXTENSION)) {
+ ARMCPRegInfo ras_cp_reginfo = {
+ .name = "VSESR_EL2", .state = ARM_CP_STATE_AA64,
+ .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 3,
+ .access = PL2_RW,
+ .fieldoffset = offsetof(CPUARMState, cp15.vsesr_el2)
+ };
+ define_one_arm_cp_reg(cpu, &ras_cp_reginfo);
+ }
}
if (arm_feature(env, ARM_FEATURE_EL2)) {
uint64_t vmpidr_def = mpidr_read_val(env);
@@ -6930,6 +6939,17 @@ void arm_cpu_do_interrupt(CPUState *cs)
qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
return;
}
+ if (cs->exception_index == EXCP_SERROR
+ && arm_feature(env, ARM_FEATURE_RAS_EXTENSION)) {
+ /* Raise virtual System Error or Asynchronous Abort and
+ * set virtual syndrome information
+ */
+ env->cp15.esr_el[2] = env->exception.syndrome;
+ env->cp15.vsesr_el2 = env->exception.syndrome & VSESR_ELx_IDS_ISS_MASK;
+ env->cp15.hcr_el2 = env->cp15.hcr_el2 | HCR_VSE;
+ qemu_log_mask(CPU_LOG_INT, "...handled as async SError\n");
+ return;
+ }
/* Semihosting semantics depend on the register width of the
* code that caused the exception, not the target exception level,
diff --git a/target/arm/internals.h b/target/arm/internals.h
index f742a41..29aabad 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -70,6 +70,7 @@ static const char * const excnames[] = {
[EXCP_VIRQ] = "Virtual IRQ",
[EXCP_VFIQ] = "Virtual FIQ",
[EXCP_SEMIHOST] = "Semihosting call",
+ [EXCP_SERROR] = "async SError"
};
/* Scale factor for generic timers, ie number of ns per tick.
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index bf6ee4c..000e7c3 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -570,12 +570,15 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
{
int ret = 0;
+ struct kvm_async_abort *async_abort = (struct kvm_async_abort *)&run->intr;
switch (run->exit_reason) {
case KVM_EXIT_DEBUG:
if (kvm_arm_handle_debug(cs, &run->debug.arch)) {
ret = EXCP_DEBUG;
} /* otherwise return to guest */
break;
+ case KVM_EXIT_INTR:
+ kvm_arm_inject_serror(cs, async_abort);
default:
qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
__func__, run->exit_reason);
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 5eee6f4..7d4c32f 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1048,3 +1048,34 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
return false;
}
+
+bool kvm_arm_inject_serror(CPUState *cs,
+ struct kvm_async_abort *async_abort_exit)
+{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUClass *cc = CPU_GET_CLASS(cs);
+ CPUARMState *env = &cpu->env;
+ hwaddr paddr;
+ ram_addr_t ram_addr;
+
+ /* Ensure PC is synchronised */
+ kvm_cpu_synchronize_state(cs);
+
+ /* record the CPER to GHES */
+ if (arm_feature(env, ARM_FEATURE_RAS_EXTENSION)
+ && async_abort_exit->address) {
+ ram_addr = qemu_ram_addr_from_host((void *)async_abort_exit->address);
+ if (ram_addr != RAM_ADDR_INVALID &&
+ kvm_physical_memory_addr_from_host(cs->kvm_state,
+ (void *)async_abort_exit->address, &paddr)) {
+ ghes_update_guest(ACPI_HEST_NOTIFY_SEI, paddr);
+ }
+ }
+ cs->exception_index = EXCP_SERROR;
+ env->exception.syndrome = async_abort_exit->hsr;
+
+ /* inject SError to guest */
+ cc->do_interrupt(cs);
+
+ return true;
+}
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 7cdde97..ed73e2b 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -120,6 +120,11 @@ bool write_kvmstate_to_list(ARMCPU *cpu);
*/
void kvm_arm_reset_vcpu(ARMCPU *cpu);
+struct kvm_async_abort {
+ unsigned int hsr;
+ unsigned long address; /* used for async SError */
+};
+
#ifdef CONFIG_KVM
/**
* kvm_arm_create_scratch_host_vcpu:
@@ -249,6 +254,15 @@ static inline const char *gicv3_class_name(void)
bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit);
/**
+ * kvm_arm_inject_serror:
+ * @cs: CPUState
+ * @async_abort_exit: the async abort exception infomation
+ *
+ * Returns: TRUE if the async SError was injected.
+ */
+bool kvm_arm_inject_serror(CPUState *cs,
+ struct kvm_async_abort *async_abort_exit);
+/**
* kvm_arm_hw_debug_active:
* @cs: CPU State
*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-04-30 5:35 [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Dongjiu Geng
` (2 preceding siblings ...)
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 4/4] target-arm: kvm64: handle SError interrupt for RAS extension Dongjiu Geng
@ 2017-05-12 21:00 ` Laszlo Ersek
2017-05-20 5:35 ` gengdongjiu
2017-05-22 14:23 ` Laszlo Ersek
2017-05-12 23:59 ` Michael S. Tsirkin
4 siblings, 2 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-12 21:00 UTC (permalink / raw)
To: Dongjiu Geng, james.morse, marc.zyngier, christoffer.dall,
rkrcmar, linux, tbaicar, imammedo, zhaoshenglong, peter.maydell,
pbonzini, qemu-devel, qemu-arm, ard.biesheuvel, mtsirkin, drjones,
ben, kvm, linux-arm-kernel, kvmarm
Cc: songwenjun, wuquanming, xiexiuqi, huangshaoyu, wangxiongfeng2
On 04/30/17 07:35, Dongjiu Geng wrote:
> This implements APEI GHES Table by passing the error cper info
> to the guest via a fw_cfg_blob. After a CPER info is added, an
> SEA/SEI exception will be injected into the guest OS.
>
> Below is the table layout, the max number of error soure is 11,
> which is classified by notification type.
>
> etc/acpi/tables etc/hardware_errors
> ================ ==========================================
> +-----------+
> +--------------+ | address | +-> +--------------+
> | HEST + | registers | | | Error Status |
> + +------------+ | +---------+ | | Data Block 1 |
> | | GHES1 | --> | |address1 | --------+ | +------------+
> | | GHES2 | --> | |address2 | ------+ | | CPER |
> | | GHES3 | --> | |address3 | ----+ | | | CPER |
> | | .... | --> | | ....... | | | | | CPER |
> | | GHES10 | --> | |address10| -+ | | | | CPER |
> +-+------------+ +-+---------+ | | | +-+------------+
> | | |
> | | +---> +--------------+
> | | | Error Status |
> | | | Data Block 2 |
> | | | +------------+
> | | | | CPER |
> | | | | CPER |
> | | +-+------------+
> | |
> | +-----> +--------------+
> | | Error Status |
> | | Data Block 3 |
> | | +------------+
> | | | CPER |
> | +-+------------+
> | ...........
> +--------> +--------------+
> | Error Status |
> | Data Block 10|
> | +------------+
> | | CPER |
> | | CPER |
> | | CPER |
> +-+------------+
>
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> ---
> default-configs/arm-softmmu.mak | 1 +
> hw/acpi/Makefile.objs | 1 +
> hw/acpi/aml-build.c | 2 +
> hw/acpi/hest_ghes.c | 203 +++++++++++++++++++++++++++++++++++
> hw/arm/virt-acpi-build.c | 6 ++
> include/hw/acpi/acpi-defs.h | 227 ++++++++++++++++++++++++++++++++++++++++
> include/hw/acpi/aml-build.h | 1 +
> include/hw/acpi/hest_ghes.h | 43 ++++++++
> 8 files changed, 484 insertions(+)
> create mode 100644 hw/acpi/hest_ghes.c
> create mode 100644 include/hw/acpi/hest_ghes.h
Disclaimer: I'm not an ACPI (or any kind of) QEMU maintainer, so I can
only share my personal opinion.
(1) This patch is too big. It should be split in two parts at least.
The first patch should contain the new ACPI structures and macros. The
second patch should contain the generation feature.
I'll reorder the diff in my response.
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 4cc3630..27adede 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -295,6 +295,58 @@ typedef struct AcpiMultipleApicTable AcpiMultipleApicTable;
> #define ACPI_APIC_GENERIC_TRANSLATOR 15
> #define ACPI_APIC_RESERVED 16 /* 16 and greater are reserved */
>
(2) Please add a comment above the following macros: they come from the
UEFI Spec 2.6, "N.2.5 Memory Error Section".
> +#define CPER_MEM_VALID_ERROR_STATUS 0x0001
> +#define CPER_MEM_VALID_PA 0x0002
> +#define CPER_MEM_VALID_PA_MASK 0x0004
> +#define CPER_MEM_VALID_NODE 0x0008
> +#define CPER_MEM_VALID_CARD 0x0010
> +#define CPER_MEM_VALID_MODULE 0x0020
> +#define CPER_MEM_VALID_BANK 0x0040
> +#define CPER_MEM_VALID_DEVICE 0x0080
> +#define CPER_MEM_VALID_ROW 0x0100
> +#define CPER_MEM_VALID_COLUMN 0x0200
> +#define CPER_MEM_VALID_BIT_POSITION 0x0400
> +#define CPER_MEM_VALID_REQUESTOR_ID 0x0800
> +#define CPER_MEM_VALID_RESPONDER_ID 0x1000
> +#define CPER_MEM_VALID_TARGET_ID 0x2000
(3) _ID should be dropped.
> +#define CPER_MEM_VALID_ERROR_TYPE 0x4000
> +#define CPER_MEM_VALID_RANK_NUMBER 0x8000
> +#define CPER_MEM_VALID_CARD_HANDLE 0x10000
> +#define CPER_MEM_VALID_MODULE_HANDLE 0x20000
(4) I think if you are padding the first 16 macros with zeroes on the
left, then they should be padded to five nibbles, given that you have 18
macros.
(5) Please prefix all of the macro names with "UEFI_".
> +
> +typedef struct {
> + uint8_t b[16];
> +} uuid_le;
> +
> +#define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
> +((uuid_le) \
> +{{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
> + (b) & 0xff, ((b) >> 8) & 0xff, \
> + (c) & 0xff, ((c) >> 8) & 0xff, \
> + (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) } })
(6) This shouldn't be necessary -- or at least not here. We already have
"include/qemu/uuid.h".
If you need this macro, then in my opinion it should be moved to
"include/qemu/uuid.h" (in a separate patch), and the macro should
produce a compound literal of the QemuUUID structure type.
And, as documented for QemuUUID, it should be in big endian byte order.
For little-endian use, it should be byte-swapped with qemu_uuid_bswap().
> +
> +/* Platform Memory */
> +#define CPER_SEC_PLATFORM_MEM \
> + UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \
> + 0xED, 0x7C, 0x83, 0xB1)
(7) Please add a comment this is from UEFI 2.6 "N.2.2 Section
Descriptor".
(8) Please prefix the macro with UEFI_.
> +
> +/* Values for Notify Type field above */
> +
> +enum acpi_hest_notify_types {
> + ACPI_HEST_NOTIFY_POLLED = 0,
> + ACPI_HEST_NOTIFY_EXTERNAL = 1,
> + ACPI_HEST_NOTIFY_LOCAL = 2,
> + ACPI_HEST_NOTIFY_SCI = 3,
> + ACPI_HEST_NOTIFY_NMI = 4,
> + ACPI_HEST_NOTIFY_CMCI = 5, /* ACPI 5.0 */
> + ACPI_HEST_NOTIFY_MCE = 6, /* ACPI 5.0 */
> + ACPI_HEST_NOTIFY_GPIO = 7, /* ACPI 6.0 */
> + ACPI_HEST_NOTIFY_SEA = 8, /* ACPI 6.1 */
> + ACPI_HEST_NOTIFY_SEI = 9, /* ACPI 6.1 */
> + ACPI_HEST_NOTIFY_GSIV = 10, /* ACPI 6.1 */
> + ACPI_HEST_NOTIFY_RESERVED = 11 /* 11 and greater are reserved */
> +};
> +
(9) Please add a comment that this is from the ACPI 6.1 spec, "18.3.2.9
Hardware Error Notification".
(10) For better or worse, type names and struct tags in this header file
use CamelCase, and generally start with the prefix Acpi. So I think the
above should be called "AcpiHestNotifyType" (singular).
The enum constants look good.
> /*
> * MADT sub-structures (Follow MULTIPLE_APIC_DESCRIPTION_TABLE)
> */
> @@ -475,6 +527,181 @@ struct AcpiSystemResourceAffinityTable
> } QEMU_PACKED;
> typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
>
> +#define ACPI_ADR_SPACE_SYSTEM_MEMORY (uint8_t) 0
> +#define ACPI_ADR_SPACE_SYSTEM_IO (uint8_t) 1
> +#define ACPI_ADR_SPACE_PCI_CONFIG (uint8_t) 2
> +#define ACPI_ADR_SPACE_EC (uint8_t) 3
> +#define ACPI_ADR_SPACE_SMBUS (uint8_t) 4
> +#define ACPI_ADR_SPACE_CMOS (uint8_t) 5
> +#define ACPI_ADR_SPACE_PCI_BAR_TARGET (uint8_t) 6
> +#define ACPI_ADR_SPACE_IPMI (uint8_t) 7
> +#define ACPI_ADR_SPACE_GPIO (uint8_t) 8
> +#define ACPI_ADR_SPACE_GSBUS (uint8_t) 9
> +#define ACPI_ADR_SPACE_PLATFORM_COMM (uint8_t) 10
(11) These macros are not necessary. Instead, please extend the
AmlRegionSpace enum type in "include/hw/acpi/aml-build.h".
(12) Additionally, where do the values 5 through 9 come from? ACPI 6.1
"5.2.3.2 Generic Address Structure" leaves them reserved.
> +
> +/* GAS - Generic Address Structure */
> +struct acpi_generic_address {
> + uint8_t space_id; /* Address space where
> + *struct or register exists
> + */
> + uint8_t bit_width; /* Size in bits of given register */
> + uint8_t bit_offset; /* Bit offset within the register */
> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */
> + uint64_t address; /* 64-bit address of struct or register */
> +} __attribute__ ((packed));
> +
(13) This structure is already defined, see AcpiGenericAddress.
> +/* Hardware Error Notification */
> +struct acpi_hest_notify {
> + uint8_t type;
> + uint8_t length;
> + uint16_t config_write_enable;
> + uint32_t poll_interval;
> + uint32_t vector;
> + uint32_t polling_threshold_value;
> + uint32_t polling_threshold_window;
> + uint32_t error_threshold_value;
> + uint32_t error_threshold_window;
> +};
(14) Please add a comment that this is from the ACPI 6.1 spec, "18.3.2.9
Hardware Error Notification".
(15) The structure should be called AcpiHestNotify. Please also add a
direct typedef for it, similarly to the other struct AcpiXxxx types seen
in this header.
(16) To the "type" field, please append a comment that the values come
from AcpiHestNotifyType.
(17) This structure should be packed. Please add QEMU_PACKED between the
closing brace and the semicolon.
> +
> +enum acpi_hest_types {
> + ACPI_HEST_TYPE_IA32_CHECK = 0,
> + ACPI_HEST_TYPE_IA32_CORRECTED_CHECK = 1,
> + ACPI_HEST_TYPE_IA32_NMI = 2,
> + ACPI_HEST_TYPE_NOT_USED3 = 3,
> + ACPI_HEST_TYPE_NOT_USED4 = 4,
> + ACPI_HEST_TYPE_NOT_USED5 = 5,
> + ACPI_HEST_TYPE_AER_ROOT_PORT = 6,
> + ACPI_HEST_TYPE_AER_ENDPOINT = 7,
> + ACPI_HEST_TYPE_AER_BRIDGE = 8,
> + ACPI_HEST_TYPE_GENERIC_ERROR = 9,
> + ACPI_HEST_TYPE_GENERIC_ERROR_V2 = 10,
> + ACPI_HEST_TYPE_RESERVED = 11 /* 11 and greater are reserved */
> +};
(18) Please add a comment that these are from ACPI 6.1, sections
"18.3.2.1 IA-32 Architecture Machine Check Exception" through "18.3.2.8
Generic Hardware Error Source version 2".
(19) The type name should be "AcpiHestSourceType" (singular).
(20) I think the enum constants should be renamed to
ACPI_HEST_SOURCE_xxx, from ACPI_HEST_TYPE_xxx.
(21) I think the NOT_USED{3,4,5} enum constants should be removed.
> +
> +/* Values for block_status flags above */
(22) Here I think we should only say, 'Block Status bitmasks from ACPI
6.1, "18.3.2.7.1 Generic Error Data"'. The block_status field that you
refer to is not above, it comes later.
> +#define ACPI_BERT_UNCORRECTABLE (1)
> +#define ACPI_BERT_CORRECTABLE (1 << 1)
> +#define ACPI_BERT_MULTIPLE_UNCORRECTABLE (1 << 2)
> +#define ACPI_BERT_MULTIPLE_CORRECTABLE (1 << 3)
> +/* 8 bits, error count */
> +#define ACPI_BERT_ERROR_ENTRY_COUNT (0xFF << 4)
>
(23) Any particular reason to call these BERT? The "Boot Error Record
Table" is specified in "18.3.1 Boot Error Source", but the block status
bitmasks don't look related.
To me ACPI_GEBS_xxx ("generic error block status") seems like a more
fitting prefix.
+
> +/* Generic Hardware Error Source Structure */
> +struct AcpiGenericHardwareErrorSource {
> + uint16_t type;
> + uint16_t source_id;
> + uint16_t related_source_id;
> + uint8_t flags;
> + uint8_t enabled;
> + uint32_t number_of_records;
> + uint32_t max_sections_per_record;
> + uint32_t max_raw_data_length;
> + struct acpi_generic_address error_status_address;
> + struct acpi_hest_notify notify;
> + uint32_t error_status_block_length;
> +} QEMU_PACKED;
> +typedef struct AcpiGenericHardwareErrorSource AcpiGenericHardwareErrorSource;
(24) This looks good to me in general.
I suggest adding a reference to ACPI 6.1 "18.3.2.7 Generic Hardware
Error Source". Also, I think we should mention that "type" has to be
ACPI_HEST_SOURCE_GENERIC_ERROR.
> +
> +/* Generic Hardware Error Source , version 2 */
> +struct AcpiGenericHardwareErrorSourceV2 {
> + uint16_t type;
> + uint16_t source_id;
> + uint16_t related_source_id;
> + uint8_t flags;
> + uint8_t enabled;
> + uint32_t number_of_records;
> + uint32_t max_sections_per_record;
> + uint32_t max_raw_data_length;
> + struct acpi_generic_address error_status_address;
> + struct acpi_hest_notify notify;
> + uint32_t error_status_block_length;
> + struct acpi_generic_address read_ack_register;
> + uint64_t read_ack_preserve;
> + uint64_t read_ack_write;
> +} QEMU_PACKED;
> +typedef struct AcpiGenericHardwareErrorSourceV2
> + AcpiGenericHardwareErrorSourceV2;
(25) Same comments; I suggest adding a reference to "18.3.2.8 Generic
Hardware Error Source version 2", and mentioning
ACPI_HEST_SOURCE_GENERIC_ERROR_V2 for the "type" field.
> +
> +/* Generic Error Status block */
> +
> +struct AcpiGenericErrorStatus {
> + uint32_t block_status;
> + uint32_t raw_data_offset;
> + uint32_t raw_data_length;
> + uint32_t data_length;
> + uint32_t error_severity;
> +};
> +typedef struct AcpiGenericErrorStatus AcpiGenericErrorStatus;
> +
(26) Please mention that this is from ACPI 6.1, "18.3.2.7.1 Generic
Error Data".
(27) Near the block_status field, we should point out that it is a
bitmask composed of ACPI_GEBS_xxx macros.
(28) QEMU_PACKED is missing. (It will make no difference in practice,
but I recommend it for consistency and documentation purposes.)
> +/* Generic Error Data entry */
> +
> +struct AcpiGenericErrorData {
> + uint8_t section_type[16];
> + uint32_t error_severity;
> + uint16_t revision;
> + uint8_t validation_bits;
> + uint8_t flags;
> + uint32_t error_data_length;
> + uint8_t fru_id[16];
> + uint8_t fru_text[20];
> +};
> +typedef struct AcpiGenericErrorData AcpiGenericErrorData;
(29) Please point to ACPI 6.1, "18.3.2.7.1 Generic Error Data" again, in
the leading comment.
(30) QEMU_PACKED is missing.
(31) I think we should use the QemuUUID type for the "section_type"
field. And, in order to make it clear that it has little endian
encoding, let's call it "section_type_le".
An added benefit is that in the code, the field can be set with a simple
structure assignment from UEFI_CPER_SEC_PLATFORM_MEM (and then can be
byte-swapped in place, for little endiannes, with qemu_uuid_bswap()).
> +
> +/* Extension for revision 0x0300 */
> +struct AcpiGenericErrorDataV300 {
> + uint8_t section_type[16];
> + uint32_t error_severity;
> + uint16_t revision;
> + uint8_t validation_bits;
> + uint8_t flags;
> + uint32_t error_data_length;
> + uint8_t fru_id[16];
> + uint8_t fru_text[20];
> + uint64_t time_stamp;
> +};
> +typedef struct AcpiGenericErrorDataV300 AcpiGenericErrorDataV300;
> +
(32) Same comments as (29), (30), (31) above.
(33) Actually, do we need both AcpiGenericErrorData and
AcpiGenericErrorDataV300? In the code we seem to be using only the
former. On the other hand, in the spec I can see only the latter. Where
does AcpiGenericErrorData come from?
> +enum {
> + CPER_SEV_RECOVERABLE,
> + CPER_SEV_FATAL,
> + CPER_SEV_CORRECTED,
> + CPER_SEV_INFORMATIONAL,
> +};
(34) I suggest giving a name to this type, for example
AcpiGenericErrorSeverity.
(35) The enumeration constants should start with ACPI_.
(36) I suggest moving this type above AcpiGenericErrorData and
AcpiGenericErrorDataV300, and remarking on the "error_severity" fields
that they take their values from AcpiGenericErrorSeverity.
(37) Where does "INFORMATIONAL" come from? In ACPI 6.1, "18.3.2.7.1
Generic Error Data", I see "None" for value 3.
> +
> +/* Memory Error Section */
> +struct cper_sec_mem_err {
> + uint64_t validation_bits;
> + uint64_t error_status;
> + uint64_t physical_addr;
> + uint64_t physical_addr_mask;
> + uint16_t node;
> + uint16_t card;
> + uint16_t module;
> + uint16_t bank;
> + uint16_t device;
> + uint16_t row;
> + uint16_t column;
> + uint16_t bit_pos;
> + uint64_t requestor_id;
> + uint64_t responder_id;
> + uint64_t target_id;
> + uint8_t error_type;
> + uint8_t reserved;
> + uint16_t rank;
> + uint16_t mem_array_handle; /* card handle in UEFI 2.4 */
> + uint16_t mem_dev_handle; /* module handle in UEFI 2.4 */
> +};
> + typedef struct cper_sec_mem_err cper_sec_mem_err;
(38) Please add a comment that is is from UEFI 2.6, "N.2.5 Memory Error
Section".
(39) The structure and the typedef should be called "UefiCperSecMemErr".
(40) I suggest adding a comment to "validation_bits" that it is a
bitmask composed of CPER_MEM_VALID_xxx macros.
(41) QEMU_PACKED is missing.
> +
> +/*
> + * HEST Description Table
> + */
> +struct AcpiHardwareErrorSourceTable {
> + ACPI_TABLE_HEADER_DEF /* ACPI common table header */
> + uint32_t error_source_count;
> +} QEMU_PACKED;
> +typedef struct AcpiHardwareErrorSourceTable AcpiHardwareErrorSourceTable;
> +
> #define ACPI_SRAT_PROCESSOR_APIC 0
> #define ACPI_SRAT_MEMORY 1
> #define ACPI_SRAT_PROCESSOR_x2APIC 2
Next file:
> diff --git a/include/hw/acpi/hest_ghes.h b/include/hw/acpi/hest_ghes.h
> new file mode 100644
> index 0000000..0cadc2b
> --- /dev/null
> +++ b/include/hw/acpi/hest_ghes.h
> @@ -0,0 +1,43 @@
> +#ifndef ACPI_GHES_H
> +#define ACPI_GHES_H
> +
> +#include "hw/acpi/bios-linker-loader.h"
> +
> +#define GHES_ERRORS_FW_CFG_FILE "etc/hardware_errors"
> +#define GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
> +
> +#define GAS_ADDRESS_OFFSET 4
> +#define ERROR_STATUS_ADDRESS_OFFSET 20
> +#define NOTIFICATION_STRUCTURE 32
> +
> +#define BFAPEI_OK 0
> +#define BFAPEI_FAIL 1
> +
> +/* The max number of error source, the error sources
> + * are classified by notification type, below is the definition
> + * 0 - Polled
> + * 1 - External Interrupt
> + * 2 - Local Interrupt
> + * 3 - SCI
> + * 4 - NMI
> + * 5 - CMCI
> + * 6 - MCE
> + * 7 - GPIO-Signal
> + * 8 - ARMv8 SEA
> + * 9 - ARMv8 SEI
> + * 10 - External Interrupt - GSIV
> + */
> +#define MAX_ERROR_SOURCE_COUNT_V6 11
I'll have to review this header file more thoroughly, once I see the
code that references these macros. For now, I have one comment:
(42) I think the notification type list should be removed from this
location. Also, the open-coded value 11 should be replaced with
the ACPI_HEST_NOTIFY_RESERVED enumeration constant.
I will try to continue reviewing this patch sometime next week (second
half of the week at the earliest, I think).
Please feel free to disagree with my comments; I prefer to write down
everything that crosses my mind. It's encouraged to raise
counter-arguments.
Thanks
Laszlo
> +/* The max size in Bytes for one error block */
> +#define MAX_RAW_DATA_LENGTH 0x1000
> +
> +typedef struct GhesErrorState {
> + uint64_t physical_addr;
> + uint64_t ghes_addr_le[8];
> +} GhesErrorState;
> +
> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
> + BIOSLinker *linker);
> +void ghes_add_fw_cfg(FWCfgState *s, GArray *guid);
> +void ghes_update_guest(uint32_t notify, uint64_t physical_address);
> +#endif
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0835e59..e7ab5dc 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -45,6 +45,8 @@
> #include "hw/arm/virt.h"
> #include "sysemu/numa.h"
> #include "kvm_arm.h"
> +#include "hw/acpi/vmgenid.h"
> +#include "hw/acpi/hest_ghes.h"
>
> #define ARM_SPI_BASE 32
> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> @@ -778,6 +780,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> acpi_add_table(table_offsets, tables_blob);
> build_spcr(tables_blob, tables->linker, vms);
>
> + acpi_add_table(table_offsets, tables_blob);
> + ghes_build_acpi(tables_blob, tables->hardware_errors, tables->linker);
> +
> if (nb_numa_nodes > 0) {
> acpi_add_table(table_offsets, tables_blob);
> build_srat(tables_blob, tables->linker, vms);
> @@ -892,6 +897,7 @@ void virt_acpi_setup(VirtMachineState *vms)
>
> build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
> ACPI_BUILD_RSDP_FILE, 0);
> + ghes_add_fw_cfg(vms->fw_cfg, tables.hardware_errors);
>
> qemu_register_reset(virt_acpi_build_reset, build_state);
> virt_acpi_build_reset(build_state);
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 1e3bd2b..d5f1552 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -121,3 +121,4 @@ CONFIG_ACPI=y
> CONFIG_SMBIOS=y
> CONFIG_ASPEED_SOC=y
> CONFIG_GPIO_KEY=y
> +CONFIG_ACPI_APEI_GENERATION=y
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 11c35bc..776b46e 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
> common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
> +common-obj-$(CONFIG_ACPI_APEI_GENERATION) += hest_ghes.o
> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>
> common-obj-y += acpi_interface.o
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index c6f2032..802b98d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1560,6 +1560,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();
> }
>
> @@ -1570,6 +1571,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..91d382e
> --- /dev/null
> +++ b/hw/acpi/hest_ghes.c
> @@ -0,0 +1,203 @@
> +/*
> + * APEI GHES table Generation
> + *
> + * Copyright (C) 2017 huawei.
> + *
> + * Author: Dongjiu Geng <gengdongjiu@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qmp-commands.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"
> +
> +static int ghes_generate_cper_record(uint64_t block_error_address,
> + uint64_t error_physical_addr)
> +{
> + AcpiGenericErrorStatus block;
> + AcpiGenericErrorData *gdata;
> + struct cper_sec_mem_err *mem_err;
> + uint64_t block_data_length;
> + unsigned char *buffer;
> +
> + cpu_physical_memory_read(block_error_address, &block,
> + sizeof(AcpiGenericErrorStatus));
> +
> + block_data_length = sizeof(AcpiGenericErrorStatus) + block.data_length;
> +
> + /* If the Generic Error Status Block is NULL, update
> + * the block header
> + */
> + if (!block.block_status) {
> + block.block_status = ACPI_BERT_UNCORRECTABLE;
> + block.error_severity = CPER_SEV_FATAL;
> + }
> +
> + block.data_length += sizeof(AcpiGenericErrorData);
> + block.data_length += sizeof(struct cper_sec_mem_err);
> +
> + /* Write back the Generic Error Status Block to guest memory */
> + cpu_physical_memory_write(block_error_address, &block,
> + sizeof(AcpiGenericErrorStatus));
> +
> + /* Fill in Generic Error Data Entry */
> + buffer = g_malloc(sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
> + memset(buffer, 0, sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
> + gdata = (AcpiGenericErrorData *)buffer;
> +
> + memcpy(gdata->section_type, (void *) &CPER_SEC_PLATFORM_MEM,
> + sizeof(uuid_le));
> + gdata->error_data_length = sizeof(struct cper_sec_mem_err);
> +
> + mem_err = (struct cper_sec_mem_err *) (gdata + 1);
> +
> + /* In order to simplify simulation, hardcode the CPER section to memory
> + * section.
> + */
> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
> + mem_err->error_type = 3;
> +
> + mem_err->validation_bits |= CPER_MEM_VALID_PA;
> + mem_err->physical_addr = error_physical_addr;
> +
> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
> + mem_err->card = 1;
> + mem_err->module = 2;
> + mem_err->bank = 3;
> + mem_err->row = 1;
> + mem_err->column = 2;
> + mem_err->bit_pos = 5;
> +
> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_STATUS;
> + mem_err->error_status = 4 << 8;
> +
> + /* Write back the Generic Error Data Entry to guest memory */
> + cpu_physical_memory_write(block_error_address + block_data_length, buffer,
> + sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
> +
> + g_free(buffer);
> + return BFAPEI_OK;
> +}
> +
> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
> + BIOSLinker *linker)
> +{
> + Aml *hest;
> + uint32_t address_registers_offset;
> + AcpiTableHeader *header;
> + AcpiGenericHardwareErrorSource *error_source;
> + int i;
> +
> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
> +
> + /* New address register and Error status block table size*/
> + g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6
> + * block_reqr_size);
> +
> + /* Put this in a HEST table */
> + hest = init_aml_allocator();
> + address_registers_offset = table_data->len
> + + sizeof(AcpiHardwareErrorSourceTable)
> + + ERROR_STATUS_ADDRESS_OFFSET
> + + GAS_ADDRESS_OFFSET;
> + /* Reserve space for HEST table size*/
> + acpi_data_push(hest->buf, sizeof(AcpiHardwareErrorSourceTable)
> + + MAX_ERROR_SOURCE_COUNT_V6
> + * sizeof(AcpiGenericHardwareErrorSource));
> +
> + g_array_append_vals(table_data, hest->buf->data, hest->buf->len);
> + /* Allocate guest memory for the Data fw_cfg blob */
> + bios_linker_loader_alloc(linker, GHES_ERRORS_FW_CFG_FILE,
> + hardware_error, 4096,
> + false /* page boundary, high memory */);
> + header = (AcpiTableHeader *)(table_data->data
> + + table_data->len - hest->buf->len);
> + *(uint32_t *)(header + 1) = MAX_ERROR_SOURCE_COUNT_V6;
> + error_source = (AcpiGenericHardwareErrorSource *)((char *)header
> + + sizeof(AcpiHardwareErrorSourceTable));
> +
> + for (i = 0; i < MAX_ERROR_SOURCE_COUNT_V6; i++) {
> + error_source->type = ACPI_HEST_TYPE_GENERIC_ERROR;
> + error_source->source_id = 0;
> + error_source->related_source_id = 0xffff;
> + error_source->flags = 0;
> + error_source->enabled = 1;
> + error_source->number_of_records = 1;
> + error_source->max_sections_per_record = 1;
> + error_source->max_raw_data_length = MAX_RAW_DATA_LENGTH;
> + error_source->error_status_address.space_id =
> + ACPI_ADR_SPACE_SYSTEM_MEMORY;
> + error_source->error_status_address.bit_width = 64;
> + error_source->error_status_address.bit_offset = 0;
> + error_source->error_status_address.access_width = 4;
> + error_source->notify.type = i;
> + error_source->notify.length = sizeof(AcpiGenericHardwareErrorSource);
> +
> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
> + sizeof(uint64_t) * i, sizeof(uint64_t),
> + GHES_ERRORS_FW_CFG_FILE,
> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
> + i * MAX_RAW_DATA_LENGTH);
> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> + address_registers_offset
> + + i * sizeof(AcpiGenericHardwareErrorSource),
> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
> + i * sizeof(uint64_t));
> +
> + bios_linker_loader_write_pointer(linker, GHES_DATA_ADDR_FW_CFG_FILE,
> + i * sizeof(uint64_t), sizeof(uint64_t),
> + GHES_ERRORS_FW_CFG_FILE,
> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
> + i * MAX_RAW_DATA_LENGTH);
> + error_source++;
> + }
> +
> + build_header(linker, table_data,
> + (void *)header, "HEST", hest->buf->len, 1, NULL, "GHES");
> +
> + free_aml_allocator();
> +}
> +
> +static GhesErrorState ges;
> +void ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_error)
> +{
> +
> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
> + int size = MAX_ERROR_SOURCE_COUNT_V6 * block_reqr_size;
> +
> + /* Create a read-only fw_cfg file for GHES */
> + fw_cfg_add_file(s, GHES_ERRORS_FW_CFG_FILE, hardware_error->data,
> + size);
> + /* Create a read-write fw_cfg file for Address */
> + fw_cfg_add_file_callback(s, GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL,
> + &(ges.ghes_addr_le[0]),
> + sizeof(uint64_t) * MAX_ERROR_SOURCE_COUNT_V6,
> + false);
> +}
> +
> +void ghes_update_guest(uint32_t notify, uint64_t physical_address)
> +{
> + uint64_t block_error_addr;
> +
> + if (physical_address) {
> + ges.physical_addr = physical_address;
> + block_error_addr = ges.ghes_addr_le[notify];
> + block_error_addr = le32_to_cpu(block_error_addr);
> +
> + /* A zero value in ghes_addr means that BIOS has not yet written
> + * the address
> + */
> + if (block_error_addr) {
> + ghes_generate_cper_record(block_error_addr, physical_address);
> + }
> + }
> +}
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 00c21f1..c1d15b3 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
> GArray *rsdp;
> GArray *tcpalog;
> GArray *vmgenid;
> + GArray *hardware_errors;
> BIOSLinker *linker;
> } AcpiBuildTables;
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-05-12 21:00 ` [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Laszlo Ersek
@ 2017-05-20 5:35 ` gengdongjiu
2017-05-22 14:23 ` Laszlo Ersek
1 sibling, 0 replies; 17+ messages in thread
From: gengdongjiu @ 2017-05-20 5:35 UTC (permalink / raw)
To: Laszlo Ersek, james.morse, marc.zyngier, christoffer.dall,
rkrcmar, linux, tbaicar, imammedo, zhaoshenglong, peter.maydell,
pbonzini, qemu-devel, qemu-arm, ard.biesheuvel, mtsirkin, drjones,
ben, kvm, linux-arm-kernel, kvmarm
Cc: songwenjun, wuquanming, xiexiuqi, huangshaoyu, wangxiongfeng2
Laszlo,
sorry for the late response.
On 2017/5/13 5:00, Laszlo Ersek wrote:
> On 04/30/17 07:35, Dongjiu Geng wrote:
>> This implements APEI GHES Table by passing the error cper info
>> to the guest via a fw_cfg_blob. After a CPER info is added, an
>> SEA/SEI exception will be injected into the guest OS.
>>
>> Below is the table layout, the max number of error soure is 11,
>> which is classified by notification type.
>>
>> etc/acpi/tables etc/hardware_errors
>> ================ ==========================================
>> +-----------+
>> +--------------+ | address | +-> +--------------+
>> | HEST + | registers | | | Error Status |
>> + +------------+ | +---------+ | | Data Block 1 |
>> | | GHES1 | --> | |address1 | --------+ | +------------+
>> | | GHES2 | --> | |address2 | ------+ | | CPER |
>> | | GHES3 | --> | |address3 | ----+ | | | CPER |
>> | | .... | --> | | ....... | | | | | CPER |
>> | | GHES10 | --> | |address10| -+ | | | | CPER |
>> +-+------------+ +-+---------+ | | | +-+------------+
>> | | |
>> | | +---> +--------------+
>> | | | Error Status |
>> | | | Data Block 2 |
>> | | | +------------+
>> | | | | CPER |
>> | | | | CPER |
>> | | +-+------------+
>> | |
>> | +-----> +--------------+
>> | | Error Status |
>> | | Data Block 3 |
>> | | +------------+
>> | | | CPER |
>> | +-+------------+
>> | ...........
>> +--------> +--------------+
>> | Error Status |
>> | Data Block 10|
>> | +------------+
>> | | CPER |
>> | | CPER |
>> | | CPER |
>> +-+------------+
>>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>> ---
>> default-configs/arm-softmmu.mak | 1 +
>> hw/acpi/Makefile.objs | 1 +
>> hw/acpi/aml-build.c | 2 +
>> hw/acpi/hest_ghes.c | 203 +++++++++++++++++++++++++++++++++++
>> hw/arm/virt-acpi-build.c | 6 ++
>> include/hw/acpi/acpi-defs.h | 227 ++++++++++++++++++++++++++++++++++++++++
>> include/hw/acpi/aml-build.h | 1 +
>> include/hw/acpi/hest_ghes.h | 43 ++++++++
>> 8 files changed, 484 insertions(+)
>> create mode 100644 hw/acpi/hest_ghes.c
>> create mode 100644 include/hw/acpi/hest_ghes.h
>
> Disclaimer: I'm not an ACPI (or any kind of) QEMU maintainer, so I can
> only share my personal opinion.
I know it, I appreciated that you can find your free time to review it.
>
> (1) This patch is too big. It should be split in two parts at least.
>
> The first patch should contain the new ACPI structures and macros. The
> second patch should contain the generation feature.
OK, have splited it.
>
> I'll reorder the diff in my response.
thanks.
>
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index 4cc3630..27adede 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -295,6 +295,58 @@ typedef struct AcpiMultipleApicTable AcpiMultipleApicTable;
>> #define ACPI_APIC_GENERIC_TRANSLATOR 15
>> #define ACPI_APIC_RESERVED 16 /* 16 and greater are reserved */
>>
>
> (2) Please add a comment above the following macros: they come from the
> UEFI Spec 2.6, "N.2.5 Memory Error Section".
good suggestion.
>
>> +#define CPER_MEM_VALID_ERROR_STATUS 0x0001
>> +#define CPER_MEM_VALID_PA 0x0002
>> +#define CPER_MEM_VALID_PA_MASK 0x0004
>> +#define CPER_MEM_VALID_NODE 0x0008
>> +#define CPER_MEM_VALID_CARD 0x0010
>> +#define CPER_MEM_VALID_MODULE 0x0020
>> +#define CPER_MEM_VALID_BANK 0x0040
>> +#define CPER_MEM_VALID_DEVICE 0x0080
>> +#define CPER_MEM_VALID_ROW 0x0100
>> +#define CPER_MEM_VALID_COLUMN 0x0200
>> +#define CPER_MEM_VALID_BIT_POSITION 0x0400
>> +#define CPER_MEM_VALID_REQUESTOR_ID 0x0800
>> +#define CPER_MEM_VALID_RESPONDER_ID 0x1000
>> +#define CPER_MEM_VALID_TARGET_ID 0x2000
>
> (3) _ID should be dropped.
OK.
I copied these macros from kernel code "include/linux/cper.h"
I have planed to remove the unused macros
>
>> +#define CPER_MEM_VALID_ERROR_TYPE 0x4000
>> +#define CPER_MEM_VALID_RANK_NUMBER 0x8000
>> +#define CPER_MEM_VALID_CARD_HANDLE 0x10000
>> +#define CPER_MEM_VALID_MODULE_HANDLE 0x20000
>
> (4) I think if you are padding the first 16 macros with zeroes on the
> left, then they should be padded to five nibbles, given that you have 18
> macros.
>
> (5) Please prefix all of the macro names with "UEFI_".
>
>> +
>> +typedef struct {
>> + uint8_t b[16];
>> +} uuid_le;
>> +
>> +#define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
>> +((uuid_le) \
>> +{{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
>> + (b) & 0xff, ((b) >> 8) & 0xff, \
>> + (c) & 0xff, ((c) >> 8) & 0xff, \
>> + (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) } })
>
> (6) This shouldn't be necessary -- or at least not here. We already have
> "include/qemu/uuid.h".
>
> If you need this macro, then in my opinion it should be moved to
> "include/qemu/uuid.h" (in a separate patch), and the macro should
> produce a compound literal of the QemuUUID structure type.
>
> And, as documented for QemuUUID, it should be in big endian byte order.
> For little-endian use, it should be byte-swapped with qemu_uuid_bswap().
I checked the struction definition in the "include/qemu/uuid.h", the QemuUUID may different
with this definition，here UUID is for the CPER section UUID. I see the spec all define them
to little-endian.
Platform Memory
•{0xA5BC1114, 0x6F64, 0x4EDE, {0xB8, 0x63, 0x3E, 0x83, 0xED, 0x7C, 0x83, 0xB1}}
PCIe}}
•{0xD995E954, 0xBBC1, 0x430F, {0xAD, 0x91, 0xB4, 0x4D, 0xCB, 0x3C, 0x6F, 0x35}}
Firmware Error Record Reference
•{0x81212A96, 0x09ED, 0x4996, {0x94, 0x71, 0x8D, 0x72, 0x9C, 0x8E, 0x69, 0xED}}
PCI/PCI-X Bus
•{0xC5753963, 0x3B84, 0x4095, {0xBF, 0x78, 0xED, 0xDA, 0xD3, 0xF9, 0xC9, 0xDD}}
PCI Component/Device
•{0xEB5E4685, 0xCA66, 0x4769, {0xB6, 0xA2, 0x26, 0x06, 0x8B, 0x00, 0x13, 0x26}}
DMAr Generic
•{0x5B51FEF7, 0xC79D, 0x4434, {0x8F, 0x1B, 0xAA,
•0x62, 0xDE, 0x3E, 0x2C, 0x64}}
>
>> +
>> +/* Platform Memory */
>> +#define CPER_SEC_PLATFORM_MEM \
>> + UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \
>> + 0xED, 0x7C, 0x83, 0xB1)
>
> (7) Please add a comment this is from UEFI 2.6 "N.2.2 Section
> Descriptor".
>
> (8) Please prefix the macro with UEFI_.
>
>> +
>> +/* Values for Notify Type field above */
>> +
>> +enum acpi_hest_notify_types {
>> + ACPI_HEST_NOTIFY_POLLED = 0,
>> + ACPI_HEST_NOTIFY_EXTERNAL = 1,
>> + ACPI_HEST_NOTIFY_LOCAL = 2,
>> + ACPI_HEST_NOTIFY_SCI = 3,
>> + ACPI_HEST_NOTIFY_NMI = 4,
>> + ACPI_HEST_NOTIFY_CMCI = 5, /* ACPI 5.0 */
>> + ACPI_HEST_NOTIFY_MCE = 6, /* ACPI 5.0 */
>> + ACPI_HEST_NOTIFY_GPIO = 7, /* ACPI 6.0 */
>> + ACPI_HEST_NOTIFY_SEA = 8, /* ACPI 6.1 */
>> + ACPI_HEST_NOTIFY_SEI = 9, /* ACPI 6.1 */
>> + ACPI_HEST_NOTIFY_GSIV = 10, /* ACPI 6.1 */
>> + ACPI_HEST_NOTIFY_RESERVED = 11 /* 11 and greater are reserved */
>> +};
>> +
>
> (9) Please add a comment that this is from the ACPI 6.1 spec, "18.3.2.9
> Hardware Error Notification".
OK.
>
> (10) For better or worse, type names and struct tags in this header file
> use CamelCase, and generally start with the prefix Acpi. So I think the
> above should be called "AcpiHestNotifyType" (singular).
good suggestion.
>
> The enum constants look good.
>
>> /*
>> * MADT sub-structures (Follow MULTIPLE_APIC_DESCRIPTION_TABLE)
>> */
>> @@ -475,6 +527,181 @@ struct AcpiSystemResourceAffinityTable
>> } QEMU_PACKED;
>> typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
>>
>> +#define ACPI_ADR_SPACE_SYSTEM_MEMORY (uint8_t) 0
>> +#define ACPI_ADR_SPACE_SYSTEM_IO (uint8_t) 1
>> +#define ACPI_ADR_SPACE_PCI_CONFIG (uint8_t) 2
>> +#define ACPI_ADR_SPACE_EC (uint8_t) 3
>> +#define ACPI_ADR_SPACE_SMBUS (uint8_t) 4
>> +#define ACPI_ADR_SPACE_CMOS (uint8_t) 5
>> +#define ACPI_ADR_SPACE_PCI_BAR_TARGET (uint8_t) 6
>> +#define ACPI_ADR_SPACE_IPMI (uint8_t) 7
>> +#define ACPI_ADR_SPACE_GPIO (uint8_t) 8
>> +#define ACPI_ADR_SPACE_GSBUS (uint8_t) 9
>> +#define ACPI_ADR_SPACE_PLATFORM_COMM (uint8_t) 10
>
> (11) These macros are not necessary. Instead, please extend the
> AmlRegionSpace enum type in "include/hw/acpi/aml-build.h".
OK.
>
> (12) Additionally, where do the values 5 through 9 come from? ACPI 6.1
> "5.2.3.2 Generic Address Structure" leaves them reserved.
good point. the values 5 through 9 from the kernel code: include/acpi/actypes.h
#define ACPI_ADR_SPACE_SYSTEM_MEMORY (acpi_adr_space_type) 0
#define ACPI_ADR_SPACE_SYSTEM_IO (acpi_adr_space_type) 1
#define ACPI_ADR_SPACE_PCI_CONFIG (acpi_adr_space_type) 2
#define ACPI_ADR_SPACE_EC (acpi_adr_space_type) 3
#define ACPI_ADR_SPACE_SMBUS (acpi_adr_space_type) 4
#define ACPI_ADR_SPACE_CMOS (acpi_adr_space_type) 5
#define ACPI_ADR_SPACE_PCI_BAR_TARGET (acpi_adr_space_type) 6
#define ACPI_ADR_SPACE_IPMI (acpi_adr_space_type) 7
#define ACPI_ADR_SPACE_GPIO (acpi_adr_space_type) 8
#define ACPI_ADR_SPACE_GSBUS (acpi_adr_space_type) 9
#define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10
I planned remove the values 5 through 9.
>
>> +
>> +/* GAS - Generic Address Structure */
>> +struct acpi_generic_address {
>> + uint8_t space_id; /* Address space where
>> + *struct or register exists
>> + */
>> + uint8_t bit_width; /* Size in bits of given register */
>> + uint8_t bit_offset; /* Bit offset within the register */
>> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */
>> + uint64_t address; /* 64-bit address of struct or register */
>> +} __attribute__ ((packed));
>> +
>
> (13) This structure is already defined, see AcpiGenericAddress.
>
>> +/* Hardware Error Notification */
>> +struct acpi_hest_notify {
>> + uint8_t type;
>> + uint8_t length;
>> + uint16_t config_write_enable;
>> + uint32_t poll_interval;
>> + uint32_t vector;
>> + uint32_t polling_threshold_value;
>> + uint32_t polling_threshold_window;
>> + uint32_t error_threshold_value;
>> + uint32_t error_threshold_window;
>> +};
>
> (14) Please add a comment that this is from the ACPI 6.1 spec, "18.3.2.9
> Hardware Error Notification".
>
> (15) The structure should be called AcpiHestNotify. Please also add a
> direct typedef for it, similarly to the other struct AcpiXxxx types seen
> in this header.
>
> (16) To the "type" field, please append a comment that the values come
> from AcpiHestNotifyType.
ok.
>
> (17) This structure should be packed. Please add QEMU_PACKED between the
> closing brace and the semicolon.
OK, have modified it.
>
>> +
>> +enum acpi_hest_types {
>> + ACPI_HEST_TYPE_IA32_CHECK = 0,
>> + ACPI_HEST_TYPE_IA32_CORRECTED_CHECK = 1,
>> + ACPI_HEST_TYPE_IA32_NMI = 2,
>> + ACPI_HEST_TYPE_NOT_USED3 = 3,
>> + ACPI_HEST_TYPE_NOT_USED4 = 4,
>> + ACPI_HEST_TYPE_NOT_USED5 = 5,
>> + ACPI_HEST_TYPE_AER_ROOT_PORT = 6,
>> + ACPI_HEST_TYPE_AER_ENDPOINT = 7,
>> + ACPI_HEST_TYPE_AER_BRIDGE = 8,
>> + ACPI_HEST_TYPE_GENERIC_ERROR = 9,
>> + ACPI_HEST_TYPE_GENERIC_ERROR_V2 = 10,
>> + ACPI_HEST_TYPE_RESERVED = 11 /* 11 and greater are reserved */
>> +};
>
> (18) Please add a comment that these are from ACPI 6.1, sections
> "18.3.2.1 IA-32 Architecture Machine Check Exception" through "18.3.2.8
> Generic Hardware Error Source version 2".
>
> (19) The type name should be "AcpiHestSourceType" (singular).
>
> (20) I think the enum constants should be renamed to
> ACPI_HEST_SOURCE_xxx, from ACPI_HEST_TYPE_xxx.
>
> (21) I think the NOT_USED{3,4,5} enum constants should be removed.
OK.
>
>> +
>> +/* Values for block_status flags above */
>
> (22) Here I think we should only say, 'Block Status bitmasks from ACPI
> 6.1, "18.3.2.7.1 Generic Error Data"'. The block_status field that you
> refer to is not above, it comes later.
>
>> +#define ACPI_BERT_UNCORRECTABLE (1)
>> +#define ACPI_BERT_CORRECTABLE (1 << 1)
>> +#define ACPI_BERT_MULTIPLE_UNCORRECTABLE (1 << 2)
>> +#define ACPI_BERT_MULTIPLE_CORRECTABLE (1 << 3)
>> +/* 8 bits, error count */
>> +#define ACPI_BERT_ERROR_ENTRY_COUNT (0xFF << 4)
>>
>
> (23) Any particular reason to call these BERT? The "Boot Error Record
> Table" is specified in "18.3.1 Boot Error Source", but the block status
> bitmasks don't look related.
>
> To me ACPI_GEBS_xxx ("generic error block status") seems like a more
> fitting prefix.
good point, it indeed confused other people with such "ACPI_BERT_xxxx" prefix.
>
> +
>> +/* Generic Hardware Error Source Structure */
>> +struct AcpiGenericHardwareErrorSource {
>> + uint16_t type;
>> + uint16_t source_id;
>> + uint16_t related_source_id;
>> + uint8_t flags;
>> + uint8_t enabled;
>> + uint32_t number_of_records;
>> + uint32_t max_sections_per_record;
>> + uint32_t max_raw_data_length;
>> + struct acpi_generic_address error_status_address;
>> + struct acpi_hest_notify notify;
>> + uint32_t error_status_block_length;
>> +} QEMU_PACKED;
>> +typedef struct AcpiGenericHardwareErrorSource AcpiGenericHardwareErrorSource;
>
> (24) This looks good to me in general.
>
> I suggest adding a reference to ACPI 6.1 "18.3.2.7 Generic Hardware
> Error Source". Also, I think we should mention that "type" has to be
> ACPI_HEST_SOURCE_GENERIC_ERROR.
Ok.
>
>> +
>> +/* Generic Hardware Error Source , version 2 */
>> +struct AcpiGenericHardwareErrorSourceV2 {
>> + uint16_t type;
>> + uint16_t source_id;
>> + uint16_t related_source_id;
>> + uint8_t flags;
>> + uint8_t enabled;
>> + uint32_t number_of_records;
>> + uint32_t max_sections_per_record;
>> + uint32_t max_raw_data_length;
>> + struct acpi_generic_address error_status_address;
>> + struct acpi_hest_notify notify;
>> + uint32_t error_status_block_length;
>> + struct acpi_generic_address read_ack_register;
>> + uint64_t read_ack_preserve;
>> + uint64_t read_ack_write;
>> +} QEMU_PACKED;
>> +typedef struct AcpiGenericHardwareErrorSourceV2
>> + AcpiGenericHardwareErrorSourceV2;
>
> (25) Same comments; I suggest adding a reference to "18.3.2.8 Generic
> Hardware Error Source version 2", and mentioning
> ACPI_HEST_SOURCE_GENERIC_ERROR_V2 for the "type" field.
>
>> +
>> +/* Generic Error Status block */
>> +
>> +struct AcpiGenericErrorStatus {
>> + uint32_t block_status;
>> + uint32_t raw_data_offset;
>> + uint32_t raw_data_length;
>> + uint32_t data_length;
>> + uint32_t error_severity;
>> +};
>> +typedef struct AcpiGenericErrorStatus AcpiGenericErrorStatus;
>> +
>
> (26) Please mention that this is from ACPI 6.1, "18.3.2.7.1 Generic
> Error Data".
>
> (27) Near the block_status field, we should point out that it is a
> bitmask composed of ACPI_GEBS_xxx macros.
>
> (28) QEMU_PACKED is missing. (It will make no difference in practice,
> but I recommend it for consistency and documentation purposes.)
OK. will do it.
>
>> +/* Generic Error Data entry */
>> +
>> +struct AcpiGenericErrorData {
>> + uint8_t section_type[16];
>> + uint32_t error_severity;
>> + uint16_t revision;
>> + uint8_t validation_bits;
>> + uint8_t flags;
>> + uint32_t error_data_length;
>> + uint8_t fru_id[16];
>> + uint8_t fru_text[20];
>> +};
>> +typedef struct AcpiGenericErrorData AcpiGenericErrorData;
>
> (29) Please point to ACPI 6.1, "18.3.2.7.1 Generic Error Data" again, in
> the leading comment.
>
> (30) QEMU_PACKED is missing.
>
> (31) I think we should use the QemuUUID type for the "section_type"
> field. And, in order to make it clear that it has little endian
> encoding, let's call it "section_type_le".
>
> An added benefit is that in the code, the field can be set with a simple
> structure assignment from UEFI_CPER_SEC_PLATFORM_MEM (and then can be
> byte-swapped in place, for little endiannes, with qemu_uuid_bswap()).
good suggestion, will follow that.
>
>> +
>> +/* Extension for revision 0x0300 */
>> +struct AcpiGenericErrorDataV300 {
>> + uint8_t section_type[16];
>> + uint32_t error_severity;
>> + uint16_t revision;
>> + uint8_t validation_bits;
>> + uint8_t flags;
>> + uint32_t error_data_length;
>> + uint8_t fru_id[16];
>> + uint8_t fru_text[20];
>> + uint64_t time_stamp;
>> +};
>> +typedef struct AcpiGenericErrorDataV300 AcpiGenericErrorDataV300;
>> +
>
> (32) Same comments as (29), (30), (31) above.
>
> (33) Actually, do we need both AcpiGenericErrorData and
> AcpiGenericErrorDataV300? In the code we seem to be using only the
> former. On the other hand, in the spec I can see only the latter. Where
> does AcpiGenericErrorData come from?
>
The AcpiGenericErrorData come from "ACPI_5.0A", link is here: http://www.acpi.info/spec50a.htm
The revision number of the error data is 0x0201 for "AcpiGenericErrorData"
The revision number of the error data is 0x0300 for "AcpiGenericErrorDataV300"
>> +enum {
>> + CPER_SEV_RECOVERABLE,
>> + CPER_SEV_FATAL,
>> + CPER_SEV_CORRECTED,
>> + CPER_SEV_INFORMATIONAL,
>> +};
>
> (34) I suggest giving a name to this type, for example
> AcpiGenericErrorSeverity.
>
> (35) The enumeration constants should start with ACPI_.
OK.
>
> (36) I suggest moving this type above AcpiGenericErrorData and
> AcpiGenericErrorDataV300, and remarking on the "error_severity" fields
> that they take their values from AcpiGenericErrorSeverity.
>
> (37) Where does "INFORMATIONAL" come from? In ACPI 6.1, "18.3.2.7.1
> Generic Error Data", I see "None" for value 3.
>
>> +
>> +/* Memory Error Section */
>> +struct cper_sec_mem_err {
>> + uint64_t validation_bits;
>> + uint64_t error_status;
>> + uint64_t physical_addr;
>> + uint64_t physical_addr_mask;
>> + uint16_t node;
>> + uint16_t card;
>> + uint16_t module;
>> + uint16_t bank;
>> + uint16_t device;
>> + uint16_t row;
>> + uint16_t column;
>> + uint16_t bit_pos;
>> + uint64_t requestor_id;
>> + uint64_t responder_id;
>> + uint64_t target_id;
>> + uint8_t error_type;
>> + uint8_t reserved;
>> + uint16_t rank;
>> + uint16_t mem_array_handle; /* card handle in UEFI 2.4 */
>> + uint16_t mem_dev_handle; /* module handle in UEFI 2.4 */
>> +};
>> + typedef struct cper_sec_mem_err cper_sec_mem_err;
>
> (38) Please add a comment that is is from UEFI 2.6, "N.2.5 Memory Error
> Section".
>
> (39) The structure and the typedef should be called "UefiCperSecMemErr".
>
> (40) I suggest adding a comment to "validation_bits" that it is a
> bitmask composed of CPER_MEM_VALID_xxx macros.
>
> (41) QEMU_PACKED is missing.
will modify it.
>
>> +
>> +/*
>> + * HEST Description Table
>> + */
>> +struct AcpiHardwareErrorSourceTable {
>> + ACPI_TABLE_HEADER_DEF /* ACPI common table header */
>> + uint32_t error_source_count;
>> +} QEMU_PACKED;
>> +typedef struct AcpiHardwareErrorSourceTable AcpiHardwareErrorSourceTable;
>> +
>> #define ACPI_SRAT_PROCESSOR_APIC 0
>> #define ACPI_SRAT_MEMORY 1
>> #define ACPI_SRAT_PROCESSOR_x2APIC 2
>
> Next file:
>
>> diff --git a/include/hw/acpi/hest_ghes.h b/include/hw/acpi/hest_ghes.h
>> new file mode 100644
>> index 0000000..0cadc2b
>> --- /dev/null
>> +++ b/include/hw/acpi/hest_ghes.h
>> @@ -0,0 +1,43 @@
>> +#ifndef ACPI_GHES_H
>> +#define ACPI_GHES_H
>> +
>> +#include "hw/acpi/bios-linker-loader.h"
>> +
>> +#define GHES_ERRORS_FW_CFG_FILE "etc/hardware_errors"
>> +#define GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
>> +
>> +#define GAS_ADDRESS_OFFSET 4
>> +#define ERROR_STATUS_ADDRESS_OFFSET 20
>> +#define NOTIFICATION_STRUCTURE 32
>> +
>> +#define BFAPEI_OK 0
>> +#define BFAPEI_FAIL 1
>> +
>> +/* The max number of error source, the error sources
>> + * are classified by notification type, below is the definition
>> + * 0 - Polled
>> + * 1 - External Interrupt
>> + * 2 - Local Interrupt
>> + * 3 - SCI
>> + * 4 - NMI
>> + * 5 - CMCI
>> + * 6 - MCE
>> + * 7 - GPIO-Signal
>> + * 8 - ARMv8 SEA
>> + * 9 - ARMv8 SEI
>> + * 10 - External Interrupt - GSIV
>> + */
>> +#define MAX_ERROR_SOURCE_COUNT_V6 11
>
> I'll have to review this header file more thoroughly, once I see the
> code that references these macros. For now, I have one comment:
>
> (42) I think the notification type list should be removed from this
> location. Also, the open-coded value 11 should be replaced with
> the ACPI_HEST_NOTIFY_RESERVED enumeration constant.
agree with you. in your suggested way, it is easy to extend.
>
> I will try to continue reviewing this patch sometime next week (second
> half of the week at the earliest, I think).
Ok, it is not hurry, you can find your free time to review it
>
> Please feel free to disagree with my comments; I prefer to write down
> everything that crosses my mind. It's encouraged to raise
> counter-arguments.
many thanks for your detailed suggestion.
>
> Thanks
> Laszlo
>
>> +/* The max size in Bytes for one error block */
>> +#define MAX_RAW_DATA_LENGTH 0x1000
>> +
>> +typedef struct GhesErrorState {
>> + uint64_t physical_addr;
>> + uint64_t ghes_addr_le[8];
>> +} GhesErrorState;
>> +
>> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
>> + BIOSLinker *linker);
>> +void ghes_add_fw_cfg(FWCfgState *s, GArray *guid);
>> +void ghes_update_guest(uint32_t notify, uint64_t physical_address);
>> +#endif
>>
>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 0835e59..e7ab5dc 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -45,6 +45,8 @@
>> #include "hw/arm/virt.h"
>> #include "sysemu/numa.h"
>> #include "kvm_arm.h"
>> +#include "hw/acpi/vmgenid.h"
>> +#include "hw/acpi/hest_ghes.h"
>>
>> #define ARM_SPI_BASE 32
>> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>> @@ -778,6 +780,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>> acpi_add_table(table_offsets, tables_blob);
>> build_spcr(tables_blob, tables->linker, vms);
>>
>> + acpi_add_table(table_offsets, tables_blob);
>> + ghes_build_acpi(tables_blob, tables->hardware_errors, tables->linker);
>> +
>> if (nb_numa_nodes > 0) {
>> acpi_add_table(table_offsets, tables_blob);
>> build_srat(tables_blob, tables->linker, vms);
>> @@ -892,6 +897,7 @@ void virt_acpi_setup(VirtMachineState *vms)
>>
>> build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
>> ACPI_BUILD_RSDP_FILE, 0);
>> + ghes_add_fw_cfg(vms->fw_cfg, tables.hardware_errors);
>>
>> qemu_register_reset(virt_acpi_build_reset, build_state);
>> virt_acpi_build_reset(build_state);
>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 1e3bd2b..d5f1552 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -121,3 +121,4 @@ CONFIG_ACPI=y
>> CONFIG_SMBIOS=y
>> CONFIG_ASPEED_SOC=y
>> CONFIG_GPIO_KEY=y
>> +CONFIG_ACPI_APEI_GENERATION=y
>
>> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
>> index 11c35bc..776b46e 100644
>> --- a/hw/acpi/Makefile.objs
>> +++ b/hw/acpi/Makefile.objs
>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
>> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>> common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
>> +common-obj-$(CONFIG_ACPI_APEI_GENERATION) += hest_ghes.o
>> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>>
>> common-obj-y += acpi_interface.o
>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index c6f2032..802b98d 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1560,6 +1560,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();
>> }
>>
>> @@ -1570,6 +1571,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..91d382e
>> --- /dev/null
>> +++ b/hw/acpi/hest_ghes.c
>> @@ -0,0 +1,203 @@
>> +/*
>> + * APEI GHES table Generation
>> + *
>> + * Copyright (C) 2017 huawei.
>> + *
>> + * Author: Dongjiu Geng <gengdongjiu@huawei.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qmp-commands.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"
>> +
>> +static int ghes_generate_cper_record(uint64_t block_error_address,
>> + uint64_t error_physical_addr)
>> +{
>> + AcpiGenericErrorStatus block;
>> + AcpiGenericErrorData *gdata;
>> + struct cper_sec_mem_err *mem_err;
>> + uint64_t block_data_length;
>> + unsigned char *buffer;
>> +
>> + cpu_physical_memory_read(block_error_address, &block,
>> + sizeof(AcpiGenericErrorStatus));
>> +
>> + block_data_length = sizeof(AcpiGenericErrorStatus) + block.data_length;
>> +
>> + /* If the Generic Error Status Block is NULL, update
>> + * the block header
>> + */
>> + if (!block.block_status) {
>> + block.block_status = ACPI_BERT_UNCORRECTABLE;
>> + block.error_severity = CPER_SEV_FATAL;
>> + }
>> +
>> + block.data_length += sizeof(AcpiGenericErrorData);
>> + block.data_length += sizeof(struct cper_sec_mem_err);
>> +
>> + /* Write back the Generic Error Status Block to guest memory */
>> + cpu_physical_memory_write(block_error_address, &block,
>> + sizeof(AcpiGenericErrorStatus));
>> +
>> + /* Fill in Generic Error Data Entry */
>> + buffer = g_malloc(sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
>> + memset(buffer, 0, sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
>> + gdata = (AcpiGenericErrorData *)buffer;
>> +
>> + memcpy(gdata->section_type, (void *) &CPER_SEC_PLATFORM_MEM,
>> + sizeof(uuid_le));
>> + gdata->error_data_length = sizeof(struct cper_sec_mem_err);
>> +
>> + mem_err = (struct cper_sec_mem_err *) (gdata + 1);
>> +
>> + /* In order to simplify simulation, hardcode the CPER section to memory
>> + * section.
>> + */
>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
>> + mem_err->error_type = 3;
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_PA;
>> + mem_err->physical_addr = error_physical_addr;
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
>> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
>> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
>> + mem_err->card = 1;
>> + mem_err->module = 2;
>> + mem_err->bank = 3;
>> + mem_err->row = 1;
>> + mem_err->column = 2;
>> + mem_err->bit_pos = 5;
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_STATUS;
>> + mem_err->error_status = 4 << 8;
>> +
>> + /* Write back the Generic Error Data Entry to guest memory */
>> + cpu_physical_memory_write(block_error_address + block_data_length, buffer,
>> + sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
>> +
>> + g_free(buffer);
>> + return BFAPEI_OK;
>> +}
>> +
>> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
>> + BIOSLinker *linker)
>> +{
>> + Aml *hest;
>> + uint32_t address_registers_offset;
>> + AcpiTableHeader *header;
>> + AcpiGenericHardwareErrorSource *error_source;
>> + int i;
>> +
>> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
>> +
>> + /* New address register and Error status block table size*/
>> + g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6
>> + * block_reqr_size);
>> +
>> + /* Put this in a HEST table */
>> + hest = init_aml_allocator();
>> + address_registers_offset = table_data->len
>> + + sizeof(AcpiHardwareErrorSourceTable)
>> + + ERROR_STATUS_ADDRESS_OFFSET
>> + + GAS_ADDRESS_OFFSET;
>> + /* Reserve space for HEST table size*/
>> + acpi_data_push(hest->buf, sizeof(AcpiHardwareErrorSourceTable)
>> + + MAX_ERROR_SOURCE_COUNT_V6
>> + * sizeof(AcpiGenericHardwareErrorSource));
>> +
>> + g_array_append_vals(table_data, hest->buf->data, hest->buf->len);
>> + /* Allocate guest memory for the Data fw_cfg blob */
>> + bios_linker_loader_alloc(linker, GHES_ERRORS_FW_CFG_FILE,
>> + hardware_error, 4096,
>> + false /* page boundary, high memory */);
>> + header = (AcpiTableHeader *)(table_data->data
>> + + table_data->len - hest->buf->len);
>> + *(uint32_t *)(header + 1) = MAX_ERROR_SOURCE_COUNT_V6;
>> + error_source = (AcpiGenericHardwareErrorSource *)((char *)header
>> + + sizeof(AcpiHardwareErrorSourceTable));
>> +
>> + for (i = 0; i < MAX_ERROR_SOURCE_COUNT_V6; i++) {
>> + error_source->type = ACPI_HEST_TYPE_GENERIC_ERROR;
>> + error_source->source_id = 0;
>> + error_source->related_source_id = 0xffff;
>> + error_source->flags = 0;
>> + error_source->enabled = 1;
>> + error_source->number_of_records = 1;
>> + error_source->max_sections_per_record = 1;
>> + error_source->max_raw_data_length = MAX_RAW_DATA_LENGTH;
>> + error_source->error_status_address.space_id =
>> + ACPI_ADR_SPACE_SYSTEM_MEMORY;
>> + error_source->error_status_address.bit_width = 64;
>> + error_source->error_status_address.bit_offset = 0;
>> + error_source->error_status_address.access_width = 4;
>> + error_source->notify.type = i;
>> + error_source->notify.length = sizeof(AcpiGenericHardwareErrorSource);
>> +
>> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>> + sizeof(uint64_t) * i, sizeof(uint64_t),
>> + GHES_ERRORS_FW_CFG_FILE,
>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>> + i * MAX_RAW_DATA_LENGTH);
>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> + address_registers_offset
>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>> + i * sizeof(uint64_t));
>> +
>> + bios_linker_loader_write_pointer(linker, GHES_DATA_ADDR_FW_CFG_FILE,
>> + i * sizeof(uint64_t), sizeof(uint64_t),
>> + GHES_ERRORS_FW_CFG_FILE,
>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>> + i * MAX_RAW_DATA_LENGTH);
>> + error_source++;
>> + }
>> +
>> + build_header(linker, table_data,
>> + (void *)header, "HEST", hest->buf->len, 1, NULL, "GHES");
>> +
>> + free_aml_allocator();
>> +}
>> +
>> +static GhesErrorState ges;
>> +void ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_error)
>> +{
>> +
>> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
>> + int size = MAX_ERROR_SOURCE_COUNT_V6 * block_reqr_size;
>> +
>> + /* Create a read-only fw_cfg file for GHES */
>> + fw_cfg_add_file(s, GHES_ERRORS_FW_CFG_FILE, hardware_error->data,
>> + size);
>> + /* Create a read-write fw_cfg file for Address */
>> + fw_cfg_add_file_callback(s, GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL,
>> + &(ges.ghes_addr_le[0]),
>> + sizeof(uint64_t) * MAX_ERROR_SOURCE_COUNT_V6,
>> + false);
>> +}
>> +
>> +void ghes_update_guest(uint32_t notify, uint64_t physical_address)
>> +{
>> + uint64_t block_error_addr;
>> +
>> + if (physical_address) {
>> + ges.physical_addr = physical_address;
>> + block_error_addr = ges.ghes_addr_le[notify];
>> + block_error_addr = le32_to_cpu(block_error_addr);
>> +
>> + /* A zero value in ghes_addr means that BIOS has not yet written
>> + * the address
>> + */
>> + if (block_error_addr) {
>> + ghes_generate_cper_record(block_error_addr, physical_address);
>> + }
>> + }
>> +}
>
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 00c21f1..c1d15b3 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
>> GArray *rsdp;
>> GArray *tcpalog;
>> GArray *vmgenid;
>> + GArray *hardware_errors;
>> BIOSLinker *linker;
>> } AcpiBuildTables;
>>
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-05-12 21:00 ` [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Laszlo Ersek
2017-05-20 5:35 ` gengdongjiu
@ 2017-05-22 14:23 ` Laszlo Ersek
2017-06-03 12:01 ` Laszlo Ersek
1 sibling, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-22 14:23 UTC (permalink / raw)
To: Dongjiu Geng, james.morse, marc.zyngier, christoffer.dall,
rkrcmar, linux, tbaicar, imammedo, zhaoshenglong, peter.maydell,
pbonzini, qemu-devel, qemu-arm, ard.biesheuvel, mtsirkin, drjones,
ben, kvm, linux-arm-kernel, kvmarm
Cc: songwenjun, wuquanming, huangshaoyu, wangxiongfeng2, xiexiuqi
Keeping some context:
On 05/12/17 23:00, Laszlo Ersek wrote:
> On 04/30/17 07:35, Dongjiu Geng wrote:
>> This implements APEI GHES Table by passing the error cper info
>> to the guest via a fw_cfg_blob. After a CPER info is added, an
>> SEA/SEI exception will be injected into the guest OS.
>>
>> Below is the table layout, the max number of error soure is 11,
>> which is classified by notification type.
>>
>> etc/acpi/tables etc/hardware_errors
>> ================ ==========================================
>> +-----------+
>> +--------------+ | address | +-> +--------------+
>> | HEST + | registers | | | Error Status |
>> + +------------+ | +---------+ | | Data Block 1 |
>> | | GHES1 | --> | |address1 | --------+ | +------------+
>> | | GHES2 | --> | |address2 | ------+ | | CPER |
>> | | GHES3 | --> | |address3 | ----+ | | | CPER |
>> | | .... | --> | | ....... | | | | | CPER |
>> | | GHES10 | --> | |address10| -+ | | | | CPER |
>> +-+------------+ +-+---------+ | | | +-+------------+
>> | | |
>> | | +---> +--------------+
>> | | | Error Status |
>> | | | Data Block 2 |
>> | | | +------------+
>> | | | | CPER |
>> | | | | CPER |
>> | | +-+------------+
>> | |
>> | +-----> +--------------+
>> | | Error Status |
>> | | Data Block 3 |
>> | | +------------+
>> | | | CPER |
>> | +-+------------+
>> | ...........
>> +--------> +--------------+
>> | Error Status |
>> | Data Block 10|
>> | +------------+
>> | | CPER |
>> | | CPER |
>> | | CPER |
>> +-+------------+
>>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>> ---
>> default-configs/arm-softmmu.mak | 1 +
>> hw/acpi/Makefile.objs | 1 +
>> hw/acpi/aml-build.c | 2 +
>> hw/acpi/hest_ghes.c | 203 +++++++++++++++++++++++++++++++++++
>> hw/arm/virt-acpi-build.c | 6 ++
>> include/hw/acpi/acpi-defs.h | 227 ++++++++++++++++++++++++++++++++++++++++
>> include/hw/acpi/aml-build.h | 1 +
>> include/hw/acpi/hest_ghes.h | 43 ++++++++
>> 8 files changed, 484 insertions(+)
>> create mode 100644 hw/acpi/hest_ghes.c
>> create mode 100644 include/hw/acpi/hest_ghes.h
> Next file:
>
>> diff --git a/include/hw/acpi/hest_ghes.h b/include/hw/acpi/hest_ghes.h
>> new file mode 100644
>> index 0000000..0cadc2b
>> --- /dev/null
>> +++ b/include/hw/acpi/hest_ghes.h
>> @@ -0,0 +1,43 @@
>> +#ifndef ACPI_GHES_H
>> +#define ACPI_GHES_H
>> +
>> +#include "hw/acpi/bios-linker-loader.h"
>> +
>> +#define GHES_ERRORS_FW_CFG_FILE "etc/hardware_errors"
>> +#define GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
>> +
>> +#define GAS_ADDRESS_OFFSET 4
>> +#define ERROR_STATUS_ADDRESS_OFFSET 20
>> +#define NOTIFICATION_STRUCTURE 32
>> +
>> +#define BFAPEI_OK 0
>> +#define BFAPEI_FAIL 1
>> +
>> +/* The max number of error source, the error sources
>> + * are classified by notification type, below is the definition
>> + * 0 - Polled
>> + * 1 - External Interrupt
>> + * 2 - Local Interrupt
>> + * 3 - SCI
>> + * 4 - NMI
>> + * 5 - CMCI
>> + * 6 - MCE
>> + * 7 - GPIO-Signal
>> + * 8 - ARMv8 SEA
>> + * 9 - ARMv8 SEI
>> + * 10 - External Interrupt - GSIV
>> + */
>> +#define MAX_ERROR_SOURCE_COUNT_V6 11
>
> I'll have to review this header file more thoroughly, once I see the
> code that references these macros. For now, I have one comment:
>
> (42) I think the notification type list should be removed from this
> location. Also, the open-coded value 11 should be replaced with
> the ACPI_HEST_NOTIFY_RESERVED enumeration constant.
>
> I will try to continue reviewing this patch sometime next week (second
> half of the week at the earliest, I think).
(43) I think all macros introduced in this header should all start with
"GHES_".
>
>> +/* The max size in Bytes for one error block */
>> +#define MAX_RAW_DATA_LENGTH 0x1000
>> +
>> +typedef struct GhesErrorState {
>> + uint64_t physical_addr;
>> + uint64_t ghes_addr_le[8];
>> +} GhesErrorState;
(44) Should this be called "GhesState" instead (dropping "Error")?
(45) (This is a question for other reviewers) I have no idea if this
abstraction should be a device (sysbus or otherwise). Should it be a
device? Below we have a static ("global") variable of this type, which
is quite unusual.
(46) What is "physical_addr" good for? Below I can only see an
assignment to it, in ghes_update_guest(). Where is the field read?
(47) "ghes_addr_le" should be an array of eight uint8_t elements (for
representing a single uint64_t in little endian). The declaration above
has a typo; the element type is currently specified as uint64_t.
It suffices for the firmware to pass the base address of
"etc/hardware_errors" back to QEMU, all the other addresses can be
computed in QEMU as needed.
(If, for some reason you *do* need to pass back multiple addresses, then
you should use
uint8_t xxxx_addr_le[N][8];
But even in this case, setting N to 8 doesn't look useful, because we
have 11 error sources / notification types.)
>> +
>> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
>> + BIOSLinker *linker);
>> +void ghes_add_fw_cfg(FWCfgState *s, GArray *guid);
(48) The second parameter should be called "hardware_errors", not
"guid".
>> +void ghes_update_guest(uint32_t notify, uint64_t physical_address);
(49) Can you call the second parameter "error_physical_addr"?
>> +#endif
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 1e3bd2b..d5f1552 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -121,3 +121,4 @@ CONFIG_ACPI=y
>> CONFIG_SMBIOS=y
>> CONFIG_ASPEED_SOC=y
>> CONFIG_GPIO_KEY=y
>> +CONFIG_ACPI_APEI_GENERATION=y
>> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
>> index 11c35bc..776b46e 100644
>> --- a/hw/acpi/Makefile.objs
>> +++ b/hw/acpi/Makefile.objs
>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
>> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>> common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
>> +common-obj-$(CONFIG_ACPI_APEI_GENERATION) += hest_ghes.o
>> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>>
>> common-obj-y += acpi_interface.o
(50) I think "CONFIG_ACPI_APEI" would be more succinct.
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 00c21f1..c1d15b3 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
>> GArray *rsdp;
>> GArray *tcpalog;
>> GArray *vmgenid;
>> + GArray *hardware_errors;
>> BIOSLinker *linker;
>> } AcpiBuildTables;
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index c6f2032..802b98d 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1560,6 +1560,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();
>> }
>>
>> @@ -1570,6 +1571,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 */
Looks good to me.
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 0835e59..e7ab5dc 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -45,6 +45,8 @@
>> #include "hw/arm/virt.h"
>> #include "sysemu/numa.h"
>> #include "kvm_arm.h"
>> +#include "hw/acpi/vmgenid.h"
(51) I think this include directive is not necessary.
>> +#include "hw/acpi/hest_ghes.h"
>>
>> #define ARM_SPI_BASE 32
>> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>> @@ -778,6 +780,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>> acpi_add_table(table_offsets, tables_blob);
>> build_spcr(tables_blob, tables->linker, vms);
>>
>> + acpi_add_table(table_offsets, tables_blob);
>> + ghes_build_acpi(tables_blob, tables->hardware_errors, tables->linker);
>> +
>> if (nb_numa_nodes > 0) {
>> acpi_add_table(table_offsets, tables_blob);
>> build_srat(tables_blob, tables->linker, vms);
>> @@ -892,6 +897,7 @@ void virt_acpi_setup(VirtMachineState *vms)
>>
>> build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
>> ACPI_BUILD_RSDP_FILE, 0);
>> + ghes_add_fw_cfg(vms->fw_cfg, tables.hardware_errors);
>>
>> qemu_register_reset(virt_acpi_build_reset, build_state);
>> virt_acpi_build_reset(build_state);
(52) I think for consistency with existing code, this function call
should be placed between the ACPI_BUILD_TPMLOG_FILE line and the
acpi_add_rom_blob() line.
>> diff --git a/hw/acpi/hest_ghes.c b/hw/acpi/hest_ghes.c
>> new file mode 100644
>> index 0000000..91d382e
>> --- /dev/null
>> +++ b/hw/acpi/hest_ghes.c
>> @@ -0,0 +1,203 @@
>> +/*
>> + * APEI GHES table Generation
>> + *
>> + * Copyright (C) 2017 huawei.
>> + *
>> + * Author: Dongjiu Geng <gengdongjiu@huawei.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qmp-commands.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"
>> +
>> +static int ghes_generate_cper_record(uint64_t block_error_address,
>> + uint64_t error_physical_addr)
>> +{
>> + AcpiGenericErrorStatus block;
>> + AcpiGenericErrorData *gdata;
>> + struct cper_sec_mem_err *mem_err;
>> + uint64_t block_data_length;
>> + unsigned char *buffer;
>> +
>> + cpu_physical_memory_read(block_error_address, &block,
>> + sizeof(AcpiGenericErrorStatus));
>> +
>> + block_data_length = sizeof(AcpiGenericErrorStatus) + block.data_length;
(53) "block.data_length" must surely be converted from LE to
host-endian, so please wrap it with le32_to_cpu().
>> +
>> + /* If the Generic Error Status Block is NULL, update
>> + * the block header
>> + */
>> + if (!block.block_status) {
>> + block.block_status = ACPI_BERT_UNCORRECTABLE;
>> + block.error_severity = CPER_SEV_FATAL;
>> + }
>> +
>> + block.data_length += sizeof(AcpiGenericErrorData);
>> + block.data_length += sizeof(struct cper_sec_mem_err);
(54) Conversion between LE and host-endian is missing.
(55) What happens if you run out of the preallocated memory?
>> +
>> + /* Write back the Generic Error Status Block to guest memory */
>> + cpu_physical_memory_write(block_error_address, &block,
>> + sizeof(AcpiGenericErrorStatus));
>> +
>> + /* Fill in Generic Error Data Entry */
>> + buffer = g_malloc(sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
>> + memset(buffer, 0, sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
(56) Please use g_malloc0() instead.
>> + gdata = (AcpiGenericErrorData *)buffer;
>> +
>> + memcpy(gdata->section_type, (void *) &CPER_SEC_PLATFORM_MEM,
>> + sizeof(uuid_le));
>> + gdata->error_data_length = sizeof(struct cper_sec_mem_err);
(57) Endianness conversion missing (cpu_to_le32()).
>> +
>> + mem_err = (struct cper_sec_mem_err *) (gdata + 1);
>> +
>> + /* In order to simplify simulation, hardcode the CPER section to memory
>> + * section.
>> + */
>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
>> + mem_err->error_type = 3;
(58) Is this supposed to stand for "Multi-bit ECC" (from "N.2.5 Memory
Error Section" in UEFI 2.6)? Should we have a macro for that?
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_PA;
>> + mem_err->physical_addr = error_physical_addr;
(59) Conversion between host and little endian is missing on all four
lines above.
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
>> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
>> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
>> + mem_err->card = 1;
>> + mem_err->module = 2;
>> + mem_err->bank = 3;
>> + mem_err->row = 1;
>> + mem_err->column = 2;
>> + mem_err->bit_pos = 5;
(60) I have no idea where these values come from.
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_STATUS;
>> + mem_err->error_status = 4 << 8;
>> +
>> + /* Write back the Generic Error Data Entry to guest memory */
>> + cpu_physical_memory_write(block_error_address + block_data_length, buffer,
>> + sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
(61) Please choose a better name for "block_data_length" -- it stands
for the length *before* the increment.
(62) Is it safe to write out the increased block length before writing
the new CPER data?
>> +
>> + g_free(buffer);
>> + return BFAPEI_OK;
>> +}
(63) This return code is not used. Can we remove it perhaps (together
with the BFAPEI_* macros)?
>> +
>> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
>> + BIOSLinker *linker)
>> +{
>> + Aml *hest;
>> + uint32_t address_registers_offset;
>> + AcpiTableHeader *header;
>> + AcpiGenericHardwareErrorSource *error_source;
>> + int i;
>> +
>> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
(64) What does "reqr" stand for?
>> +
>> + /* New address register and Error status block table size*/
>> + g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6
>> + * block_reqr_size);
(65) The QEMU coding style is
g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6 *
block_reqr_size);
That is, the operator is at the end of the line.
Several function calls below are affected by this; please fix them all
up.
Running "scripts/checkpatch.pl" on the patches before you post them
should help catch this kind of problem early.
>> +
>> + /* Put this in a HEST table */
>> + hest = init_aml_allocator();
>> + address_registers_offset = table_data->len
>> + + sizeof(AcpiHardwareErrorSourceTable)
>> + + ERROR_STATUS_ADDRESS_OFFSET
>> + + GAS_ADDRESS_OFFSET;
(66) Please drop the ERROR_STATUS_ADDRESS_OFFSET and GAS_ADDRESS_OFFSET
macros (from the header file as well), and use the following expressions
instead:
...
offsetof(AcpiGenericHardwareErrorSource, error_status_address) +
offsetof(AcpiGenericAddress, address);
>> + /* Reserve space for HEST table size*/
>> + acpi_data_push(hest->buf, sizeof(AcpiHardwareErrorSourceTable)
>> + + MAX_ERROR_SOURCE_COUNT_V6
>> + * sizeof(AcpiGenericHardwareErrorSource));
>> +
>> + g_array_append_vals(table_data, hest->buf->data, hest->buf->len);
(67) HEST is a data table, it contains no AML. Accordingly, you use
"hest" *only* for pushing zero bytes to "table_data".
But that's not a good enough reason to have "hest" at all, or to call
init_aml_allocator() / free_aml_allocator(). Instead, please just
calculate the needed size, and push that many bytes directly to
"table_data".
Your current code already populates those bytes within "table_data", so
that's good.
>> + /* Allocate guest memory for the Data fw_cfg blob */
>> + bios_linker_loader_alloc(linker, GHES_ERRORS_FW_CFG_FILE,
>> + hardware_error, 4096,
>> + false /* page boundary, high memory */);
(68) In the code below, you are not taking an "OVMF header probe
suppressor" into account.
But, we have already planned to replace that quirk with a separate,
dedicated allocation hint or command, so I'm not going to describe what
an "OVMF header probe suppressor" is; instead, I'll describe the
replacement for it.
So please add a new patch to the series that does the following:
- In "hw/acpi/bios-linker-loader.c", modify the documentation of
"BiosLinkerLoaderEntry.alloc.zone". The most significant bit (i.e.,
bit 7), when set, should mean that @alloc.file contains no ACPI
tables. When the bit is clear, no information is given about the
contents of @alloc.file (i.e., same as now).
- This will require a small patch for SeaBIOS: mask off bit 7 of
"entry->alloc.zone" in the switch statement in
romfile_loader_allocate(), "src/fw/romfile_loader.c". SeaBIOS does not
have to care about this bit.
- OVMF will need some code to handle this bit specially, but I will
write that.
- In QEMU, please modify the bios_linker_loader_alloc() function so that
it takes a "noacpi" boolean as well, and set the high bit of
"entry.alloc.zone" from it. The default value for "noacpi" should be
false, at all currently existing call sites.
- And finally, in this patch, we should pass "true" as "noacpi".
>> + header = (AcpiTableHeader *)(table_data->data
>> + + table_data->len - hest->buf->len);
>> + *(uint32_t *)(header + 1) = MAX_ERROR_SOURCE_COUNT_V6;
(69) Ugh, this is ugly. Please access the error source count field
through "AcpiHardwareErrorSourceTable.error_source_count".
>> + error_source = (AcpiGenericHardwareErrorSource *)((char *)header
>> + + sizeof(AcpiHardwareErrorSourceTable));
>> +
>> + for (i = 0; i < MAX_ERROR_SOURCE_COUNT_V6; i++) {
>> + error_source->type = ACPI_HEST_TYPE_GENERIC_ERROR;
>> + error_source->source_id = 0;
(70) I think this is wrong, this identifier should be unique among all
error sources. You should assign cpu_to_le16(i).
>> + error_source->related_source_id = 0xffff;
>> + error_source->flags = 0;
>> + error_source->enabled = 1;
>> + error_source->number_of_records = 1;
(71) Why do we pre-alloc only one record? Don't several CPER objects fit
in an error status data block (of size MAX_RAW_DATA_LENGTH, 0x1000)?
>> + error_source->max_sections_per_record = 1;
>> + error_source->max_raw_data_length = MAX_RAW_DATA_LENGTH;
>> + error_source->error_status_address.space_id =
>> + ACPI_ADR_SPACE_SYSTEM_MEMORY;
>> + error_source->error_status_address.bit_width = 64;
>> + error_source->error_status_address.bit_offset = 0;
>> + error_source->error_status_address.access_width = 4;
>> + error_source->notify.type = i;
OK, this is one byte wide only.
>> + error_source->notify.length = sizeof(AcpiGenericHardwareErrorSource);
(72) This seems to be wrong. The right hand side should be
sizeof(AcpiHestNotify).
>> +
>> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>> + sizeof(uint64_t) * i, sizeof(uint64_t),
>> + GHES_ERRORS_FW_CFG_FILE,
>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>> + i * MAX_RAW_DATA_LENGTH);
(73) Looks good, but please indent the arguments 1 position relative to
the opening paren.
>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> + address_registers_offset
>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>> + i * sizeof(uint64_t));
(74) Passing "sizeof(uint32_t)" as "dst_patched_size" is wrong. The
address registers at the start of the "etc/hardware_errors" blob are
64-bit wide each.
>> +
>> + bios_linker_loader_write_pointer(linker, GHES_DATA_ADDR_FW_CFG_FILE,
>> + i * sizeof(uint64_t), sizeof(uint64_t),
>> + GHES_ERRORS_FW_CFG_FILE,
>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>> + i * MAX_RAW_DATA_LENGTH);
(75) We should create only one WRITE_POINTER command, for the base
address of "etc/hardware_errors". This should be done outside of the
loop.
The base addresses of the individual error status data blocks should be
calculated in ghes_update_guest(), based on the error source /
notification type.
>> + error_source++;
>> + }
>> +
>> + build_header(linker, table_data,
>> + (void *)header, "HEST", hest->buf->len, 1, NULL, "GHES");
(76) Any particular reason for not passing NULL in "oem_table_id"?
>> +
>> + free_aml_allocator();
>> +}
>> +
>> +static GhesErrorState ges;
>> +void ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_error)
>> +{
>> +
>> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
>> + int size = MAX_ERROR_SOURCE_COUNT_V6 * block_reqr_size;
(77) These variable should have type "size_t".
(78) What does "reqr" stand for?
(79) I just noticed that the commit message lists error sources /
notification types 1 through 10 (count=10 in total). However, we have 11
error sources actually. Can you update the commit message so that it
mentions sources 0 through 10?
>> +
>> + /* Create a read-only fw_cfg file for GHES */
>> + fw_cfg_add_file(s, GHES_ERRORS_FW_CFG_FILE, hardware_error->data,
>> + size);
>> + /* Create a read-write fw_cfg file for Address */
>> + fw_cfg_add_file_callback(s, GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL,
>> + &(ges.ghes_addr_le[0]),
>> + sizeof(uint64_t) * MAX_ERROR_SOURCE_COUNT_V6,
>> + false);
>> +}
>> +
(80) The size calculation for GHES_DATA_ADDR_FW_CFG_FILE is incorrect.
First, it does not match the element count 8 that I highlighted under
remark (47). Second (again), passing back just the base address is
sufficient, so 8 uint8_t elements (for representing a single uint64_t in
LE) are enough.
In other words, the size calculation should be
ARRAY_SIZE(ges.ghes_addr_le) here.
>> +void ghes_update_guest(uint32_t notify, uint64_t physical_address)
>> +{
>> + uint64_t block_error_addr;
>> +
>> + if (physical_address) {
>> + ges.physical_addr = physical_address;
>> + block_error_addr = ges.ghes_addr_le[notify];
>> + block_error_addr = le32_to_cpu(block_error_addr);
>> +
>> + /* A zero value in ghes_addr means that BIOS has not yet written
>> + * the address
>> + */
>> + if (block_error_addr) {
>> + ghes_generate_cper_record(block_error_addr, physical_address);
>> + }
>> + }
>> +}
(81) First, the value of "notify" should be range-checked before using it
as an array subscript. If it is outside of the permissible range,
nothing should be done. Second, in order to locate the affected error
status data block, we should move the offset calculation here, from
ghes_build_acpi().
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-05-22 14:23 ` Laszlo Ersek
@ 2017-06-03 12:01 ` Laszlo Ersek
2017-06-24 14:20 ` gengdongjiu
2017-07-07 8:32 ` gengdongjiu
0 siblings, 2 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-06-03 12:01 UTC (permalink / raw)
To: Dongjiu Geng
Cc: james.morse, marc.zyngier, christoffer.dall, rkrcmar, linux,
tbaicar, imammedo, zhaoshenglong, peter.maydell, pbonzini,
qemu-devel, qemu-arm, ard.biesheuvel, mtsirkin, drjones, ben, kvm,
linux-arm-kernel, kvmarm, songwenjun, wuquanming, huangshaoyu,
wangxiongfeng2, xiexiuqi
On 05/22/17 16:23, Laszlo Ersek wrote:
> Keeping some context:
>
> On 05/12/17 23:00, Laszlo Ersek wrote:
>> On 04/30/17 07:35, Dongjiu Geng wrote:
> (68) In the code below, you are not taking an "OVMF header probe
> suppressor" into account.
>
> But, we have already planned to replace that quirk with a separate,
> dedicated allocation hint or command, so I'm not going to describe what
> an "OVMF header probe suppressor" is; instead, I'll describe the
> replacement for it.
>
> [...]
So, the NOACPI allocation hint is a no-go at the moment, based on the
discussion in the following threads:
http://mid.mail-archive.com/20170601112241.2580-1-ard.biesheuvel@linaro.org
http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com
Therefore the header probe suppression remains necessary.
In this case, it is not hard to do, you just have to reorder the
following two ADD_POINTER additions a bit:
>>> +
>>> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>>> + sizeof(uint64_t) * i, sizeof(uint64_t),
>>> + GHES_ERRORS_FW_CFG_FILE,
>>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>>> + i * MAX_RAW_DATA_LENGTH);
This one should be moved out to a separate loop, after the current loop.
>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>> + address_registers_offset
>>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>>> + i * sizeof(uint64_t));
This one should be kept in the first (i.e., current) loop.
The idea is, when you first point the HEST/GHES_n entries in
ACPI_BUILD_TABLE_FILE to the "address registers" in
GHES_ERRORS_FW_CFG_FILE, all those address registers will still be
zero-filled. This will fail the ACPI table header probe in
OvmfPkg/AcpiPlatformDxe, which is what we want.
After this is done, the address registers in GHES_ERRORS_FW_CFG_FILE
should be pointed to the error status data blocks in the same fw_cfg
blob, in a separate loop. (Those error status data blocks will again be
zero-filled, so no ACPI table headers will be mistakenly recognized in
them.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-06-03 12:01 ` Laszlo Ersek
@ 2017-06-24 14:20 ` gengdongjiu
2017-07-07 8:32 ` gengdongjiu
1 sibling, 0 replies; 17+ messages in thread
From: gengdongjiu @ 2017-06-24 14:20 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Dongjiu Geng, mtsirkin, kvm, tbaicar, qemu-devel, kvmarm, ben,
linux, wangxiongfeng2, huangshaoyu, songwenjun, wuquanming,
marc.zyngier, qemu-arm, imammedo, linux-arm-kernel,
ard.biesheuvel, pbonzini
Laszlo,
Thank you very much for your carefully review again and again. I am
following your suggestion.
2017-06-03 20:01 GMT+08:00, Laszlo Ersek <lersek@redhat.com>:
> On 05/22/17 16:23, Laszlo Ersek wrote:
>> Keeping some context:
>>
>> On 05/12/17 23:00, Laszlo Ersek wrote:
>>> On 04/30/17 07:35, Dongjiu Geng wrote:
>
>> (68) In the code below, you are not taking an "OVMF header probe
>> suppressor" into account.
>>
>> But, we have already planned to replace that quirk with a separate,
>> dedicated allocation hint or command, so I'm not going to describe what
>> an "OVMF header probe suppressor" is; instead, I'll describe the
>> replacement for it.
>>
>> [...]
>
> So, the NOACPI allocation hint is a no-go at the moment, based on the
> discussion in the following threads:
>
> http://mid.mail-archive.com/20170601112241.2580-1-ard.biesheuvel@linaro.org
>
> http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com
>
> Therefore the header probe suppression remains necessary.
>
> In this case, it is not hard to do, you just have to reorder the
> following two ADD_POINTER additions a bit:
>
>>>> +
>>>> + bios_linker_loader_add_pointer(linker,
>>>> GHES_ERRORS_FW_CFG_FILE,
>>>> + sizeof(uint64_t) * i,
>>>> sizeof(uint64_t),
>>>> + GHES_ERRORS_FW_CFG_FILE,
>>>> + MAX_ERROR_SOURCE_COUNT_V6 *
>>>> sizeof(uint64_t) +
>>>> + i * MAX_RAW_DATA_LENGTH);
>
> This one should be moved out to a separate loop, after the current loop.
>
>>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>> + address_registers_offset
>>>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>>>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>>>> + i * sizeof(uint64_t));
>
> This one should be kept in the first (i.e., current) loop.
>
> The idea is, when you first point the HEST/GHES_n entries in
> ACPI_BUILD_TABLE_FILE to the "address registers" in
> GHES_ERRORS_FW_CFG_FILE, all those address registers will still be
> zero-filled. This will fail the ACPI table header probe in
> OvmfPkg/AcpiPlatformDxe, which is what we want.
>
> After this is done, the address registers in GHES_ERRORS_FW_CFG_FILE
> should be pointed to the error status data blocks in the same fw_cfg
> blob, in a separate loop. (Those error status data blocks will again be
> zero-filled, so no ACPI table headers will be mistakenly recognized in
> them.)
>
> Thanks
> Laszlo
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-06-03 12:01 ` Laszlo Ersek
2017-06-24 14:20 ` gengdongjiu
@ 2017-07-07 8:32 ` gengdongjiu
2017-07-07 9:43 ` Laszlo Ersek
1 sibling, 1 reply; 17+ messages in thread
From: gengdongjiu @ 2017-07-07 8:32 UTC (permalink / raw)
To: Laszlo Ersek
Cc: peter.maydell, mtsirkin, kvm, rkrcmar, tbaicar, qemu-devel,
kvmarm, ben, linux, wangxiongfeng2, huangshaoyu, zhaoshenglong,
songwenjun, drjones, wuquanming, xiexiuqi, marc.zyngier, qemu-arm,
imammedo, linux-arm-kernel, ard.biesheuvel, james.morse, pbonzini,
christoffer.dall
Hi Laszlo,
sorry for my late response.
On 2017/6/3 20:01, Laszlo Ersek wrote:
> On 05/22/17 16:23, Laszlo Ersek wrote:
>> Keeping some context:
>>
>> On 05/12/17 23:00, Laszlo Ersek wrote:
>>> On 04/30/17 07:35, Dongjiu Geng wrote:
>
>> (68) In the code below, you are not taking an "OVMF header probe
>> suppressor" into account.
>>
>> But, we have already planned to replace that quirk with a separate,
>> dedicated allocation hint or command, so I'm not going to describe what
>> an "OVMF header probe suppressor" is; instead, I'll describe the
>> replacement for it.
>>
>> [...]
>
> So, the NOACPI allocation hint is a no-go at the moment, based on the
> discussion in the following threads:
>
> http://mid.mail-archive.com/20170601112241.2580-1-ard.biesheuvel@linaro.org
>
> http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com
>
> Therefore the header probe suppression remains necessary.
>
> In this case, it is not hard to do, you just have to reorder the
> following two ADD_POINTER additions a bit:
Ok, it is no problem.
>
>>>> +
>>>> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>>>> + sizeof(uint64_t) * i, sizeof(uint64_t),
>>>> + GHES_ERRORS_FW_CFG_FILE,
>>>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>>>> + i * MAX_RAW_DATA_LENGTH);
>
> This one should be moved out to a separate loop, after the current loop.
>
>>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>> + address_registers_offset
>>>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>>>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>>>> + i * sizeof(uint64_t));
>
> This one should be kept in the first (i.e., current) loop.
>
> The idea is, when you first point the HEST/GHES_n entries in
> ACPI_BUILD_TABLE_FILE to the "address registers" in
> GHES_ERRORS_FW_CFG_FILE, all those address registers will still be
> zero-filled. This will fail the ACPI table header probe in
> OvmfPkg/AcpiPlatformDxe, which is what we want.
>
> After this is done, the address registers in GHES_ERRORS_FW_CFG_FILE
> should be pointed to the error status data blocks in the same fw_cfg
> blob, in a separate loop. (Those error status data blocks will again be
> zero-filled, so no ACPI table headers will be mistakenly recognized in
> them.)
I understand your idear. but I have a question:
how about we exchange the two function's place, such as shown below:
then it still meets ours needs, the change is easy.
For every loop:
(1)when patch address in ACPI_BUILD_TABLE_FILE to the "address registers", the address register is zero-filed.
(2)when patch address in GHES_ERRORS_FW_CFG_FILE to the error status data blocks, the error status data block is still zero-filed.
for (i = 0; i < GHES_ACPI_HEST_NOTIFY_RESERVED; i++) {
.....................................
bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
address_registers_offset
+ i * sizeof(AcpiGenericHardwareErrorSource),
sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
i * sizeof(uint64_t));
bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
sizeof(uint64_t) * i, sizeof(uint64_t),
GHES_ERRORS_FW_CFG_FILE,
MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
i * MAX_RAW_DATA_LENGTH);
.........................................
}
thanks
>
> Thanks
> Laszlo
>
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-07-07 8:32 ` gengdongjiu
@ 2017-07-07 9:43 ` Laszlo Ersek
2017-07-09 3:41 ` gengdongjiu
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-07-07 9:43 UTC (permalink / raw)
To: gengdongjiu
Cc: peter.maydell, mtsirkin, kvm, rkrcmar, tbaicar, qemu-devel,
kvmarm, ben, linux, wangxiongfeng2, huangshaoyu, zhaoshenglong,
songwenjun, drjones, wuquanming, xiexiuqi, marc.zyngier, qemu-arm,
imammedo, linux-arm-kernel, ard.biesheuvel, james.morse, pbonzini,
christoffer.dall
On 07/07/17 10:32, gengdongjiu wrote:
> Hi Laszlo,
> sorry for my late response.
>
> On 2017/6/3 20:01, Laszlo Ersek wrote:
>> On 05/22/17 16:23, Laszlo Ersek wrote:
>>> Keeping some context:
>>>
>>> On 05/12/17 23:00, Laszlo Ersek wrote:
>>>> On 04/30/17 07:35, Dongjiu Geng wrote:
>>
>>> (68) In the code below, you are not taking an "OVMF header probe
>>> suppressor" into account.
>>>
>>> But, we have already planned to replace that quirk with a separate,
>>> dedicated allocation hint or command, so I'm not going to describe what
>>> an "OVMF header probe suppressor" is; instead, I'll describe the
>>> replacement for it.
>>>
>>> [...]
>>
>> So, the NOACPI allocation hint is a no-go at the moment, based on the
>> discussion in the following threads:
>>
>> http://mid.mail-archive.com/20170601112241.2580-1-ard.biesheuvel@linaro.org
>>
>> http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com
>>
>> Therefore the header probe suppression remains necessary.
>>
>> In this case, it is not hard to do, you just have to reorder the
>> following two ADD_POINTER additions a bit:
> Ok, it is no problem.
>
>>
>>>>> +
>>>>> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>>>>> + sizeof(uint64_t) * i, sizeof(uint64_t),
>>>>> + GHES_ERRORS_FW_CFG_FILE,
>>>>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>>>>> + i * MAX_RAW_DATA_LENGTH);
>>
>> This one should be moved out to a separate loop, after the current loop.
>>
>>>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>>>> + address_registers_offset
>>>>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>>>>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>>>>> + i * sizeof(uint64_t));
>>
>> This one should be kept in the first (i.e., current) loop.
>>
>> The idea is, when you first point the HEST/GHES_n entries in
>> ACPI_BUILD_TABLE_FILE to the "address registers" in
>> GHES_ERRORS_FW_CFG_FILE, all those address registers will still be
>> zero-filled. This will fail the ACPI table header probe in
>> OvmfPkg/AcpiPlatformDxe, which is what we want.
>>
>> After this is done, the address registers in GHES_ERRORS_FW_CFG_FILE
>> should be pointed to the error status data blocks in the same fw_cfg
>> blob, in a separate loop. (Those error status data blocks will again be
>> zero-filled, so no ACPI table headers will be mistakenly recognized in
>> them.)
> I understand your idear. but I have a question:
> how about we exchange the two function's place, such as shown below:
> then it still meets ours needs, the change is easy.
> For every loop:
> (1)when patch address in ACPI_BUILD_TABLE_FILE to the "address registers", the address register is zero-filed.
> (2)when patch address in GHES_ERRORS_FW_CFG_FILE to the error status data blocks, the error status data block is still zero-filed.
>
> for (i = 0; i < GHES_ACPI_HEST_NOTIFY_RESERVED; i++) {
> .....................................
> bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> address_registers_offset
> + i * sizeof(AcpiGenericHardwareErrorSource),
> sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
> i * sizeof(uint64_t));
>
>
> bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
> sizeof(uint64_t) * i, sizeof(uint64_t),
> GHES_ERRORS_FW_CFG_FILE,
> MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
> i * MAX_RAW_DATA_LENGTH);
> .........................................
>
> }
Your suggestion seems to do the same, but there is a subtle difference.
When the firmware scans the targets of the ADD_POINTER commands for byte
sequences that "look like" an ACPI table header, in order to suppress
that probe, we should keep 36 bytes (i.e., the size of the ACPI table
header structure) zeroed at the target location.
* In the patch, as posted, this was not the case, because it first
filled in the address register inside the GHES_ERRORS_FW_CFG_FILE blob,
and then pointed a GHES pointer field in the HEST table to the *now*
nonzero address register.
* My suggestion was to first fill in all the GHES pointers (when still
all the pointed-to address registers are zero), and then the address
registers themselves should be patched.
Under this scheme, it is irrelevant whether both loops are incrementing
or decrementing loops.
* Your suggestion would preserve the safe patching order between any
given GHES field and its corresponding (pointed-to) address register. So
that's great.
However, consider the following case:
- Assume that the loop is decrementing, and not incrementing.
- First, the last GHES pointer field, GHES[9].<whatever> is patched. It
is pointed to the last address register in "etc/hardware_errors", which
is still zero-filled. Also, the last address register in
"etc/hardware_errors" is immediately followed by the first Error Status
Data Block, which is also filled with zero. This means that when the
header probe will look at the *target* of GHES[9].<whatever>, it will
definitely fail (which is what we want), because all consecutive 36
bytes that start at the last address register in "etc/hardware_errors"
are zero.
- Second, the last address register, address[9], is patched. It is
pointed to the last Error Status Data Block, which is zero filled, so
the header probe in the firmware fails -- great.
- Third, GHES[8].<whatever> is patched. It is set to &address[8].
However, at this point, the 36 consecutive bytes starting at &address[8]
are *not* all zero, because at relative offset 8, we have the already
patched address[9] register, which is nonzero. I'm not saying that this
would necessarily cause a problem, but we'd better be safe than sorry.
In other words, your suggestion is OK, because the loop we have is
incrementing, not decrementing. But, if you decide to go with your idea,
please add a comment before the "for" instruction, "this loop must
remain an incrementing loop, to safely suppress the ACPI SDT header
probe in the firmware".
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-07-07 9:43 ` Laszlo Ersek
@ 2017-07-09 3:41 ` gengdongjiu
0 siblings, 0 replies; 17+ messages in thread
From: gengdongjiu @ 2017-07-09 3:41 UTC (permalink / raw)
To: Laszlo Ersek
Cc: gengdongjiu, mtsirkin, kvm, tbaicar, qemu-devel, wangxiongfeng2,
ben, linux, kvmarm, huangshaoyu, songwenjun, wuquanming,
marc.zyngier, qemu-arm, pbonzini, linux-arm-kernel,
ard.biesheuvel, imammedo
Laszlo,
thanks for your clear and detailed answer. I completely understand
what you mean.
2017-07-07 17:43 GMT+08:00, Laszlo Ersek <lersek@redhat.com>:
> On 07/07/17 10:32, gengdongjiu wrote:
>> Hi Laszlo,
>> sorry for my late response.
>>
>> On 2017/6/3 20:01, Laszlo Ersek wrote:
>>> On 05/22/17 16:23, Laszlo Ersek wrote:
>>>> Keeping some context:
>>>>
>>>> On 05/12/17 23:00, Laszlo Ersek wrote:
>>>>> On 04/30/17 07:35, Dongjiu Geng wrote:
>>>
>>>> (68) In the code below, you are not taking an "OVMF header probe
>>>> suppressor" into account.
>>>>
>>>> But, we have already planned to replace that quirk with a separate,
>>>> dedicated allocation hint or command, so I'm not going to describe what
>>>> an "OVMF header probe suppressor" is; instead, I'll describe the
>>>> replacement for it.
>>>>
>>>> [...]
>>>
>>> So, the NOACPI allocation hint is a no-go at the moment, based on the
>>> discussion in the following threads:
>>>
>>> http://mid.mail-archive.com/20170601112241.2580-1-ard.biesheuvel@linaro.org
>>>
>>> http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com
>>>
>>> Therefore the header probe suppression remains necessary.
>>>
>>> In this case, it is not hard to do, you just have to reorder the
>>> following two ADD_POINTER additions a bit:
>> Ok, it is no problem.
>>
>>>
>>>>>> +
>>>>>> + bios_linker_loader_add_pointer(linker,
>>>>>> GHES_ERRORS_FW_CFG_FILE,
>>>>>> + sizeof(uint64_t) * i,
>>>>>> sizeof(uint64_t),
>>>>>> + GHES_ERRORS_FW_CFG_FILE,
>>>>>> + MAX_ERROR_SOURCE_COUNT_V6 *
>>>>>> sizeof(uint64_t) +
>>>>>> + i * MAX_RAW_DATA_LENGTH);
>>>
>>> This one should be moved out to a separate loop, after the current loop.
>>>
>>>>>> + bios_linker_loader_add_pointer(linker,
>>>>>> ACPI_BUILD_TABLE_FILE,
>>>>>> + address_registers_offset
>>>>>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>>>>>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>>>>>> + i * sizeof(uint64_t));
>>>
>>> This one should be kept in the first (i.e., current) loop.
>>>
>>> The idea is, when you first point the HEST/GHES_n entries in
>>> ACPI_BUILD_TABLE_FILE to the "address registers" in
>>> GHES_ERRORS_FW_CFG_FILE, all those address registers will still be
>>> zero-filled. This will fail the ACPI table header probe in
>>> OvmfPkg/AcpiPlatformDxe, which is what we want.
>>>
>>> After this is done, the address registers in GHES_ERRORS_FW_CFG_FILE
>>> should be pointed to the error status data blocks in the same fw_cfg
>>> blob, in a separate loop. (Those error status data blocks will again be
>>> zero-filled, so no ACPI table headers will be mistakenly recognized in
>>> them.)
>> I understand your idear. but I have a question:
>> how about we exchange the two function's place, such as shown below:
>> then it still meets ours needs, the change is easy.
>> For every loop:
>> (1)when patch address in ACPI_BUILD_TABLE_FILE to the "address registers",
>> the address register is zero-filed.
>> (2)when patch address in GHES_ERRORS_FW_CFG_FILE to the error status data
>> blocks, the error status data block is still zero-filed.
>>
>> for (i = 0; i < GHES_ACPI_HEST_NOTIFY_RESERVED; i++) {
>> .....................................
>> bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> address_registers_offset
>> + i * sizeof(AcpiGenericHardwareErrorSource),
>> sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>> i * sizeof(uint64_t));
>>
>>
>> bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>> sizeof(uint64_t) * i, sizeof(uint64_t),
>> GHES_ERRORS_FW_CFG_FILE,
>> MAX_ERROR_SOURCE_COUNT_V6 *
>> sizeof(uint64_t) +
>> i * MAX_RAW_DATA_LENGTH);
>> .........................................
>>
>> }
>
> Your suggestion seems to do the same, but there is a subtle difference.
>
> When the firmware scans the targets of the ADD_POINTER commands for byte
> sequences that "look like" an ACPI table header, in order to suppress
> that probe, we should keep 36 bytes (i.e., the size of the ACPI table
> header structure) zeroed at the target location.
>
> * In the patch, as posted, this was not the case, because it first
> filled in the address register inside the GHES_ERRORS_FW_CFG_FILE blob,
> and then pointed a GHES pointer field in the HEST table to the *now*
> nonzero address register.
>
> * My suggestion was to first fill in all the GHES pointers (when still
> all the pointed-to address registers are zero), and then the address
> registers themselves should be patched.
>
> Under this scheme, it is irrelevant whether both loops are incrementing
> or decrementing loops.
>
> * Your suggestion would preserve the safe patching order between any
> given GHES field and its corresponding (pointed-to) address register. So
> that's great.
>
> However, consider the following case:
>
> - Assume that the loop is decrementing, and not incrementing.
>
> - First, the last GHES pointer field, GHES[9].<whatever> is patched. It
> is pointed to the last address register in "etc/hardware_errors", which
> is still zero-filled. Also, the last address register in
> "etc/hardware_errors" is immediately followed by the first Error Status
> Data Block, which is also filled with zero. This means that when the
> header probe will look at the *target* of GHES[9].<whatever>, it will
> definitely fail (which is what we want), because all consecutive 36
> bytes that start at the last address register in "etc/hardware_errors"
> are zero.
>
> - Second, the last address register, address[9], is patched. It is
> pointed to the last Error Status Data Block, which is zero filled, so
> the header probe in the firmware fails -- great.
>
> - Third, GHES[8].<whatever> is patched. It is set to &address[8].
> However, at this point, the 36 consecutive bytes starting at &address[8]
> are *not* all zero, because at relative offset 8, we have the already
> patched address[9] register, which is nonzero. I'm not saying that this
> would necessarily cause a problem, but we'd better be safe than sorry.
yes, it it. when the loop is decrementing, my suggested method will
have problem.
>
>
> In other words, your suggestion is OK, because the loop we have is
> incrementing, not decrementing. But, if you decide to go with your idea,
> please add a comment before the "for" instruction, "this loop must
> remain an incrementing loop, to safely suppress the ACPI SDT header
> probe in the firmware".
Laszlo, I will use your method that separated them into different loop
to avoid the potential problem, thanks.
>
> Thanks
> Laszlo
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-04-30 5:35 [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Dongjiu Geng
` (3 preceding siblings ...)
2017-05-12 21:00 ` [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Laszlo Ersek
@ 2017-05-12 23:59 ` Michael S. Tsirkin
2017-05-20 5:47 ` gengdongjiu
4 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2017-05-12 23:59 UTC (permalink / raw)
To: Dongjiu Geng
Cc: james.morse, marc.zyngier, christoffer.dall, rkrcmar, linux,
tbaicar, imammedo, zhaoshenglong, peter.maydell, pbonzini,
qemu-devel, qemu-arm, lersek, ard.biesheuvel, drjones, ben, kvm,
linux-arm-kernel, kvmarm, xiexiuqi, wangxiongfeng2, songwenjun,
wuquanming, huangshaoyu
On Sun, Apr 30, 2017 at 01:35:03PM +0800, Dongjiu Geng wrote:
> This implements APEI GHES Table by passing the error cper info
> to the guest via a fw_cfg_blob. After a CPER info is added, an
> SEA/SEI exception will be injected into the guest OS.
>
> Below is the table layout, the max number of error soure is 11,
> which is classified by notification type.
>
> etc/acpi/tables etc/hardware_errors
> ================ ==========================================
> +-----------+
> +--------------+ | address | +-> +--------------+
> | HEST + | registers | | | Error Status |
> + +------------+ | +---------+ | | Data Block 1 |
> | | GHES1 | --> | |address1 | --------+ | +------------+
> | | GHES2 | --> | |address2 | ------+ | | CPER |
> | | GHES3 | --> | |address3 | ----+ | | | CPER |
> | | .... | --> | | ....... | | | | | CPER |
> | | GHES10 | --> | |address10| -+ | | | | CPER |
> +-+------------+ +-+---------+ | | | +-+------------+
> | | |
> | | +---> +--------------+
> | | | Error Status |
> | | | Data Block 2 |
> | | | +------------+
> | | | | CPER |
> | | | | CPER |
> | | +-+------------+
> | |
> | +-----> +--------------+
> | | Error Status |
> | | Data Block 3 |
> | | +------------+
> | | | CPER |
> | +-+------------+
> | ...........
> +--------> +--------------+
> | Error Status |
> | Data Block 10|
> | +------------+
> | | CPER |
> | | CPER |
> | | CPER |
> +-+------------+
>
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> ---
> default-configs/arm-softmmu.mak | 1 +
> hw/acpi/Makefile.objs | 1 +
> hw/acpi/aml-build.c | 2 +
> hw/acpi/hest_ghes.c | 203 +++++++++++++++++++++++++++++++++++
> hw/arm/virt-acpi-build.c | 6 ++
> include/hw/acpi/acpi-defs.h | 227 ++++++++++++++++++++++++++++++++++++++++
> include/hw/acpi/aml-build.h | 1 +
> include/hw/acpi/hest_ghes.h | 43 ++++++++
> 8 files changed, 484 insertions(+)
> create mode 100644 hw/acpi/hest_ghes.c
> create mode 100644 include/hw/acpi/hest_ghes.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 1e3bd2b..d5f1552 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -121,3 +121,4 @@ CONFIG_ACPI=y
> CONFIG_SMBIOS=y
> CONFIG_ASPEED_SOC=y
> CONFIG_GPIO_KEY=y
> +CONFIG_ACPI_APEI_GENERATION=y
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 11c35bc..776b46e 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
> common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
> +common-obj-$(CONFIG_ACPI_APEI_GENERATION) += hest_ghes.o
> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>
> common-obj-y += acpi_interface.o
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index c6f2032..802b98d 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1560,6 +1560,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();
> }
>
> @@ -1570,6 +1571,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..91d382e
> --- /dev/null
> +++ b/hw/acpi/hest_ghes.c
> @@ -0,0 +1,203 @@
> +/*
> + * APEI GHES table Generation
> + *
> + * Copyright (C) 2017 huawei.
> + *
> + * Author: Dongjiu Geng <gengdongjiu@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qmp-commands.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"
> +
> +static int ghes_generate_cper_record(uint64_t block_error_address,
> + uint64_t error_physical_addr)
> +{
> + AcpiGenericErrorStatus block;
> + AcpiGenericErrorData *gdata;
> + struct cper_sec_mem_err *mem_err;
> + uint64_t block_data_length;
> + unsigned char *buffer;
> +
> + cpu_physical_memory_read(block_error_address, &block,
> + sizeof(AcpiGenericErrorStatus));
> +
> + block_data_length = sizeof(AcpiGenericErrorStatus) + block.data_length;
> +
> + /* If the Generic Error Status Block is NULL, update
> + * the block header
> + */
> + if (!block.block_status) {
> + block.block_status = ACPI_BERT_UNCORRECTABLE;
> + block.error_severity = CPER_SEV_FATAL;
> + }
> +
> + block.data_length += sizeof(AcpiGenericErrorData);
> + block.data_length += sizeof(struct cper_sec_mem_err);
> +
> + /* Write back the Generic Error Status Block to guest memory */
> + cpu_physical_memory_write(block_error_address, &block,
> + sizeof(AcpiGenericErrorStatus));
> +
> + /* Fill in Generic Error Data Entry */
> + buffer = g_malloc(sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
> + memset(buffer, 0, sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
> + gdata = (AcpiGenericErrorData *)buffer;
> +
> + memcpy(gdata->section_type, (void *) &CPER_SEC_PLATFORM_MEM,
> + sizeof(uuid_le));
> + gdata->error_data_length = sizeof(struct cper_sec_mem_err);
> +
> + mem_err = (struct cper_sec_mem_err *) (gdata + 1);
> +
> + /* In order to simplify simulation, hardcode the CPER section to memory
> + * section.
> + */
> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
> + mem_err->error_type = 3;
> +
> + mem_err->validation_bits |= CPER_MEM_VALID_PA;
> + mem_err->physical_addr = error_physical_addr;
> +
> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
> + mem_err->card = 1;
> + mem_err->module = 2;
> + mem_err->bank = 3;
> + mem_err->row = 1;
> + mem_err->column = 2;
> + mem_err->bit_pos = 5;
> +
> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_STATUS;
> + mem_err->error_status = 4 << 8;
> +
> + /* Write back the Generic Error Data Entry to guest memory */
> + cpu_physical_memory_write(block_error_address + block_data_length, buffer,
> + sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
> +
> + g_free(buffer);
> + return BFAPEI_OK;
> +}
> +
> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
> + BIOSLinker *linker)
> +{
> + Aml *hest;
> + uint32_t address_registers_offset;
> + AcpiTableHeader *header;
> + AcpiGenericHardwareErrorSource *error_source;
> + int i;
> +
> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
> +
> + /* New address register and Error status block table size*/
> + g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6
> + * block_reqr_size);
> +
> + /* Put this in a HEST table */
> + hest = init_aml_allocator();
> + address_registers_offset = table_data->len
> + + sizeof(AcpiHardwareErrorSourceTable)
> + + ERROR_STATUS_ADDRESS_OFFSET
> + + GAS_ADDRESS_OFFSET;
> + /* Reserve space for HEST table size*/
> + acpi_data_push(hest->buf, sizeof(AcpiHardwareErrorSourceTable)
> + + MAX_ERROR_SOURCE_COUNT_V6
> + * sizeof(AcpiGenericHardwareErrorSource));
> +
> + g_array_append_vals(table_data, hest->buf->data, hest->buf->len);
> + /* Allocate guest memory for the Data fw_cfg blob */
> + bios_linker_loader_alloc(linker, GHES_ERRORS_FW_CFG_FILE,
> + hardware_error, 4096,
> + false /* page boundary, high memory */);
> + header = (AcpiTableHeader *)(table_data->data
> + + table_data->len - hest->buf->len);
> + *(uint32_t *)(header + 1) = MAX_ERROR_SOURCE_COUNT_V6;
> + error_source = (AcpiGenericHardwareErrorSource *)((char *)header
> + + sizeof(AcpiHardwareErrorSourceTable));
> +
> + for (i = 0; i < MAX_ERROR_SOURCE_COUNT_V6; i++) {
> + error_source->type = ACPI_HEST_TYPE_GENERIC_ERROR;
> + error_source->source_id = 0;
> + error_source->related_source_id = 0xffff;
> + error_source->flags = 0;
> + error_source->enabled = 1;
> + error_source->number_of_records = 1;
> + error_source->max_sections_per_record = 1;
> + error_source->max_raw_data_length = MAX_RAW_DATA_LENGTH;
> + error_source->error_status_address.space_id =
> + ACPI_ADR_SPACE_SYSTEM_MEMORY;
> + error_source->error_status_address.bit_width = 64;
> + error_source->error_status_address.bit_offset = 0;
> + error_source->error_status_address.access_width = 4;
> + error_source->notify.type = i;
> + error_source->notify.length = sizeof(AcpiGenericHardwareErrorSource);
> +
> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
> + sizeof(uint64_t) * i, sizeof(uint64_t),
> + GHES_ERRORS_FW_CFG_FILE,
> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
> + i * MAX_RAW_DATA_LENGTH);
> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> + address_registers_offset
> + + i * sizeof(AcpiGenericHardwareErrorSource),
> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
> + i * sizeof(uint64_t));
> +
> + bios_linker_loader_write_pointer(linker, GHES_DATA_ADDR_FW_CFG_FILE,
> + i * sizeof(uint64_t), sizeof(uint64_t),
> + GHES_ERRORS_FW_CFG_FILE,
> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
> + i * MAX_RAW_DATA_LENGTH);
> + error_source++;
> + }
> +
> + build_header(linker, table_data,
> + (void *)header, "HEST", hest->buf->len, 1, NULL, "GHES");
> +
> + free_aml_allocator();
> +}
> +
> +static GhesErrorState ges;
> +void ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_error)
> +{
> +
> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
> + int size = MAX_ERROR_SOURCE_COUNT_V6 * block_reqr_size;
> +
> + /* Create a read-only fw_cfg file for GHES */
> + fw_cfg_add_file(s, GHES_ERRORS_FW_CFG_FILE, hardware_error->data,
> + size);
> + /* Create a read-write fw_cfg file for Address */
> + fw_cfg_add_file_callback(s, GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL,
> + &(ges.ghes_addr_le[0]),
> + sizeof(uint64_t) * MAX_ERROR_SOURCE_COUNT_V6,
> + false);
> +}
> +
> +void ghes_update_guest(uint32_t notify, uint64_t physical_address)
> +{
> + uint64_t block_error_addr;
> +
> + if (physical_address) {
> + ges.physical_addr = physical_address;
> + block_error_addr = ges.ghes_addr_le[notify];
> + block_error_addr = le32_to_cpu(block_error_addr);
> +
> + /* A zero value in ghes_addr means that BIOS has not yet written
> + * the address
> + */
> + if (block_error_addr) {
> + ghes_generate_cper_record(block_error_addr, physical_address);
> + }
> + }
> +}
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0835e59..e7ab5dc 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -45,6 +45,8 @@
> #include "hw/arm/virt.h"
> #include "sysemu/numa.h"
> #include "kvm_arm.h"
> +#include "hw/acpi/vmgenid.h"
> +#include "hw/acpi/hest_ghes.h"
>
> #define ARM_SPI_BASE 32
> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> @@ -778,6 +780,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> acpi_add_table(table_offsets, tables_blob);
> build_spcr(tables_blob, tables->linker, vms);
>
> + acpi_add_table(table_offsets, tables_blob);
> + ghes_build_acpi(tables_blob, tables->hardware_errors, tables->linker);
> +
> if (nb_numa_nodes > 0) {
> acpi_add_table(table_offsets, tables_blob);
> build_srat(tables_blob, tables->linker, vms);
> @@ -892,6 +897,7 @@ void virt_acpi_setup(VirtMachineState *vms)
>
> build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
> ACPI_BUILD_RSDP_FILE, 0);
> + ghes_add_fw_cfg(vms->fw_cfg, tables.hardware_errors);
>
> qemu_register_reset(virt_acpi_build_reset, build_state);
> virt_acpi_build_reset(build_state);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 4cc3630..27adede 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -295,6 +295,58 @@ typedef struct AcpiMultipleApicTable AcpiMultipleApicTable;
> #define ACPI_APIC_GENERIC_TRANSLATOR 15
> #define ACPI_APIC_RESERVED 16 /* 16 and greater are reserved */
>
> +#define CPER_MEM_VALID_ERROR_STATUS 0x0001
> +#define CPER_MEM_VALID_PA 0x0002
> +#define CPER_MEM_VALID_PA_MASK 0x0004
> +#define CPER_MEM_VALID_NODE 0x0008
> +#define CPER_MEM_VALID_CARD 0x0010
> +#define CPER_MEM_VALID_MODULE 0x0020
> +#define CPER_MEM_VALID_BANK 0x0040
> +#define CPER_MEM_VALID_DEVICE 0x0080
> +#define CPER_MEM_VALID_ROW 0x0100
> +#define CPER_MEM_VALID_COLUMN 0x0200
> +#define CPER_MEM_VALID_BIT_POSITION 0x0400
> +#define CPER_MEM_VALID_REQUESTOR_ID 0x0800
> +#define CPER_MEM_VALID_RESPONDER_ID 0x1000
> +#define CPER_MEM_VALID_TARGET_ID 0x2000
> +#define CPER_MEM_VALID_ERROR_TYPE 0x4000
> +#define CPER_MEM_VALID_RANK_NUMBER 0x8000
> +#define CPER_MEM_VALID_CARD_HANDLE 0x10000
> +#define CPER_MEM_VALID_MODULE_HANDLE 0x20000
> +
> +typedef struct {
> + uint8_t b[16];
> +} uuid_le;
> +
> +#define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
> +((uuid_le) \
> +{{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
> + (b) & 0xff, ((b) >> 8) & 0xff, \
> + (c) & 0xff, ((c) >> 8) & 0xff, \
> + (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) } })
> +
> +/* Platform Memory */
> +#define CPER_SEC_PLATFORM_MEM \
> + UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \
> + 0xED, 0x7C, 0x83, 0xB1)
> +
There's no reason to define these messy one-time use macros.
They just make it hard to look things up in the spec.
You can use build_append_int_noprefix to add data of
any length in LE format.
> +/* Values for Notify Type field above */
> +
> +enum acpi_hest_notify_types {
> + ACPI_HEST_NOTIFY_POLLED = 0,
> + ACPI_HEST_NOTIFY_EXTERNAL = 1,
> + ACPI_HEST_NOTIFY_LOCAL = 2,
> + ACPI_HEST_NOTIFY_SCI = 3,
> + ACPI_HEST_NOTIFY_NMI = 4,
> + ACPI_HEST_NOTIFY_CMCI = 5, /* ACPI 5.0 */
> + ACPI_HEST_NOTIFY_MCE = 6, /* ACPI 5.0 */
> + ACPI_HEST_NOTIFY_GPIO = 7, /* ACPI 6.0 */
> + ACPI_HEST_NOTIFY_SEA = 8, /* ACPI 6.1 */
> + ACPI_HEST_NOTIFY_SEI = 9, /* ACPI 6.1 */
> + ACPI_HEST_NOTIFY_GSIV = 10, /* ACPI 6.1 */
> + ACPI_HEST_NOTIFY_RESERVED = 11 /* 11 and greater are reserved */
> +};
> +
these are never reused, it's best
> /*
> * MADT sub-structures (Follow MULTIPLE_APIC_DESCRIPTION_TABLE)
> */
> @@ -475,6 +527,181 @@ struct AcpiSystemResourceAffinityTable
> } QEMU_PACKED;
> typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
>
> +#define ACPI_ADR_SPACE_SYSTEM_MEMORY (uint8_t) 0
> +#define ACPI_ADR_SPACE_SYSTEM_IO (uint8_t) 1
> +#define ACPI_ADR_SPACE_PCI_CONFIG (uint8_t) 2
> +#define ACPI_ADR_SPACE_EC (uint8_t) 3
> +#define ACPI_ADR_SPACE_SMBUS (uint8_t) 4
> +#define ACPI_ADR_SPACE_CMOS (uint8_t) 5
> +#define ACPI_ADR_SPACE_PCI_BAR_TARGET (uint8_t) 6
> +#define ACPI_ADR_SPACE_IPMI (uint8_t) 7
> +#define ACPI_ADR_SPACE_GPIO (uint8_t) 8
> +#define ACPI_ADR_SPACE_GSBUS (uint8_t) 9
> +#define ACPI_ADR_SPACE_PLATFORM_COMM (uint8_t) 10
> +
We already have macros for some of these and APIs to
format it.
> +/* GAS - Generic Address Structure */
> +struct acpi_generic_address {
> + uint8_t space_id; /* Address space where
> + *struct or register exists
> + */
> + uint8_t bit_width; /* Size in bits of given register */
> + uint8_t bit_offset; /* Bit offset within the register */
> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */
> + uint64_t address; /* 64-bit address of struct or register */
> +} __attribute__ ((packed));
> +
> +/* Hardware Error Notification */
> +struct acpi_hest_notify {
> + uint8_t type;
> + uint8_t length;
> + uint16_t config_write_enable;
> + uint32_t poll_interval;
> + uint32_t vector;
> + uint32_t polling_threshold_value;
> + uint32_t polling_threshold_window;
> + uint32_t error_threshold_value;
> + uint32_t error_threshold_window;
> +};
> +
> +enum acpi_hest_types {
> + ACPI_HEST_TYPE_IA32_CHECK = 0,
> + ACPI_HEST_TYPE_IA32_CORRECTED_CHECK = 1,
> + ACPI_HEST_TYPE_IA32_NMI = 2,
> + ACPI_HEST_TYPE_NOT_USED3 = 3,
> + ACPI_HEST_TYPE_NOT_USED4 = 4,
> + ACPI_HEST_TYPE_NOT_USED5 = 5,
> + ACPI_HEST_TYPE_AER_ROOT_PORT = 6,
> + ACPI_HEST_TYPE_AER_ENDPOINT = 7,
> + ACPI_HEST_TYPE_AER_BRIDGE = 8,
> + ACPI_HEST_TYPE_GENERIC_ERROR = 9,
> + ACPI_HEST_TYPE_GENERIC_ERROR_V2 = 10,
> + ACPI_HEST_TYPE_RESERVED = 11 /* 11 and greater are reserved */
> +};
> +
> +/* Values for block_status flags above */
> +#define ACPI_BERT_UNCORRECTABLE (1)
> +#define ACPI_BERT_CORRECTABLE (1 << 1)
> +#define ACPI_BERT_MULTIPLE_UNCORRECTABLE (1 << 2)
> +#define ACPI_BERT_MULTIPLE_CORRECTABLE (1 << 3)
> +/* 8 bits, error count */
> +#define ACPI_BERT_ERROR_ENTRY_COUNT (0xFF << 4)
> +
> +/* Generic Hardware Error Source Structure */
> +struct AcpiGenericHardwareErrorSource {
> + uint16_t type;
> + uint16_t source_id;
> + uint16_t related_source_id;
> + uint8_t flags;
> + uint8_t enabled;
> + uint32_t number_of_records;
> + uint32_t max_sections_per_record;
> + uint32_t max_raw_data_length;
> + struct acpi_generic_address error_status_address;
> + struct acpi_hest_notify notify;
> + uint32_t error_status_block_length;
> +} QEMU_PACKED;
> +typedef struct AcpiGenericHardwareErrorSource AcpiGenericHardwareErrorSource;
> +
> +/* Generic Hardware Error Source , version 2 */
> +struct AcpiGenericHardwareErrorSourceV2 {
> + uint16_t type;
> + uint16_t source_id;
> + uint16_t related_source_id;
> + uint8_t flags;
> + uint8_t enabled;
> + uint32_t number_of_records;
> + uint32_t max_sections_per_record;
> + uint32_t max_raw_data_length;
> + struct acpi_generic_address error_status_address;
> + struct acpi_hest_notify notify;
> + uint32_t error_status_block_length;
> + struct acpi_generic_address read_ack_register;
> + uint64_t read_ack_preserve;
> + uint64_t read_ack_write;
> +} QEMU_PACKED;
> +typedef struct AcpiGenericHardwareErrorSourceV2
> + AcpiGenericHardwareErrorSourceV2;
> +
> +/* Generic Error Status block */
> +
> +struct AcpiGenericErrorStatus {
> + uint32_t block_status;
> + uint32_t raw_data_offset;
> + uint32_t raw_data_length;
> + uint32_t data_length;
> + uint32_t error_severity;
> +};
> +typedef struct AcpiGenericErrorStatus AcpiGenericErrorStatus;
> +
> +/* Generic Error Data entry */
> +
> +struct AcpiGenericErrorData {
> + uint8_t section_type[16];
> + uint32_t error_severity;
> + uint16_t revision;
> + uint8_t validation_bits;
> + uint8_t flags;
> + uint32_t error_data_length;
> + uint8_t fru_id[16];
> + uint8_t fru_text[20];
> +};
> +typedef struct AcpiGenericErrorData AcpiGenericErrorData;
> +
> +/* Extension for revision 0x0300 */
> +struct AcpiGenericErrorDataV300 {
> + uint8_t section_type[16];
> + uint32_t error_severity;
> + uint16_t revision;
> + uint8_t validation_bits;
> + uint8_t flags;
> + uint32_t error_data_length;
> + uint8_t fru_id[16];
> + uint8_t fru_text[20];
> + uint64_t time_stamp;
> +};
> +typedef struct AcpiGenericErrorDataV300 AcpiGenericErrorDataV300;
> +
> +enum {
> + CPER_SEV_RECOVERABLE,
> + CPER_SEV_FATAL,
> + CPER_SEV_CORRECTED,
> + CPER_SEV_INFORMATIONAL,
> +};
> +
> +/* Memory Error Section */
> +struct cper_sec_mem_err {
> + uint64_t validation_bits;
> + uint64_t error_status;
> + uint64_t physical_addr;
> + uint64_t physical_addr_mask;
> + uint16_t node;
> + uint16_t card;
> + uint16_t module;
> + uint16_t bank;
> + uint16_t device;
> + uint16_t row;
> + uint16_t column;
> + uint16_t bit_pos;
> + uint64_t requestor_id;
> + uint64_t responder_id;
> + uint64_t target_id;
> + uint8_t error_type;
> + uint8_t reserved;
> + uint16_t rank;
> + uint16_t mem_array_handle; /* card handle in UEFI 2.4 */
> + uint16_t mem_dev_handle; /* module handle in UEFI 2.4 */
> +};
> + typedef struct cper_sec_mem_err cper_sec_mem_err;
> +
> +/*
> + * HEST Description Table
> + */
> +struct AcpiHardwareErrorSourceTable {
> + ACPI_TABLE_HEADER_DEF /* ACPI common table header */
> + uint32_t error_source_count;
> +} QEMU_PACKED;
> +typedef struct AcpiHardwareErrorSourceTable AcpiHardwareErrorSourceTable;
> +
> #define ACPI_SRAT_PROCESSOR_APIC 0
> #define ACPI_SRAT_MEMORY 1
> #define ACPI_SRAT_PROCESSOR_x2APIC 2
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 00c21f1..c1d15b3 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
> GArray *rsdp;
> GArray *tcpalog;
> GArray *vmgenid;
> + GArray *hardware_errors;
> BIOSLinker *linker;
> } AcpiBuildTables;
>
> diff --git a/include/hw/acpi/hest_ghes.h b/include/hw/acpi/hest_ghes.h
> new file mode 100644
> index 0000000..0cadc2b
> --- /dev/null
> +++ b/include/hw/acpi/hest_ghes.h
> @@ -0,0 +1,43 @@
> +#ifndef ACPI_GHES_H
> +#define ACPI_GHES_H
> +
> +#include "hw/acpi/bios-linker-loader.h"
> +
> +#define GHES_ERRORS_FW_CFG_FILE "etc/hardware_errors"
> +#define GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
> +
> +#define GAS_ADDRESS_OFFSET 4
> +#define ERROR_STATUS_ADDRESS_OFFSET 20
> +#define NOTIFICATION_STRUCTURE 32
> +
> +#define BFAPEI_OK 0
> +#define BFAPEI_FAIL 1
> +
> +/* The max number of error source, the error sources
> + * are classified by notification type, below is the definition
> + * 0 - Polled
> + * 1 - External Interrupt
> + * 2 - Local Interrupt
> + * 3 - SCI
> + * 4 - NMI
> + * 5 - CMCI
> + * 6 - MCE
> + * 7 - GPIO-Signal
> + * 8 - ARMv8 SEA
> + * 9 - ARMv8 SEI
> + * 10 - External Interrupt - GSIV
> + */
> +#define MAX_ERROR_SOURCE_COUNT_V6 11
> +/* The max size in Bytes for one error block */
> +#define MAX_RAW_DATA_LENGTH 0x1000
> +
> +typedef struct GhesErrorState {
> + uint64_t physical_addr;
> + uint64_t ghes_addr_le[8];
> +} GhesErrorState;
> +
> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
> + BIOSLinker *linker);
> +void ghes_add_fw_cfg(FWCfgState *s, GArray *guid);
> +void ghes_update_guest(uint32_t notify, uint64_t physical_address);
> +#endif
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-05-12 23:59 ` Michael S. Tsirkin
@ 2017-05-20 5:47 ` gengdongjiu
0 siblings, 0 replies; 17+ messages in thread
From: gengdongjiu @ 2017-05-20 5:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: james.morse, marc.zyngier, christoffer.dall, rkrcmar, linux,
tbaicar, imammedo, zhaoshenglong, peter.maydell, pbonzini,
qemu-devel, qemu-arm, lersek, ard.biesheuvel, drjones, ben, kvm,
linux-arm-kernel, kvmarm, xiexiuqi, wangxiongfeng2, songwenjun,
wuquanming, huangshaoyu
Michael,
very sorry for the late response due to recently busy.
On 2017/5/13 7:59, Michael S. Tsirkin wrote:
> On Sun, Apr 30, 2017 at 01:35:03PM +0800, Dongjiu Geng wrote:
>> This implements APEI GHES Table by passing the error cper info
>> to the guest via a fw_cfg_blob. After a CPER info is added, an
>> SEA/SEI exception will be injected into the guest OS.
>>
>> Below is the table layout, the max number of error soure is 11,
>> which is classified by notification type.
>>
>> etc/acpi/tables etc/hardware_errors
>> ================ ==========================================
>> +-----------+
>> +--------------+ | address | +-> +--------------+
>> | HEST + | registers | | | Error Status |
>> + +------------+ | +---------+ | | Data Block 1 |
>> | | GHES1 | --> | |address1 | --------+ | +------------+
>> | | GHES2 | --> | |address2 | ------+ | | CPER |
>> | | GHES3 | --> | |address3 | ----+ | | | CPER |
>> | | .... | --> | | ....... | | | | | CPER |
>> | | GHES10 | --> | |address10| -+ | | | | CPER |
>> +-+------------+ +-+---------+ | | | +-+------------+
>> | | |
>> | | +---> +--------------+
>> | | | Error Status |
>> | | | Data Block 2 |
>> | | | +------------+
>> | | | | CPER |
>> | | | | CPER |
>> | | +-+------------+
>> | |
>> | +-----> +--------------+
>> | | Error Status |
>> | | Data Block 3 |
>> | | +------------+
>> | | | CPER |
>> | +-+------------+
>> | ...........
>> +--------> +--------------+
>> | Error Status |
>> | Data Block 10|
>> | +------------+
>> | | CPER |
>> | | CPER |
>> | | CPER |
>> +-+------------+
>>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>> ---
>> default-configs/arm-softmmu.mak | 1 +
>> hw/acpi/Makefile.objs | 1 +
>> hw/acpi/aml-build.c | 2 +
>> hw/acpi/hest_ghes.c | 203 +++++++++++++++++++++++++++++++++++
>> hw/arm/virt-acpi-build.c | 6 ++
>> include/hw/acpi/acpi-defs.h | 227 ++++++++++++++++++++++++++++++++++++++++
>> include/hw/acpi/aml-build.h | 1 +
>> include/hw/acpi/hest_ghes.h | 43 ++++++++
>> 8 files changed, 484 insertions(+)
>> create mode 100644 hw/acpi/hest_ghes.c
>> create mode 100644 include/hw/acpi/hest_ghes.h
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 1e3bd2b..d5f1552 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -121,3 +121,4 @@ CONFIG_ACPI=y
>> CONFIG_SMBIOS=y
>> CONFIG_ASPEED_SOC=y
>> CONFIG_GPIO_KEY=y
>> +CONFIG_ACPI_APEI_GENERATION=y
>> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
>> index 11c35bc..776b46e 100644
>> --- a/hw/acpi/Makefile.objs
>> +++ b/hw/acpi/Makefile.objs
>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
>> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>> common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
>> +common-obj-$(CONFIG_ACPI_APEI_GENERATION) += hest_ghes.o
>> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>>
>> common-obj-y += acpi_interface.o
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index c6f2032..802b98d 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1560,6 +1560,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();
>> }
>>
>> @@ -1570,6 +1571,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..91d382e
>> --- /dev/null
>> +++ b/hw/acpi/hest_ghes.c
>> @@ -0,0 +1,203 @@
>> +/*
>> + * APEI GHES table Generation
>> + *
>> + * Copyright (C) 2017 huawei.
>> + *
>> + * Author: Dongjiu Geng <gengdongjiu@huawei.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qmp-commands.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"
>> +
>> +static int ghes_generate_cper_record(uint64_t block_error_address,
>> + uint64_t error_physical_addr)
>> +{
>> + AcpiGenericErrorStatus block;
>> + AcpiGenericErrorData *gdata;
>> + struct cper_sec_mem_err *mem_err;
>> + uint64_t block_data_length;
>> + unsigned char *buffer;
>> +
>> + cpu_physical_memory_read(block_error_address, &block,
>> + sizeof(AcpiGenericErrorStatus));
>> +
>> + block_data_length = sizeof(AcpiGenericErrorStatus) + block.data_length;
>> +
>> + /* If the Generic Error Status Block is NULL, update
>> + * the block header
>> + */
>> + if (!block.block_status) {
>> + block.block_status = ACPI_BERT_UNCORRECTABLE;
>> + block.error_severity = CPER_SEV_FATAL;
>> + }
>> +
>> + block.data_length += sizeof(AcpiGenericErrorData);
>> + block.data_length += sizeof(struct cper_sec_mem_err);
>> +
>> + /* Write back the Generic Error Status Block to guest memory */
>> + cpu_physical_memory_write(block_error_address, &block,
>> + sizeof(AcpiGenericErrorStatus));
>> +
>> + /* Fill in Generic Error Data Entry */
>> + buffer = g_malloc(sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
>> + memset(buffer, 0, sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
>> + gdata = (AcpiGenericErrorData *)buffer;
>> +
>> + memcpy(gdata->section_type, (void *) &CPER_SEC_PLATFORM_MEM,
>> + sizeof(uuid_le));
>> + gdata->error_data_length = sizeof(struct cper_sec_mem_err);
>> +
>> + mem_err = (struct cper_sec_mem_err *) (gdata + 1);
>> +
>> + /* In order to simplify simulation, hardcode the CPER section to memory
>> + * section.
>> + */
>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
>> + mem_err->error_type = 3;
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_PA;
>> + mem_err->physical_addr = error_physical_addr;
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
>> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
>> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
>> + mem_err->card = 1;
>> + mem_err->module = 2;
>> + mem_err->bank = 3;
>> + mem_err->row = 1;
>> + mem_err->column = 2;
>> + mem_err->bit_pos = 5;
>> +
>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_STATUS;
>> + mem_err->error_status = 4 << 8;
>> +
>> + /* Write back the Generic Error Data Entry to guest memory */
>> + cpu_physical_memory_write(block_error_address + block_data_length, buffer,
>> + sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
>> +
>> + g_free(buffer);
>> + return BFAPEI_OK;
>> +}
>> +
>> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
>> + BIOSLinker *linker)
>> +{
>> + Aml *hest;
>> + uint32_t address_registers_offset;
>> + AcpiTableHeader *header;
>> + AcpiGenericHardwareErrorSource *error_source;
>> + int i;
>> +
>> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
>> +
>> + /* New address register and Error status block table size*/
>> + g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6
>> + * block_reqr_size);
>> +
>> + /* Put this in a HEST table */
>> + hest = init_aml_allocator();
>> + address_registers_offset = table_data->len
>> + + sizeof(AcpiHardwareErrorSourceTable)
>> + + ERROR_STATUS_ADDRESS_OFFSET
>> + + GAS_ADDRESS_OFFSET;
>> + /* Reserve space for HEST table size*/
>> + acpi_data_push(hest->buf, sizeof(AcpiHardwareErrorSourceTable)
>> + + MAX_ERROR_SOURCE_COUNT_V6
>> + * sizeof(AcpiGenericHardwareErrorSource));
>> +
>> + g_array_append_vals(table_data, hest->buf->data, hest->buf->len);
>> + /* Allocate guest memory for the Data fw_cfg blob */
>> + bios_linker_loader_alloc(linker, GHES_ERRORS_FW_CFG_FILE,
>> + hardware_error, 4096,
>> + false /* page boundary, high memory */);
>> + header = (AcpiTableHeader *)(table_data->data
>> + + table_data->len - hest->buf->len);
>> + *(uint32_t *)(header + 1) = MAX_ERROR_SOURCE_COUNT_V6;
>> + error_source = (AcpiGenericHardwareErrorSource *)((char *)header
>> + + sizeof(AcpiHardwareErrorSourceTable));
>> +
>> + for (i = 0; i < MAX_ERROR_SOURCE_COUNT_V6; i++) {
>> + error_source->type = ACPI_HEST_TYPE_GENERIC_ERROR;
>> + error_source->source_id = 0;
>> + error_source->related_source_id = 0xffff;
>> + error_source->flags = 0;
>> + error_source->enabled = 1;
>> + error_source->number_of_records = 1;
>> + error_source->max_sections_per_record = 1;
>> + error_source->max_raw_data_length = MAX_RAW_DATA_LENGTH;
>> + error_source->error_status_address.space_id =
>> + ACPI_ADR_SPACE_SYSTEM_MEMORY;
>> + error_source->error_status_address.bit_width = 64;
>> + error_source->error_status_address.bit_offset = 0;
>> + error_source->error_status_address.access_width = 4;
>> + error_source->notify.type = i;
>> + error_source->notify.length = sizeof(AcpiGenericHardwareErrorSource);
>> +
>> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>> + sizeof(uint64_t) * i, sizeof(uint64_t),
>> + GHES_ERRORS_FW_CFG_FILE,
>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>> + i * MAX_RAW_DATA_LENGTH);
>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> + address_registers_offset
>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>> + i * sizeof(uint64_t));
>> +
>> + bios_linker_loader_write_pointer(linker, GHES_DATA_ADDR_FW_CFG_FILE,
>> + i * sizeof(uint64_t), sizeof(uint64_t),
>> + GHES_ERRORS_FW_CFG_FILE,
>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>> + i * MAX_RAW_DATA_LENGTH);
>> + error_source++;
>> + }
>> +
>> + build_header(linker, table_data,
>> + (void *)header, "HEST", hest->buf->len, 1, NULL, "GHES");
>> +
>> + free_aml_allocator();
>> +}
>> +
>> +static GhesErrorState ges;
>> +void ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_error)
>> +{
>> +
>> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
>> + int size = MAX_ERROR_SOURCE_COUNT_V6 * block_reqr_size;
>> +
>> + /* Create a read-only fw_cfg file for GHES */
>> + fw_cfg_add_file(s, GHES_ERRORS_FW_CFG_FILE, hardware_error->data,
>> + size);
>> + /* Create a read-write fw_cfg file for Address */
>> + fw_cfg_add_file_callback(s, GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL,
>> + &(ges.ghes_addr_le[0]),
>> + sizeof(uint64_t) * MAX_ERROR_SOURCE_COUNT_V6,
>> + false);
>> +}
>> +
>> +void ghes_update_guest(uint32_t notify, uint64_t physical_address)
>> +{
>> + uint64_t block_error_addr;
>> +
>> + if (physical_address) {
>> + ges.physical_addr = physical_address;
>> + block_error_addr = ges.ghes_addr_le[notify];
>> + block_error_addr = le32_to_cpu(block_error_addr);
>> +
>> + /* A zero value in ghes_addr means that BIOS has not yet written
>> + * the address
>> + */
>> + if (block_error_addr) {
>> + ghes_generate_cper_record(block_error_addr, physical_address);
>> + }
>> + }
>> +}
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 0835e59..e7ab5dc 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -45,6 +45,8 @@
>> #include "hw/arm/virt.h"
>> #include "sysemu/numa.h"
>> #include "kvm_arm.h"
>> +#include "hw/acpi/vmgenid.h"
>> +#include "hw/acpi/hest_ghes.h"
>>
>> #define ARM_SPI_BASE 32
>> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>> @@ -778,6 +780,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>> acpi_add_table(table_offsets, tables_blob);
>> build_spcr(tables_blob, tables->linker, vms);
>>
>> + acpi_add_table(table_offsets, tables_blob);
>> + ghes_build_acpi(tables_blob, tables->hardware_errors, tables->linker);
>> +
>> if (nb_numa_nodes > 0) {
>> acpi_add_table(table_offsets, tables_blob);
>> build_srat(tables_blob, tables->linker, vms);
>> @@ -892,6 +897,7 @@ void virt_acpi_setup(VirtMachineState *vms)
>>
>> build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
>> ACPI_BUILD_RSDP_FILE, 0);
>> + ghes_add_fw_cfg(vms->fw_cfg, tables.hardware_errors);
>>
>> qemu_register_reset(virt_acpi_build_reset, build_state);
>> virt_acpi_build_reset(build_state);
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index 4cc3630..27adede 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -295,6 +295,58 @@ typedef struct AcpiMultipleApicTable AcpiMultipleApicTable;
>> #define ACPI_APIC_GENERIC_TRANSLATOR 15
>> #define ACPI_APIC_RESERVED 16 /* 16 and greater are reserved */
>>
>> +#define CPER_MEM_VALID_ERROR_STATUS 0x0001
>> +#define CPER_MEM_VALID_PA 0x0002
>> +#define CPER_MEM_VALID_PA_MASK 0x0004
>> +#define CPER_MEM_VALID_NODE 0x0008
>> +#define CPER_MEM_VALID_CARD 0x0010
>> +#define CPER_MEM_VALID_MODULE 0x0020
>> +#define CPER_MEM_VALID_BANK 0x0040
>> +#define CPER_MEM_VALID_DEVICE 0x0080
>> +#define CPER_MEM_VALID_ROW 0x0100
>> +#define CPER_MEM_VALID_COLUMN 0x0200
>> +#define CPER_MEM_VALID_BIT_POSITION 0x0400
>> +#define CPER_MEM_VALID_REQUESTOR_ID 0x0800
>> +#define CPER_MEM_VALID_RESPONDER_ID 0x1000
>> +#define CPER_MEM_VALID_TARGET_ID 0x2000
>> +#define CPER_MEM_VALID_ERROR_TYPE 0x4000
>> +#define CPER_MEM_VALID_RANK_NUMBER 0x8000
>> +#define CPER_MEM_VALID_CARD_HANDLE 0x10000
>> +#define CPER_MEM_VALID_MODULE_HANDLE 0x20000
>> +
>> +typedef struct {
>> + uint8_t b[16];
>> +} uuid_le;
>> +
>> +#define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
>> +((uuid_le) \
>> +{{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
>> + (b) & 0xff, ((b) >> 8) & 0xff, \
>> + (c) & 0xff, ((c) >> 8) & 0xff, \
>> + (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) } })
>> +
>> +/* Platform Memory */
>> +#define CPER_SEC_PLATFORM_MEM \
>> + UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \
>> + 0xED, 0x7C, 0x83, 0xB1)
>> +
>
> There's no reason to define these messy one-time use macros.
> They just make it hard to look things up in the spec.
will remove these definition.
>
>
> You can use build_append_int_noprefix to add data of
> any length in LE format.
good suggestion.
>
>
>> +/* Values for Notify Type field above */
>> +
>> +enum acpi_hest_notify_types {
>> + ACPI_HEST_NOTIFY_POLLED = 0,
>> + ACPI_HEST_NOTIFY_EXTERNAL = 1,
>> + ACPI_HEST_NOTIFY_LOCAL = 2,
>> + ACPI_HEST_NOTIFY_SCI = 3,
>> + ACPI_HEST_NOTIFY_NMI = 4,
>> + ACPI_HEST_NOTIFY_CMCI = 5, /* ACPI 5.0 */
>> + ACPI_HEST_NOTIFY_MCE = 6, /* ACPI 5.0 */
>> + ACPI_HEST_NOTIFY_GPIO = 7, /* ACPI 6.0 */
>> + ACPI_HEST_NOTIFY_SEA = 8, /* ACPI 6.1 */
>> + ACPI_HEST_NOTIFY_SEI = 9, /* ACPI 6.1 */
>> + ACPI_HEST_NOTIFY_GSIV = 10, /* ACPI 6.1 */
>> + ACPI_HEST_NOTIFY_RESERVED = 11 /* 11 and greater are reserved */
>> +};
>> +
>
> these are never reused, it's best
>
>> /*
>> * MADT sub-structures (Follow MULTIPLE_APIC_DESCRIPTION_TABLE)
>> */
>> @@ -475,6 +527,181 @@ struct AcpiSystemResourceAffinityTable
>> } QEMU_PACKED;
>> typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
>>
>> +#define ACPI_ADR_SPACE_SYSTEM_MEMORY (uint8_t) 0
>> +#define ACPI_ADR_SPACE_SYSTEM_IO (uint8_t) 1
>> +#define ACPI_ADR_SPACE_PCI_CONFIG (uint8_t) 2
>> +#define ACPI_ADR_SPACE_EC (uint8_t) 3
>> +#define ACPI_ADR_SPACE_SMBUS (uint8_t) 4
>> +#define ACPI_ADR_SPACE_CMOS (uint8_t) 5
>> +#define ACPI_ADR_SPACE_PCI_BAR_TARGET (uint8_t) 6
>> +#define ACPI_ADR_SPACE_IPMI (uint8_t) 7
>> +#define ACPI_ADR_SPACE_GPIO (uint8_t) 8
>> +#define ACPI_ADR_SPACE_GSBUS (uint8_t) 9
>> +#define ACPI_ADR_SPACE_PLATFORM_COMM (uint8_t) 10
>> +
>
> We already have macros for some of these and APIs to
> format it.
in the qemu code "include/hw/acpi/aml-build.h".
I see there are some definition, but it is incomplete, anyway I will remove these macros.
typedef enum {
AML_SYSTEM_MEMORY = 0X00,
AML_SYSTEM_IO = 0X01,
AML_PCI_CONFIG = 0X02,
} AmlRegionSpace;
>
>> +/* GAS - Generic Address Structure */
>> +struct acpi_generic_address {
>> + uint8_t space_id; /* Address space where
>> + *struct or register exists
>> + */
>> + uint8_t bit_width; /* Size in bits of given register */
>> + uint8_t bit_offset; /* Bit offset within the register */
>> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */
>> + uint64_t address; /* 64-bit address of struct or register */
>> +} __attribute__ ((packed));
>> +
>> +/* Hardware Error Notification */
>> +struct acpi_hest_notify {
>> + uint8_t type;
>> + uint8_t length;
>> + uint16_t config_write_enable;
>> + uint32_t poll_interval;
>> + uint32_t vector;
>> + uint32_t polling_threshold_value;
>> + uint32_t polling_threshold_window;
>> + uint32_t error_threshold_value;
>> + uint32_t error_threshold_window;
>> +};
>> +
>> +enum acpi_hest_types {
>> + ACPI_HEST_TYPE_IA32_CHECK = 0,
>> + ACPI_HEST_TYPE_IA32_CORRECTED_CHECK = 1,
>> + ACPI_HEST_TYPE_IA32_NMI = 2,
>> + ACPI_HEST_TYPE_NOT_USED3 = 3,
>> + ACPI_HEST_TYPE_NOT_USED4 = 4,
>> + ACPI_HEST_TYPE_NOT_USED5 = 5,
>> + ACPI_HEST_TYPE_AER_ROOT_PORT = 6,
>> + ACPI_HEST_TYPE_AER_ENDPOINT = 7,
>> + ACPI_HEST_TYPE_AER_BRIDGE = 8,
>> + ACPI_HEST_TYPE_GENERIC_ERROR = 9,
>> + ACPI_HEST_TYPE_GENERIC_ERROR_V2 = 10,
>> + ACPI_HEST_TYPE_RESERVED = 11 /* 11 and greater are reserved */
>> +};
>> +
>> +/* Values for block_status flags above */
>> +#define ACPI_BERT_UNCORRECTABLE (1)
>> +#define ACPI_BERT_CORRECTABLE (1 << 1)
>> +#define ACPI_BERT_MULTIPLE_UNCORRECTABLE (1 << 2)
>> +#define ACPI_BERT_MULTIPLE_CORRECTABLE (1 << 3)
>> +/* 8 bits, error count */
>> +#define ACPI_BERT_ERROR_ENTRY_COUNT (0xFF << 4)
>> +
>> +/* Generic Hardware Error Source Structure */
>> +struct AcpiGenericHardwareErrorSource {
>> + uint16_t type;
>> + uint16_t source_id;
>> + uint16_t related_source_id;
>> + uint8_t flags;
>> + uint8_t enabled;
>> + uint32_t number_of_records;
>> + uint32_t max_sections_per_record;
>> + uint32_t max_raw_data_length;
>> + struct acpi_generic_address error_status_address;
>> + struct acpi_hest_notify notify;
>> + uint32_t error_status_block_length;
>> +} QEMU_PACKED;
>> +typedef struct AcpiGenericHardwareErrorSource AcpiGenericHardwareErrorSource;
>> +
>> +/* Generic Hardware Error Source , version 2 */
>> +struct AcpiGenericHardwareErrorSourceV2 {
>> + uint16_t type;
>> + uint16_t source_id;
>> + uint16_t related_source_id;
>> + uint8_t flags;
>> + uint8_t enabled;
>> + uint32_t number_of_records;
>> + uint32_t max_sections_per_record;
>> + uint32_t max_raw_data_length;
>> + struct acpi_generic_address error_status_address;
>> + struct acpi_hest_notify notify;
>> + uint32_t error_status_block_length;
>> + struct acpi_generic_address read_ack_register;
>> + uint64_t read_ack_preserve;
>> + uint64_t read_ack_write;
>> +} QEMU_PACKED;
>> +typedef struct AcpiGenericHardwareErrorSourceV2
>> + AcpiGenericHardwareErrorSourceV2;
>> +
>> +/* Generic Error Status block */
>> +
>> +struct AcpiGenericErrorStatus {
>> + uint32_t block_status;
>> + uint32_t raw_data_offset;
>> + uint32_t raw_data_length;
>> + uint32_t data_length;
>> + uint32_t error_severity;
>> +};
>> +typedef struct AcpiGenericErrorStatus AcpiGenericErrorStatus;
>> +
>> +/* Generic Error Data entry */
>> +
>> +struct AcpiGenericErrorData {
>> + uint8_t section_type[16];
>> + uint32_t error_severity;
>> + uint16_t revision;
>> + uint8_t validation_bits;
>> + uint8_t flags;
>> + uint32_t error_data_length;
>> + uint8_t fru_id[16];
>> + uint8_t fru_text[20];
>> +};
>> +typedef struct AcpiGenericErrorData AcpiGenericErrorData;
>> +
>> +/* Extension for revision 0x0300 */
>> +struct AcpiGenericErrorDataV300 {
>> + uint8_t section_type[16];
>> + uint32_t error_severity;
>> + uint16_t revision;
>> + uint8_t validation_bits;
>> + uint8_t flags;
>> + uint32_t error_data_length;
>> + uint8_t fru_id[16];
>> + uint8_t fru_text[20];
>> + uint64_t time_stamp;
>> +};
>> +typedef struct AcpiGenericErrorDataV300 AcpiGenericErrorDataV300;
>> +
>> +enum {
>> + CPER_SEV_RECOVERABLE,
>> + CPER_SEV_FATAL,
>> + CPER_SEV_CORRECTED,
>> + CPER_SEV_INFORMATIONAL,
>> +};
>> +
>> +/* Memory Error Section */
>> +struct cper_sec_mem_err {
>> + uint64_t validation_bits;
>> + uint64_t error_status;
>> + uint64_t physical_addr;
>> + uint64_t physical_addr_mask;
>> + uint16_t node;
>> + uint16_t card;
>> + uint16_t module;
>> + uint16_t bank;
>> + uint16_t device;
>> + uint16_t row;
>> + uint16_t column;
>> + uint16_t bit_pos;
>> + uint64_t requestor_id;
>> + uint64_t responder_id;
>> + uint64_t target_id;
>> + uint8_t error_type;
>> + uint8_t reserved;
>> + uint16_t rank;
>> + uint16_t mem_array_handle; /* card handle in UEFI 2.4 */
>> + uint16_t mem_dev_handle; /* module handle in UEFI 2.4 */
>> +};
>> + typedef struct cper_sec_mem_err cper_sec_mem_err;
>> +
>> +/*
>> + * HEST Description Table
>> + */
>> +struct AcpiHardwareErrorSourceTable {
>> + ACPI_TABLE_HEADER_DEF /* ACPI common table header */
>> + uint32_t error_source_count;
>> +} QEMU_PACKED;
>> +typedef struct AcpiHardwareErrorSourceTable AcpiHardwareErrorSourceTable;
>> +
>> #define ACPI_SRAT_PROCESSOR_APIC 0
>> #define ACPI_SRAT_MEMORY 1
>> #define ACPI_SRAT_PROCESSOR_x2APIC 2
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 00c21f1..c1d15b3 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
>> GArray *rsdp;
>> GArray *tcpalog;
>> GArray *vmgenid;
>> + GArray *hardware_errors;
>> BIOSLinker *linker;
>> } AcpiBuildTables;
>>
>> diff --git a/include/hw/acpi/hest_ghes.h b/include/hw/acpi/hest_ghes.h
>> new file mode 100644
>> index 0000000..0cadc2b
>> --- /dev/null
>> +++ b/include/hw/acpi/hest_ghes.h
>> @@ -0,0 +1,43 @@
>> +#ifndef ACPI_GHES_H
>> +#define ACPI_GHES_H
>> +
>> +#include "hw/acpi/bios-linker-loader.h"
>> +
>> +#define GHES_ERRORS_FW_CFG_FILE "etc/hardware_errors"
>> +#define GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
>> +
>> +#define GAS_ADDRESS_OFFSET 4
>> +#define ERROR_STATUS_ADDRESS_OFFSET 20
>> +#define NOTIFICATION_STRUCTURE 32
>> +
>> +#define BFAPEI_OK 0
>> +#define BFAPEI_FAIL 1
>> +
>> +/* The max number of error source, the error sources
>> + * are classified by notification type, below is the definition
>> + * 0 - Polled
>> + * 1 - External Interrupt
>> + * 2 - Local Interrupt
>> + * 3 - SCI
>> + * 4 - NMI
>> + * 5 - CMCI
>> + * 6 - MCE
>> + * 7 - GPIO-Signal
>> + * 8 - ARMv8 SEA
>> + * 9 - ARMv8 SEI
>> + * 10 - External Interrupt - GSIV
>> + */
>> +#define MAX_ERROR_SOURCE_COUNT_V6 11
>> +/* The max size in Bytes for one error block */
>> +#define MAX_RAW_DATA_LENGTH 0x1000
>> +
>> +typedef struct GhesErrorState {
>> + uint64_t physical_addr;
>> + uint64_t ghes_addr_le[8];
>> +} GhesErrorState;
>> +
>> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
>> + BIOSLinker *linker);
>> +void ghes_add_fw_cfg(FWCfgState *s, GArray *guid);
>> +void ghes_update_guest(uint32_t notify, uint64_t physical_address);
>> +#endif
>> --
>> 1.8.3.1
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
@ 2017-05-29 15:27 gengdongjiu
2017-05-29 16:03 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: gengdongjiu @ 2017-05-29 15:27 UTC (permalink / raw)
To: ard.biesheuvel@linaro.org, edk2-devel@lists.01.org,
qemu-devel@nongnu.org, Zhaoshenglong, James Morse,
Christoffer Dall, Xiexiuqi, Marc Zyngier, catalin.marinas@arm.com,
will.deacon@arm.com, christoffer.dall@linaro.org,
rkrcmar@redhat.com, suzuki.poulose@arm.com,
andre.przywara@arm.com, mark.rutland@arm.com,
vladimir.murzin@arm.com, linux-arm-kernel@lists.infradead.org,
kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, wangxiongfeng (C), Wuquanming,
Huangshaoyu, Leif.Lindholm@linaro.com
Dear Laszlo,
Thank your very much for your review and detailed comment. and very sorry for the late response due to recently debug the wholes RAS solution.
On 2017/5/22 22:23, Laszlo Ersek wrote:
> Keeping some context:
>
> On 05/12/17 23:00, Laszlo Ersek wrote:
>> On 04/30/17 07:35, Dongjiu Geng wrote:
>>> This implements APEI GHES Table by passing the error cper info to
>>> the guest via a fw_cfg_blob. After a CPER info is added, an SEA/SEI
>>> exception will be injected into the guest OS.
>>>
>>> Below is the table layout, the max number of error soure is 11,
>>> which is classified by notification type.
>>>
>>> etc/acpi/tables etc/hardware_errors
>>> ================ ==========================================
>>> +-----------+
>>> +--------------+ | address | +-> +--------------+
>>> | HEST + | registers | | | Error Status |
>>> + +------------+ | +---------+ | | Data Block 1 |
>>> | | GHES1 | --> | |address1 | --------+ | +------------+
>>> | | GHES2 | --> | |address2 | ------+ | | CPER |
>>> | | GHES3 | --> | |address3 | ----+ | | | CPER |
>>> | | .... | --> | | ....... | | | | | CPER |
>>> | | GHES10 | --> | |address10| -+ | | | | CPER |
>>> +-+------------+ +-+---------+ | | | +-+------------+
>>> | | |
>>> | | +---> +--------------+
>>> | | | Error Status |
>>> | | | Data Block 2 |
>>> | | | +------------+
>>> | | | | CPER |
>>> | | | | CPER |
>>> | | +-+------------+
>>> | |
>>> | +-----> +--------------+
>>> | | Error Status |
>>> | | Data Block 3 |
>>> | | +------------+
>>> | | | CPER |
>>> | +-+------------+
>>> | ...........
>>> +--------> +--------------+
>>> | Error Status |
>>> | Data Block 10|
>>> | +------------+
>>> | | CPER |
>>> | | CPER |
>>> | | CPER |
>>> +-+------------+
>>>
>>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>>> ---
>>> default-configs/arm-softmmu.mak | 1 +
>>> hw/acpi/Makefile.objs | 1 +
>>> hw/acpi/aml-build.c | 2 +
>>> hw/acpi/hest_ghes.c | 203 +++++++++++++++++++++++++++++++++++
>>> hw/arm/virt-acpi-build.c | 6 ++
>>> include/hw/acpi/acpi-defs.h | 227 ++++++++++++++++++++++++++++++++++++++++
>>> include/hw/acpi/aml-build.h | 1 +
>>> include/hw/acpi/hest_ghes.h | 43 ++++++++
>>> 8 files changed, 484 insertions(+)
>>> create mode 100644 hw/acpi/hest_ghes.c create mode 100644
>>> include/hw/acpi/hest_ghes.h
>
>> Next file:
>>
>>> diff --git a/include/hw/acpi/hest_ghes.h
>>> b/include/hw/acpi/hest_ghes.h new file mode 100644 index
>>> 0000000..0cadc2b
>>> --- /dev/null
>>> +++ b/include/hw/acpi/hest_ghes.h
>>> @@ -0,0 +1,43 @@
>>> +#ifndef ACPI_GHES_H
>>> +#define ACPI_GHES_H
>>> +
>>> +#include "hw/acpi/bios-linker-loader.h"
>>> +
>>> +#define GHES_ERRORS_FW_CFG_FILE "etc/hardware_errors"
>>> +#define GHES_DATA_ADDR_FW_CFG_FILE "etc/hardware_errors_addr"
>>> +
>>> +#define GAS_ADDRESS_OFFSET 4
>>> +#define ERROR_STATUS_ADDRESS_OFFSET 20
>>> +#define NOTIFICATION_STRUCTURE 32
>>> +
>>> +#define BFAPEI_OK 0
>>> +#define BFAPEI_FAIL 1
>>> +
>>> +/* The max number of error source, the error sources
>>> + * are classified by notification type, below is the definition
>>> + * 0 - Polled
>>> + * 1 - External Interrupt
>>> + * 2 - Local Interrupt
>>> + * 3 - SCI
>>> + * 4 - NMI
>>> + * 5 - CMCI
>>> + * 6 - MCE
>>> + * 7 - GPIO-Signal
>>> + * 8 - ARMv8 SEA
>>> + * 9 - ARMv8 SEI
>>> + * 10 - External Interrupt - GSIV
>>> + */
>>> +#define MAX_ERROR_SOURCE_COUNT_V6 11
>>
>> I'll have to review this header file more thoroughly, once I see the
>> code that references these macros. For now, I have one comment:
>>
>> (42) I think the notification type list should be removed from this
>> location. Also, the open-coded value 11 should be replaced with the
>> ACPI_HEST_NOTIFY_RESERVED enumeration constant.
>>
>> I will try to continue reviewing this patch sometime next week
>> (second half of the week at the earliest, I think).
>
> (43) I think all macros introduced in this header should all start
> with "GHES_".
Ok.
>
>>
>>> +/* The max size in Bytes for one error block */
>>> +#define MAX_RAW_DATA_LENGTH 0x1000
>>> +
>>> +typedef struct GhesErrorState {
>>> + uint64_t physical_addr;
>>> + uint64_t ghes_addr_le[8];
>>> +} GhesErrorState;
>
> (44) Should this be called "GhesState" instead (dropping "Error")?
good suggestion.
>
> (45) (This is a question for other reviewers) I have no idea if this
> abstraction should be a device (sysbus or otherwise). Should it be a
> device? Below we have a static ("global") variable of this type, which
> is quite unusual.
Before I also indeed consider this question that whether this abstraction should be a device, maybe global variable is clean. anyway I will consider again about this point.
>
> (46) What is "physical_addr" good for? Below I can only see an
> assignment to it, in ghes_update_guest(). Where is the field read?
this "physical_addr" address is the physical error address in the CPER.
such as the physical address that happen hwpoison, this address is delivered by the KVM and QEMU transfer this address to physical.
>
> (47) "ghes_addr_le" should be an array of eight uint8_t elements (for
> representing a single uint64_t in little endian). The declaration
> above has a typo; the element type is currently specified as uint64_t.
>
> It suffices for the firmware to pass the base address of
> "etc/hardware_errors" back to QEMU, all the other addresses can be
> computed in QEMU as needed.
>
> (If, for some reason you *do* need to pass back multiple addresses,
> then you should use
>
> uint8_t xxxx_addr_le[N][8];
>
> But even in this case, setting N to 8 doesn't look useful, because we
> have 11 error sources / notification types.)
Thanks for this suggestion. it seems good.
>
>>> +
>>> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
>>> + BIOSLinker *linker); void
>>> +ghes_add_fw_cfg(FWCfgState *s, GArray *guid);
>
> (48) The second parameter should be called "hardware_errors", not
> "guid".
Ok, will change it.
>
>>> +void ghes_update_guest(uint32_t notify, uint64_t physical_address);
>
> (49) Can you call the second parameter "error_physical_addr"?
Ok, will change it.
>
>>> +#endif
>>>
>
>>> diff --git a/default-configs/arm-softmmu.mak
>>> b/default-configs/arm-softmmu.mak index 1e3bd2b..d5f1552 100644
>>> --- a/default-configs/arm-softmmu.mak
>>> +++ b/default-configs/arm-softmmu.mak
>>> @@ -121,3 +121,4 @@ CONFIG_ACPI=y
>>> CONFIG_SMBIOS=y
>>> CONFIG_ASPEED_SOC=y
>>> CONFIG_GPIO_KEY=y
>>> +CONFIG_ACPI_APEI_GENERATION=y
>
>>> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs index
>>> 11c35bc..776b46e 100644
>>> --- a/hw/acpi/Makefile.objs
>>> +++ b/hw/acpi/Makefile.objs
>>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) +=
>>> memory_hotplug.o
>>> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
>>> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>>> common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
>>> +common-obj-$(CONFIG_ACPI_APEI_GENERATION) += hest_ghes.o
>>> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>>>
>>> common-obj-y += acpi_interface.o
>
> (50) I think "CONFIG_ACPI_APEI" would be more succinct.
OK, I will change it.
>
>>> diff --git a/include/hw/acpi/aml-build.h
>>> b/include/hw/acpi/aml-build.h index 00c21f1..c1d15b3 100644
>>> --- a/include/hw/acpi/aml-build.h
>>> +++ b/include/hw/acpi/aml-build.h
>>> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
>>> GArray *rsdp;
>>> GArray *tcpalog;
>>> GArray *vmgenid;
>>> + GArray *hardware_errors;
>>> BIOSLinker *linker;
>>> } AcpiBuildTables;
>>>
>
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index
>>> c6f2032..802b98d 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -1560,6 +1560,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(); }
>>>
>>> @@ -1570,6 +1571,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 */
>
> Looks good to me.
thanks.
>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 0835e59..e7ab5dc 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -45,6 +45,8 @@
>>> #include "hw/arm/virt.h"
>>> #include "sysemu/numa.h"
>>> #include "kvm_arm.h"
>>> +#include "hw/acpi/vmgenid.h"
>
> (51) I think this include directive is not necessary.
sorry, this is my typo error, will remove it.
>
>>> +#include "hw/acpi/hest_ghes.h"
>>>
>>> #define ARM_SPI_BASE 32
>>> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>> @@ -778,6 +780,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>> acpi_add_table(table_offsets, tables_blob);
>>> build_spcr(tables_blob, tables->linker, vms);
>>>
>>> + acpi_add_table(table_offsets, tables_blob);
>>> + ghes_build_acpi(tables_blob, tables->hardware_errors,
>>> + tables->linker);
>>> +
>>> if (nb_numa_nodes > 0) {
>>> acpi_add_table(table_offsets, tables_blob);
>>> build_srat(tables_blob, tables->linker, vms); @@ -892,6
>>> +897,7 @@ void virt_acpi_setup(VirtMachineState *vms)
>>>
>>> build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
>>> ACPI_BUILD_RSDP_FILE,
>>> 0);
>>> + ghes_add_fw_cfg(vms->fw_cfg, tables.hardware_errors);
>>>
>>> qemu_register_reset(virt_acpi_build_reset, build_state);
>>> virt_acpi_build_reset(build_state);
>
> (52) I think for consistency with existing code, this function call
> should be placed between the ACPI_BUILD_TPMLOG_FILE line and the
> acpi_add_rom_blob() line.
OK.
>
>>> diff --git a/hw/acpi/hest_ghes.c b/hw/acpi/hest_ghes.c new file mode
>>> 100644 index 0000000..91d382e
>>> --- /dev/null
>>> +++ b/hw/acpi/hest_ghes.c
>>> @@ -0,0 +1,203 @@
>>> +/*
>>> + * APEI GHES table Generation
>>> + *
>>> + * Copyright (C) 2017 huawei.
>>> + *
>>> + * Author: Dongjiu Geng <gengdongjiu@huawei.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qmp-commands.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"
>>> +
>>> +static int ghes_generate_cper_record(uint64_t block_error_address,
>>> + uint64_t error_physical_addr) {
>>> + AcpiGenericErrorStatus block;
>>> + AcpiGenericErrorData *gdata;
>>> + struct cper_sec_mem_err *mem_err;
>>> + uint64_t block_data_length;
>>> + unsigned char *buffer;
>>> +
>>> + cpu_physical_memory_read(block_error_address, &block,
>>> + sizeof(AcpiGenericErrorStatus));
>>> +
>>> + block_data_length = sizeof(AcpiGenericErrorStatus) +
>>> + block.data_length;
>
> (53) "block.data_length" must surely be converted from LE to
> host-endian, so please wrap it with le32_to_cpu().
OK.
>
>>> +
>>> + /* If the Generic Error Status Block is NULL, update
>>> + * the block header
>>> + */
>>> + if (!block.block_status) {
>>> + block.block_status = ACPI_BERT_UNCORRECTABLE;
>>> + block.error_severity = CPER_SEV_FATAL;
>>> + }
>>> +
>>> + block.data_length += sizeof(AcpiGenericErrorData);
>>> + block.data_length += sizeof(struct cper_sec_mem_err);
>
> (54) Conversion between LE and host-endian is missing.
OK.
>
> (55) What happens if you run out of the preallocated memory?
if it run out of the preallocated memory. it will overwrite other
error source. every block's size is fixed. so it does not easy dynamically extend the size if it is overflow.
Anyway I will add a error report if it happens overwrite.
>
>>> +
>>> + /* Write back the Generic Error Status Block to guest memory */
>>> + cpu_physical_memory_write(block_error_address, &block,
>>> + sizeof(AcpiGenericErrorStatus));
>>> +
>>> + /* Fill in Generic Error Data Entry */
>>> + buffer = g_malloc(sizeof(AcpiGenericErrorData) + sizeof(cper_sec_mem_err));
>>> + memset(buffer, 0, sizeof(AcpiGenericErrorData) +
>>> + sizeof(cper_sec_mem_err));
>
> (56) Please use g_malloc0() instead.
Ok.
>
>>> + gdata = (AcpiGenericErrorData *)buffer;
>>> +
>>> + memcpy(gdata->section_type, (void *) &CPER_SEC_PLATFORM_MEM,
>>> + sizeof(uuid_le));
>>> + gdata->error_data_length = sizeof(struct cper_sec_mem_err);
>
> (57) Endianness conversion missing (cpu_to_le32()).
Thanks for the reminder.
>
>>> +
>>> + mem_err = (struct cper_sec_mem_err *) (gdata + 1);
>>> +
>>> + /* In order to simplify simulation, hardcode the CPER section to memory
>>> + * section.
>>> + */
>>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
>>> + mem_err->error_type = 3;
>
> (58) Is this supposed to stand for "Multi-bit ECC" (from "N.2.5 Memory
> Error Section" in UEFI 2.6)? Should we have a macro for that?
Yes, it is. What do you mean a macro? For all the errors that happen in the guest OS,
in order to simulate easy, I abstract all the error section to memory section, even though the error section is processor or other section.
I do not know whether do you have some suggestion for that.
>>> +
>>> + mem_err->validation_bits |= CPER_MEM_VALID_PA;
>>> + mem_err->physical_addr = error_physical_addr;
>
> (59) Conversion between host and little endian is missing on all four
> lines above.
Thanks.
>
>>> +
>>> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
>>> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
>>> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
>>> + mem_err->card = 1;
>>> + mem_err->module = 2;
>>> + mem_err->bank = 3;
>>> + mem_err->row = 1;
>>> + mem_err->column = 2;
>>> + mem_err->bit_pos = 5;
>
> (60) I have no idea where these values come from.
For all the errors that happen in the guest OS,
in order to simulate easy, I abstract all the error section to memory section, and hard code the memory section error value as above.
>
>>> +
>>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_STATUS;
>>> + mem_err->error_status = 4 << 8;
>>> +
>>> + /* Write back the Generic Error Data Entry to guest memory */
>>> + cpu_physical_memory_write(block_error_address + block_data_length, buffer,
>>> + sizeof(AcpiGenericErrorData) +
>>> + sizeof(cper_sec_mem_err));
>
> (61) Please choose a better name for "block_data_length" -- it stands
> for the length *before* the increment.
OK, thanks for the suggestion.
>
> (62) Is it safe to write out the increased block length before writing
> the new CPER data?
It indeed needs to add a judgment to avoid the data overflow, if overflow, report an error.
As we talk before, the total size for a block is fixed. So cannot dynamic extend the total size. So if is overflowed, will let the CPER adding is failed.
>
>>> +
>>> + g_free(buffer);
>>> + return BFAPEI_OK;
>>> +}
>
> (63) This return code is not used. Can we remove it perhaps (together
> with the BFAPEI_* macros)?
OK. Thanks Laszlo. Will remove it.
>
>>> +
>>> +void ghes_build_acpi(GArray *table_data, GArray *hardware_error,
>>> + BIOSLinker *linker) {
>>> + Aml *hest;
>>> + uint32_t address_registers_offset;
>>> + AcpiTableHeader *header;
>>> + AcpiGenericHardwareErrorSource *error_source;
>>> + int i;
>>> +
>>> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
>
> (64) What does "reqr" stand for?
It stand for the request size.
>
>>> +
>>> + /* New address register and Error status block table size*/
>>> + g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6
>>> + * block_reqr_size);
>
> (65) The QEMU coding style is
>
> g_array_set_size(hardware_error, MAX_ERROR_SOURCE_COUNT_V6 *
> block_reqr_size);
>
> That is, the operator is at the end of the line.
>
> Several function calls below are affected by this; please fix them all
> up.
>
> Running "scripts/checkpatch.pl" on the patches before you post them
> should help catch this kind of problem early.
I have ran the "scripts/checkpatch.pl" before post these patches, it does not remind such errors. anyway I will modified it.
>
>>> +
>>> + /* Put this in a HEST table */
>>> + hest = init_aml_allocator();
>>> + address_registers_offset = table_data->len
>>> + + sizeof(AcpiHardwareErrorSourceTable)
>>> + + ERROR_STATUS_ADDRESS_OFFSET
>>> + + GAS_ADDRESS_OFFSET;
>
> (66) Please drop the ERROR_STATUS_ADDRESS_OFFSET and
> GAS_ADDRESS_OFFSET macros (from the header file as well), and use the
> following expressions
> instead:
>
> ...
> offsetof(AcpiGenericHardwareErrorSource, error_status_address) +
> offsetof(AcpiGenericAddress, address);
Good suggestion and point.
>
>>> + /* Reserve space for HEST table size*/
>>> + acpi_data_push(hest->buf, sizeof(AcpiHardwareErrorSourceTable)
>>> + + MAX_ERROR_SOURCE_COUNT_V6
>>> + *
>>> + sizeof(AcpiGenericHardwareErrorSource));
>>> +
>>> + g_array_append_vals(table_data, hest->buf->data,
>>> + hest->buf->len);
>
> (67) HEST is a data table, it contains no AML. Accordingly, you use
> "hest" *only* for pushing zero bytes to "table_data".
>
> But that's not a good enough reason to have "hest" at all, or to call
> init_aml_allocator() / free_aml_allocator(). Instead, please just
> calculate the needed size, and push that many bytes directly to
> "table_data".
The hest table indeed does not include AML, so will do it according to your suggestion.
>
> Your current code already populates those bytes within "table_data",
> so that's good.
>
>>> + /* Allocate guest memory for the Data fw_cfg blob */
>>> + bios_linker_loader_alloc(linker, GHES_ERRORS_FW_CFG_FILE,
>>> + hardware_error, 4096,
>>> + false /* page boundary, high memory
>>> + */);
>
> (68) In the code below, you are not taking an "OVMF header probe
> suppressor" into account.
>
> But, we have already planned to replace that quirk with a separate,
> dedicated allocation hint or command, so I'm not going to describe
> what an "OVMF header probe suppressor" is; instead, I'll describe the
> replacement for it.
>
> So please add a new patch to the series that does the following:
OK.
>
> - In "hw/acpi/bios-linker-loader.c", modify the documentation of
> "BiosLinkerLoaderEntry.alloc.zone". The most significant bit (i.e.,
> bit 7), when set, should mean that @alloc.file contains no ACPI
> tables. When the bit is clear, no information is given about the
> contents of @alloc.file (i.e., same as now).
>
> - This will require a small patch for SeaBIOS: mask off bit 7 of
> "entry->alloc.zone" in the switch statement in
> romfile_loader_allocate(), "src/fw/romfile_loader.c". SeaBIOS does not
> have to care about this bit.
Your description is detailed, thanks again, I will do that.
>
> - OVMF will need some code to handle this bit specially, but I will
> write that.
Ok, it is good.
>
> - In QEMU, please modify the bios_linker_loader_alloc() function so that
> it takes a "noacpi" boolean as well, and set the high bit of
> "entry.alloc.zone" from it. The default value for "noacpi" should be
> false, at all currently existing call sites.
>
> - And finally, in this patch, we should pass "true" as "noacpi".
I basically understand your idea.
>
>>> + header = (AcpiTableHeader *)(table_data->data
>>> + + table_data->len - hest->buf->len);
>>> + *(uint32_t *)(header + 1) = MAX_ERROR_SOURCE_COUNT_V6;
>
> (69) Ugh, this is ugly. Please access the error source count field
> through "AcpiHardwareErrorSourceTable.error_source_count".
OK.
>
>>> + error_source = (AcpiGenericHardwareErrorSource *)((char *)header
>>> + +
>>> + sizeof(AcpiHardwareErrorSourceTable));
>>> +
>>> + for (i = 0; i < MAX_ERROR_SOURCE_COUNT_V6; i++) {
>>> + error_source->type = ACPI_HEST_TYPE_GENERIC_ERROR;
>>> + error_source->source_id = 0;
>
> (70) I think this is wrong, this identifier should be unique among all
> error sources. You should assign cpu_to_le16(i).
Thanks for the point, you are right, this is wrong, the source id is Uniquely identify
>
>>> + error_source->related_source_id = 0xffff;
>>> + error_source->flags = 0;
>>> + error_source->enabled = 1;
>>> + error_source->number_of_records = 1;
>
> (71) Why do we pre-alloc only one record? Don't several CPER objects
> fit in an error status data block (of size MAX_RAW_DATA_LENGTH, 0x1000)?
From my understanding, number_of_records stands for the number of error data status data block.
Here setting the number_of_records to 1 means that one error source includes one error source status data block.
But One error source status data block can includes many CPER objects. So although pre-alloc only one record, but it can still
Includes several CPER objects.
>
>>> + error_source->max_sections_per_record = 1;
>>> + error_source->max_raw_data_length = MAX_RAW_DATA_LENGTH;
>>> + error_source->error_status_address.space_id =
>>> + ACPI_ADR_SPACE_SYSTEM_MEMORY;
>>> + error_source->error_status_address.bit_width = 64;
>>> + error_source->error_status_address.bit_offset = 0;
>>> + error_source->error_status_address.access_width = 4;
>>> + error_source->notify.type = i;
>
> OK, this is one byte wide only.
>
>>> + error_source->notify.length =
>>> + sizeof(AcpiGenericHardwareErrorSource);
>
> (72) This seems to be wrong. The right hand side should be
> sizeof(AcpiHestNotify).
Thanks Laszlo's carefully review, this is wrong, it should be sizeof(AcpiHestNotify).
>
>>> +
>>> + bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>>> + sizeof(uint64_t) * i, sizeof(uint64_t),
>>> + GHES_ERRORS_FW_CFG_FILE,
>>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>>> + i * MAX_RAW_DATA_LENGTH);
>
> (73) Looks good, but please indent the arguments 1 position relative
> to the opening paren.
Ok, thanks for the comment.
>
>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>> + address_registers_offset
>>> + + i * sizeof(AcpiGenericHardwareErrorSource),
>>> + sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>>> + i * sizeof(uint64_t));
>
> (74) Passing "sizeof(uint32_t)" as "dst_patched_size" is wrong. The
> address registers at the start of the "etc/hardware_errors" blob are
> 64-bit wide each.
thanks a lot for the point, you are right. I am wrong.
>
>>> +
>>> + bios_linker_loader_write_pointer(linker, GHES_DATA_ADDR_FW_CFG_FILE,
>>> + i * sizeof(uint64_t), sizeof(uint64_t),
>>> + GHES_ERRORS_FW_CFG_FILE,
>>> + MAX_ERROR_SOURCE_COUNT_V6 * sizeof(uint64_t) +
>>> + i * MAX_RAW_DATA_LENGTH);
>
> (75) We should create only one WRITE_POINTER command, for the base
> address of "etc/hardware_errors". This should be done outside of the
> loop.
>
> The base addresses of the individual error status data blocks should
> be calculated in ghes_update_guest(), based on the error source /
> notification type.
Thanks a lot for the review, you are right, I will modify it.
>
>>> + error_source++;
>>> + }
>>> +
>>> + build_header(linker, table_data,
>>> + (void *)header, "HEST", hest->buf->len, 1, NULL, "GHES");
>
> (76) Any particular reason for not passing NULL in "oem_table_id"?
>
>>> +
>>> + free_aml_allocator();
>>> +}
>>> +
>>> +static GhesErrorState ges;
>>> +void ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_error) {
>>> +
>>> + int block_reqr_size = sizeof(uint64_t) + MAX_RAW_DATA_LENGTH;
>>> + int size = MAX_ERROR_SOURCE_COUNT_V6 * block_reqr_size;
>
> (77) These variable should have type "size_t".
>
> (78) What does "reqr" stand for?
Reqr is request size for block.
>
> (79) I just noticed that the commit message lists error sources /
> notification types 1 through 10 (count=10 in total). However, we have
> 11 error sources actually. Can you update the commit message so that
> it mentions sources 0 through 10?
OK, thanks very much for your suggestion.
>
>>> +
>>> + /* Create a read-only fw_cfg file for GHES */
>>> + fw_cfg_add_file(s, GHES_ERRORS_FW_CFG_FILE, hardware_error->data,
>>> + size);
>>> + /* Create a read-write fw_cfg file for Address */
>>> + fw_cfg_add_file_callback(s, GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL,
>>> + &(ges.ghes_addr_le[0]),
>>> + sizeof(uint64_t) * MAX_ERROR_SOURCE_COUNT_V6,
>>> + false); }
>>> +
>
> (80) The size calculation for GHES_DATA_ADDR_FW_CFG_FILE is incorrect.
> First, it does not match the element count 8 that I highlighted under
> remark (47). Second (again), passing back just the base address is
> sufficient, so 8 uint8_t elements (for representing a single uint64_t
> in
> LE) are enough.
>
> In other words, the size calculation should be
> ARRAY_SIZE(ges.ghes_addr_le) here.
I will consider your suggestion, thanks very much.
>
>>> +void ghes_update_guest(uint32_t notify, uint64_t physical_address)
>>> +{
>>> + uint64_t block_error_addr;
>>> +
>>> + if (physical_address) {
>>> + ges.physical_addr = physical_address;
>>> + block_error_addr = ges.ghes_addr_le[notify];
>>> + block_error_addr = le32_to_cpu(block_error_addr);
>>> +
>>> + /* A zero value in ghes_addr means that BIOS has not yet written
>>> + * the address
>>> + */
>>> + if (block_error_addr) {
>>> + ghes_generate_cper_record(block_error_addr, physical_address);
>>> + }
>>> + }
>>> +}
>
> (81) First, the value of "notify" should be range-checked before using
> it as an array subscript. If it is outside of the permissible range,
> nothing should be done. Second, in order to locate the affected error
> status data block, we should move the offset calculation here, from
> ghes_build_acpi().
>
Thanks Laszlo, I will consider your suggestion.
> Thanks
> Laszlo
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-05-29 15:27 gengdongjiu
@ 2017-05-29 16:03 ` Laszlo Ersek
2017-05-31 2:13 ` gengdongjiu
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-29 16:03 UTC (permalink / raw)
To: gengdongjiu
Cc: ard.biesheuvel@linaro.org, edk2-devel@lists.01.org,
qemu-devel@nongnu.org, Zhaoshenglong, James Morse,
Christoffer Dall, Xiexiuqi, Marc Zyngier, catalin.marinas@arm.com,
will.deacon@arm.com, christoffer.dall@linaro.org,
rkrcmar@redhat.com, suzuki.poulose@arm.com,
andre.przywara@arm.com, mark.rutland@arm.com,
vladimir.murzin@arm.com, linux-arm-kernel@lists.infradead.org,
kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, wangxiongfeng (C), Wuquanming,
Huangshaoyu, Leif.Lindholm@linaro.com
Hi,
did you remove me from the To: / Cc: list intentionally, or was that an
oversight? I caught your message in my list folders only by luck.
Some followup below:
On 05/29/17 17:27, gengdongjiu wrote:
>> (46) What is "physical_addr" good for? Below I can only see an
>> assignment to it, in ghes_update_guest(). Where is the field read?
> this "physical_addr" address is the physical error address in the
> CPER. such as the physical address that happen hwpoison, this address
> is delivered by the KVM and QEMU transfer this address to physical.
I understand that in the ghes_update_guest() function, you accept a
parameter called "physical_address", and you pass it on to
ghes_generate_cper_record(). That makes sense, yes.
However, you also assign the same value to "ges.physical_addr". And that
structure field is never read. So my point is that the
"GhesErrorState.physical_addr" field is superfluous and should be removed.
I checked the other three patches in the series and they don't seem to
read that structure member either. Correct me if I'm wrong.
>> (55) What happens if you run out of the preallocated memory?
> if it run out of the preallocated memory. it will overwrite other
> error source. every block's size is fixed. so it does not easy
> dynamically extend the size if it is overflow. Anyway I will add a
> error report if it happens overwrite.
I understand (and agree) that dynamic allocation is not possible here.
But that doesn't justify overwriting the error status data block that
belongs to a different data source. (Worse, if this happens with the
last error status data block, for error source 10, you could overwrite
memory that belongs to the OS.)
If an error status data block becomes full, then we should either wrap
back to the start of the same data block, or else stop forwarding errors
for that error source.
Does the ACPI spec say anything about this? I.e., about the case when
the system runs out of the memory that was reserved for recording
hardware errors?
>>>> +
>>>> + mem_err = (struct cper_sec_mem_err *) (gdata + 1);
>>>> +
>>>> + /* In order to simplify simulation, hardcode the CPER section to memory
>>>> + * section.
>>>> + */
>>>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
>>>> + mem_err->error_type = 3;
>>
>> (58) Is this supposed to stand for "Multi-bit ECC" (from "N.2.5 Memory
>> Error Section" in UEFI 2.6)? Should we have a macro for that?
> Yes, it is. What do you mean a macro?
A #define for the integer value 3.
> For all the errors that happen in the guest OS, in order to simulate
> easy, I abstract all the error section to memory section, even though
> the error section is processor or other section.
Why is that a valid thing to do? (I'm not doubting it is valid, I'm just
asking.) Will that not confuse the ACPI subsystem of the guest OS?
> I do not know whether do you have some suggestion for that.
Well I would have thought (without any expertise on the subject) that
hardware errors from the host side should be mapped to the guest more or
less "type correct". IOW, it looks strange that, say, a CPU error is
reported as a memory error. But this is just an uneducated guess.
>>>> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
>>>> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
>>>> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
>>>> + mem_err->card = 1;
>>>> + mem_err->module = 2;
>>>> + mem_err->bank = 3;
>>>> + mem_err->row = 1;
>>>> + mem_err->column = 2;
>>>> + mem_err->bit_pos = 5;
>>
>> (60) I have no idea where these values come from.
> For all the errors that happen in the guest OS, in order to simulate
> easy, I abstract all the error section to memory section, and hard
> code the memory section error value as above.
Sure, but why is that safe? Will the guest OS not want to do something
about these error details? If we are feeding the guest OS invalid error
details, will that not lead to confusion?
>> (64) What does "reqr" stand for?
> It stand for the request size.
Can you please call it "req_size" or something similar? The English
expression
request size
contains only one "r" letter, so it's hard to understand where the
second "r" in "reqr" comes from.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support
2017-05-29 16:03 ` Laszlo Ersek
@ 2017-05-31 2:13 ` gengdongjiu
0 siblings, 0 replies; 17+ messages in thread
From: gengdongjiu @ 2017-05-31 2:13 UTC (permalink / raw)
To: Laszlo Ersek
Cc: ard.biesheuvel@linaro.org, edk2-devel@lists.01.org,
qemu-devel@nongnu.org, Zhaoshenglong, James Morse,
Christoffer Dall, Xiexiuqi, Marc Zyngier, catalin.marinas@arm.com,
will.deacon@arm.com, christoffer.dall@linaro.org,
rkrcmar@redhat.com, suzuki.poulose@arm.com,
andre.przywara@arm.com, mark.rutland@arm.com,
vladimir.murzin@arm.com, linux-arm-kernel@lists.infradead.org,
kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, wangxiongfeng (C), Wuquanming,
Huangshaoyu, Leif.Lindholm@linaro.com
Laszlo,
very sorry for that, it was my mistake that missing your name.
when I reply mail, I copy the "CC" list to the mail reply list, but forget to copy the "To" list.
I will check your comments in detailed later and reply you. thanks again.
On 2017/5/30 0:03, Laszlo Ersek wrote:
> Hi,
>
> did you remove me from the To: / Cc: list intentionally, or was that an
> oversight? I caught your message in my list folders only by luck.
>
> Some followup below:
>
> On 05/29/17 17:27, gengdongjiu wrote:
>
>>> (46) What is "physical_addr" good for? Below I can only see an
>>> assignment to it, in ghes_update_guest(). Where is the field read?
>
>> this "physical_addr" address is the physical error address in the
>> CPER. such as the physical address that happen hwpoison, this address
>> is delivered by the KVM and QEMU transfer this address to physical.
> I understand that in the ghes_update_guest() function, you accept a
> parameter called "physical_address", and you pass it on to
> ghes_generate_cper_record(). That makes sense, yes.
>
> However, you also assign the same value to "ges.physical_addr". And that
> structure field is never read. So my point is that the
> "GhesErrorState.physical_addr" field is superfluous and should be removed.
>
> I checked the other three patches in the series and they don't seem to
> read that structure member either. Correct me if I'm wrong.
>
>>> (55) What happens if you run out of the preallocated memory?
>
>> if it run out of the preallocated memory. it will overwrite other
>> error source. every block's size is fixed. so it does not easy
>> dynamically extend the size if it is overflow. Anyway I will add a
>> error report if it happens overwrite.
> I understand (and agree) that dynamic allocation is not possible here.
>
> But that doesn't justify overwriting the error status data block that
> belongs to a different data source. (Worse, if this happens with the
> last error status data block, for error source 10, you could overwrite
> memory that belongs to the OS.)
>
> If an error status data block becomes full, then we should either wrap
> back to the start of the same data block, or else stop forwarding errors
> for that error source.
>
> Does the ACPI spec say anything about this? I.e., about the case when
> the system runs out of the memory that was reserved for recording
> hardware errors?
>
>>>>> +
>>>>> + mem_err = (struct cper_sec_mem_err *) (gdata + 1);
>>>>> +
>>>>> + /* In order to simplify simulation, hardcode the CPER section to memory
>>>>> + * section.
>>>>> + */
>>>>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE;
>>>>> + mem_err->error_type = 3;
>>>
>>> (58) Is this supposed to stand for "Multi-bit ECC" (from "N.2.5 Memory
>>> Error Section" in UEFI 2.6)? Should we have a macro for that?
>
>> Yes, it is. What do you mean a macro?
>
> A #define for the integer value 3.
>
>> For all the errors that happen in the guest OS, in order to simulate
>> easy, I abstract all the error section to memory section, even though
>> the error section is processor or other section.
> Why is that a valid thing to do? (I'm not doubting it is valid, I'm just
> asking.) Will that not confuse the ACPI subsystem of the guest OS?
>
>> I do not know whether do you have some suggestion for that.
> Well I would have thought (without any expertise on the subject) that
> hardware errors from the host side should be mapped to the guest more or
> less "type correct". IOW, it looks strange that, say, a CPU error is
> reported as a memory error. But this is just an uneducated guess.
>
>>>>> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE |
>>>>> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW |
>>>>> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION;
>>>>> + mem_err->card = 1;
>>>>> + mem_err->module = 2;
>>>>> + mem_err->bank = 3;
>>>>> + mem_err->row = 1;
>>>>> + mem_err->column = 2;
>>>>> + mem_err->bit_pos = 5;
>>>
>>> (60) I have no idea where these values come from.
>
>> For all the errors that happen in the guest OS, in order to simulate
>> easy, I abstract all the error section to memory section, and hard
>> code the memory section error value as above.
> Sure, but why is that safe? Will the guest OS not want to do something
> about these error details? If we are feeding the guest OS invalid error
> details, will that not lead to confusion?
>
>>> (64) What does "reqr" stand for?
>> It stand for the request size.
> Can you please call it "req_size" or something similar? The English
> expression
>
> request size
>
> contains only one "r" letter, so it's hard to understand where the
> second "r" in "reqr" comes from.
>
> Thanks,
> Laszlo
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-07-09 3:41 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-30 5:35 [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 2/4] target-arm: kvm64: detect guest RAS EXTENSION feature Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 3/4] target-arm: kvm64: handle SIGBUS signal for synchronous External Abort Dongjiu Geng
2017-04-30 5:35 ` [Qemu-devel] [PATCH v3 4/4] target-arm: kvm64: handle SError interrupt for RAS extension Dongjiu Geng
2017-05-12 21:00 ` [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support Laszlo Ersek
2017-05-20 5:35 ` gengdongjiu
2017-05-22 14:23 ` Laszlo Ersek
2017-06-03 12:01 ` Laszlo Ersek
2017-06-24 14:20 ` gengdongjiu
2017-07-07 8:32 ` gengdongjiu
2017-07-07 9:43 ` Laszlo Ersek
2017-07-09 3:41 ` gengdongjiu
2017-05-12 23:59 ` Michael S. Tsirkin
2017-05-20 5:47 ` gengdongjiu
-- strict thread matches above, loose matches on Subject: below --
2017-05-29 15:27 gengdongjiu
2017-05-29 16:03 ` Laszlo Ersek
2017-05-31 2:13 ` gengdongjiu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).