[PATCH V3 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

[PATCH V3 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

[Tyler Baicar]

When a memory error, CPU error, PCIe error, or other type of hardware error
that's covered by RAS occurs, firmware should populate the shared GHES memory
location with the proper GHES structures to notify the OS of the error.
For example, platforms that implement firmware first handling may implement
separate GHES sources for corrected errors and uncorrected errors. If the
error is an uncorrectable error, then the firmware will notify the OS
immediately since the error needs to be handled ASAP. The OS will then be able
to take the appropriate action needed such as offlining a page. If the error
is a corrected error, then the firmware will not interrupt the OS immediately.
Instead, the OS will see and report the error the next time it's GHES timer
expires. The kernel will first parse the GHES structures and report the errors
through the kernel logs and then notify the user space through RAS trace
events. This allows user space applications such as RAS Daemon to see the
errors and report them however the user desires. This patchset extends the
kernel functionality for RAS errors based on updates in the UEFI 2.6 and
ACPI 6.1 specifications.

An example flow from firmware to user space could be:

                 +---------------+
       +-------->|               |
       |         |  GHES polling |--+
+-------------+  |    source     |  |   +---------------+   +------------+
|             |  +---------------+  |   |  Kernel GHES  |   |            |
|  Firmware   |                     +-->|  CPER AER and |-->|  RAS trace |
|             |  +---------------+  |   |  EDAC drivers |   |   event    |
+-------------+  |               |  |   +---------------+   +------------+
       |         |  GHES sci     |--+
       +-------->|   source      |
                 +---------------+

Add support for Generic Hardware Error Source (GHES) v2, which introduces the
capability for the OS to acknowledge the consumption of the error record
generated by the Reliability, Availability and Serviceability (RAS) controller.
This eliminates potential race conditions between the OS and the RAS controller.

Add support for the timestamp field added to the Generic Error Data Entry v3,
allowing the OS to log the time that the error is generated by the firmware,
rather than the time the error is consumed. This improves the correctness of
event sequences when analyzing error logs. The timestamp is added in
ACPI 6.1, reference Table 18-343 Generic Error Data Entry.

Add support for ARMv8 Common Platform Error Record (CPER) per UEFI 2.6
specification. ARMv8 specific processor error information is reported as part of
the CPER records.  This provides more detail on for processor error logs. This
can help describe ARMv8 cache, tlb, and bus errors.

Synchronous External Abort (SEA) represents a specific processor error condition
in ARM systems. A handler is added to recognize SEA errors, and a notifier is
added to parse and report the errors before the process is killed. Refer to
section N.2.1.1 in the Common Platform Error Record appendix of the UEFI 2.6
specification.

Currently the kernel ignores CPER records that are unrecognized.
On the other hand, UEFI spec allows for non-standard (eg. vendor
proprietary) error section type in CPER (Common Platform Error Record),
as defined in section N2.3 of UEFI version 2.5. Therefore, user
is not able to see hardware error data of non-standard section.

If section Type field of Generic Error Data Entry is unrecognized,
prints out the raw data in dmesg buffer, and also adds a tracepoint
for reporting such hardware errors.

Currently even if an error status block's severity is fatal, the kernel
does not honor the severity level and panic. With the firmware first
model, the platform could inform the OS about a fatal hardware error
through the non-NMI GHES notification type. The OS should panic when a
hardware error record is received with this severity.

Add support to handle SEAs that occur while a KVM guest kernel is
running. Currently these are unsupported by the guest abort handling.

Depends on: [PATCH v14] acpi, apei, arm64: APEI initial support for aarch64.
            https://lkml.org/lkml/2016/8/10/231

V3: Fix unmapped address to the read_ack_register in ghes.c
    Add helper function to get the proper payload based on generic data entry
     version
    Move timestamp print to avoid changing function calls in cper.c
    Remove patch "arm64: exception: handle instruction abort at current EL"
     since the el1_ia handler is already added in 4.8
    Add EFI and ARM64 dependencies for HAVE_ACPI_APEI_SEA
    Add a new trace event for ARM type errors
    Add support to handle KVM guest SEAs

V2: Add PSCI state print for the ARMv8 error type.
    Separate timestamp year into year and century using BCD format.
    Rebase on top of ACPICA 20160318 release and remove header file changes
     in include/acpi/actbl1.h.
    Add panic OS with fatal error status block patch.
    Add processing of unrecognized CPER error section patches with updates
     from previous comments. Original patches: https://lkml.org/lkml/2015/9/8/646

V1: https://lkml.org/lkml/2016/2/5/544

Jonathan (Zhixiong) Zhang (1):
  acpi: apei: panic OS with fatal error status block

Tyler Baicar (9):
  acpi: apei: read ack upon ghes record consumption
  ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1
  efi: parse ARMv8 processor error
  arm64: exception: handle Synchronous External Abort
  acpi: apei: handle SEA notification type for ARMv8
  efi: print unrecognized CPER section
  ras: acpi / apei: generate trace event for unrecognized CPER section
  trace, ras: add ARM processor error trace event
  arm64: KVM: add guest SEA support

 arch/arm/include/asm/kvm_arm.h       |   1 +
 arch/arm/include/asm/system_misc.h   |   5 +
 arch/arm/kvm/mmu.c                   |  15 ++-
 arch/arm64/Kconfig                   |   1 +
 arch/arm64/include/asm/kvm_arm.h     |   1 +
 arch/arm64/include/asm/system_misc.h |  15 +++
 arch/arm64/mm/fault.c                |  71 ++++++++--
 drivers/acpi/apei/Kconfig            |  15 +++
 drivers/acpi/apei/ghes.c             | 177 +++++++++++++++++++++++-
 drivers/acpi/apei/hest.c             |   7 +-
 drivers/firmware/efi/cper.c          | 252 ++++++++++++++++++++++++++++++++---
 drivers/ras/ras.c                    |   2 +
 include/acpi/ghes.h                  |   1 +
 include/linux/cper.h                 |  72 ++++++++++
 include/ras/ras_event.h              | 112 ++++++++++++++++
 15 files changed, 711 insertions(+), 36 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 01/10] acpi: apei: read ack upon ghes record consumption

[Tyler Baicar]

A RAS (Reliability, Availability, Serviceability) controller
may be a separate processor running in parallel with OS
execution, and may generate error records for consumption by
the OS. If the RAS controller produces multiple error records,
then they may be overwritten before the OS has consumed them.

The Generic Hardware Error Source (GHES) v2 structure
introduces the capability for the OS to acknowledge the
consumption of the error record generated by the RAS
controller. A RAS controller supporting GHESv2 shall wait for
the acknowledgment before writing a new error record, thus
eliminating the race condition.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
---
 drivers/acpi/apei/ghes.c | 41 +++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/apei/hest.c |  7 +++++--
 include/acpi/ghes.h      |  1 +
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 60746ef..3021f0e 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -45,6 +45,7 @@
 #include <linux/aer.h>
 #include <linux/nmi.h>
 
+#include <acpi/actbl1.h>
 #include <acpi/ghes.h>
 #include <acpi/apei.h>
 #include <asm/tlbflush.h>
@@ -244,10 +245,22 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 	struct ghes *ghes;
 	unsigned int error_block_length;
 	int rc;
+	struct acpi_hest_header *hest_hdr;
 
 	ghes = kzalloc(sizeof(*ghes), GFP_KERNEL);
 	if (!ghes)
 		return ERR_PTR(-ENOMEM);
+
+	hest_hdr = (struct acpi_hest_header *)generic;
+	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR_V2) {
+		ghes->generic_v2 = (struct acpi_hest_generic_v2 *)generic;
+		rc = apei_map_generic_address(
+			&ghes->generic_v2->read_ack_register);
+		if (rc)
+			goto err_unmap;
+	} else
+		ghes->generic_v2 = NULL;
+
 	ghes->generic = generic;
 	rc = apei_map_generic_address(&generic->error_status_address);
 	if (rc)
@@ -270,6 +283,9 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
 
 err_unmap:
 	apei_unmap_generic_address(&generic->error_status_address);
+	if (ghes->generic_v2)
+		apei_unmap_generic_address(
+			&ghes->generic_v2->read_ack_register);
 err_free:
 	kfree(ghes);
 	return ERR_PTR(rc);
@@ -279,6 +295,9 @@ static void ghes_fini(struct ghes *ghes)
 {
 	kfree(ghes->estatus);
 	apei_unmap_generic_address(&ghes->generic->error_status_address);
+	if (ghes->generic_v2)
+		apei_unmap_generic_address(
+			&ghes->generic_v2->read_ack_register);
 }
 
 static inline int ghes_severity(int severity)
@@ -648,6 +667,22 @@ static void ghes_estatus_cache_add(
 	rcu_read_unlock();
 }
 
+static int ghes_do_read_ack(struct acpi_hest_generic_v2 *generic_v2)
+{
+	int rc;
+	u64 val = 0;
+
+	rc = apei_read(&val, &generic_v2->read_ack_register);
+	if (rc)
+		return rc;
+	val &= generic_v2->read_ack_preserve <<
+		generic_v2->read_ack_register.bit_offset;
+	val |= generic_v2->read_ack_write;
+	rc = apei_write(val, &generic_v2->read_ack_register);
+
+	return rc;
+}
+
 static int ghes_proc(struct ghes *ghes)
 {
 	int rc;
@@ -660,6 +695,12 @@ static int ghes_proc(struct ghes *ghes)
 			ghes_estatus_cache_add(ghes->generic, ghes->estatus);
 	}
 	ghes_do_proc(ghes, ghes->estatus);
+
+	if (ghes->generic_v2) {
+		rc = ghes_do_read_ack(ghes->generic_v2);
+		if (rc)
+			return rc;
+	}
 out:
 	ghes_clear_estatus(ghes);
 	return 0;
diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 792a0d9..ef725a9 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -52,6 +52,7 @@ static const int hest_esrc_len_tab[ACPI_HEST_TYPE_RESERVED] = {
 	[ACPI_HEST_TYPE_AER_ENDPOINT] = sizeof(struct acpi_hest_aer),
 	[ACPI_HEST_TYPE_AER_BRIDGE] = sizeof(struct acpi_hest_aer_bridge),
 	[ACPI_HEST_TYPE_GENERIC_ERROR] = sizeof(struct acpi_hest_generic),
+	[ACPI_HEST_TYPE_GENERIC_ERROR_V2] = sizeof(struct acpi_hest_generic_v2),
 };
 
 static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
@@ -146,7 +147,8 @@ static int __init hest_parse_ghes_count(struct acpi_hest_header *hest_hdr, void
 {
 	int *count = data;
 
-	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR)
+	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR ||
+	    hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR_V2)
 		(*count)++;
 	return 0;
 }
@@ -157,7 +159,8 @@ static int __init hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
 	struct ghes_arr *ghes_arr = data;
 	int rc, i;
 
-	if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR)
+	if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR &&
+	    hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR_V2)
 		return 0;
 
 	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 720446c..d0108b6 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -14,6 +14,7 @@
 
 struct ghes {
 	struct acpi_hest_generic *generic;
+	struct acpi_hest_generic_v2 *generic_v2;
 	struct acpi_hest_generic_status *estatus;
 	u64 buffer_paddr;
 	unsigned long flags;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Tyler Baicar]

Currently when a RAS error is reported it is not timestamped.
The ACPI 6.1 spec adds the timestamp field to the generic error
data entry v3 structure. The timestamp of when the firmware
generated the error is now being reported.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
---
 drivers/acpi/apei/ghes.c    | 25 ++++++++++--
 drivers/firmware/efi/cper.c | 97 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 105 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 3021f0e..c8488f1 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -80,6 +80,10 @@
 	((struct acpi_hest_generic_status *)				\
 	 ((struct ghes_estatus_node *)(estatus_node) + 1))
 
+#define acpi_hest_generic_data_version(gdata)			\
+	(gdata->revision >> 8)
+
+
 /*
  * This driver isn't really modular, however for the time being,
  * continuing to use module_param is the easiest way to remain
@@ -412,6 +416,13 @@ static void ghes_clear_estatus(struct ghes *ghes)
 	ghes->flags &= ~GHES_TO_CLEAR;
 }
 
+inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
+{
+	return acpi_hest_generic_data_version(gdata) >= 3 ?
+		(void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
+		gdata + 1;
+}
+
 static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int sev)
 {
 #ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
@@ -419,7 +430,8 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
 	int flags = -1;
 	int sec_sev = ghes_severity(gdata->error_severity);
 	struct cper_sec_mem_err *mem_err;
-	mem_err = (struct cper_sec_mem_err *)(gdata + 1);
+
+	mem_err = acpi_hest_generic_data_payload(gdata);
 
 	if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
 		return;
@@ -449,14 +461,18 @@ static void ghes_do_proc(struct ghes *ghes,
 {
 	int sev, sec_sev;
 	struct acpi_hest_generic_data *gdata;
+	uuid_le sec_type;
 
 	sev = ghes_severity(estatus->error_severity);
 	apei_estatus_for_each_section(estatus, gdata) {
 		sec_sev = ghes_severity(gdata->error_severity);
-		if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
+		sec_type = *(uuid_le *)gdata->section_type;
+
+		if (!uuid_le_cmp(sec_type,
 				 CPER_SEC_PLATFORM_MEM)) {
 			struct cper_sec_mem_err *mem_err;
-			mem_err = (struct cper_sec_mem_err *)(gdata+1);
+
+			mem_err = acpi_hest_generic_data_payload(gdata);
 			ghes_edac_report_mem_error(ghes, sev, mem_err);
 
 			arch_apei_report_mem_error(sev, mem_err);
@@ -466,7 +482,8 @@ static void ghes_do_proc(struct ghes *ghes,
 		else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
 				      CPER_SEC_PCIE)) {
 			struct cper_sec_pcie *pcie_err;
-			pcie_err = (struct cper_sec_pcie *)(gdata+1);
+
+			pcie_err = acpi_hest_generic_data_payload(gdata);
 			if (sev == GHES_SEV_RECOVERABLE &&
 			    sec_sev == GHES_SEV_RECOVERABLE &&
 			    pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index d425374..9fa1317 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -32,9 +32,14 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/aer.h>
+#include <linux/printk.h>
+#include <linux/bcd.h>
 
 #define INDENT_SP	" "
 
+#define acpi_hest_generic_data_version(gdata)		\
+	(gdata->revision >> 8)
+
 static char rcd_decode_str[CPER_REC_LEN];
 
 /*
@@ -386,13 +391,47 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
 	pfx, pcie->bridge.secondary_status, pcie->bridge.control);
 }
 
+static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
+{
+	return acpi_hest_generic_data_version(gdata) >= 3 ?
+		(void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
+		gdata + 1;
+}
+
+static void cper_estatus_print_section_v300(const char *pfx,
+	const struct acpi_hest_generic_data_v300 *gdata)
+{
+	__u8 hour, min, sec, day, mon, year, century, *timestamp;
+
+	if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
+		timestamp = (__u8 *)&(gdata->time_stamp);
+		memcpy(&sec, timestamp, 1);
+		memcpy(&min, timestamp + 1, 1);
+		memcpy(&hour, timestamp + 2, 1);
+		memcpy(&day, timestamp + 4, 1);
+		memcpy(&mon, timestamp + 5, 1);
+		memcpy(&year, timestamp + 6, 1);
+		memcpy(&century, timestamp + 7, 1);
+		printk("%stime: ", pfx);
+		printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
+		printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
+			bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
+			bcd2bin(century), bcd2bin(year), bcd2bin(mon),
+			bcd2bin(day));
+	}
+}
+
 static void cper_estatus_print_section(
-	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
+	const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
 {
 	uuid_le *sec_type = (uuid_le *)gdata->section_type;
 	__u16 severity;
 	char newpfx[64];
 
+	if ((gdata->revision >> 8) >= 0x03)
+		cper_estatus_print_section_v300(pfx,
+			(const struct acpi_hest_generic_data_v300 *)gdata);
+
 	severity = gdata->error_severity;
 	printk("%s""Error %d, type: %s\n", pfx, sec_no,
 	       cper_severity_str(severity));
@@ -403,14 +442,18 @@ static void cper_estatus_print_section(
 
 	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
 	if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_GENERIC)) {
-		struct cper_sec_proc_generic *proc_err = (void *)(gdata + 1);
+		struct cper_sec_proc_generic *proc_err;
+
+		proc_err = acpi_hest_generic_data_payload(gdata);
 		printk("%s""section_type: general processor error\n", newpfx);
 		if (gdata->error_data_length >= sizeof(*proc_err))
 			cper_print_proc_generic(newpfx, proc_err);
 		else
 			goto err_section_too_small;
 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
-		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
+		struct cper_sec_mem_err *mem_err;
+
+		mem_err = acpi_hest_generic_data_payload(gdata);
 		printk("%s""section_type: memory error\n", newpfx);
 		if (gdata->error_data_length >=
 		    sizeof(struct cper_sec_mem_err_old))
@@ -419,7 +462,9 @@ static void cper_estatus_print_section(
 		else
 			goto err_section_too_small;
 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) {
-		struct cper_sec_pcie *pcie = (void *)(gdata + 1);
+		struct cper_sec_pcie *pcie;
+
+		pcie = acpi_hest_generic_data_payload(gdata);
 		printk("%s""section_type: PCIe error\n", newpfx);
 		if (gdata->error_data_length >= sizeof(*pcie))
 			cper_print_pcie(newpfx, pcie, gdata);
@@ -438,6 +483,7 @@ void cper_estatus_print(const char *pfx,
 			const struct acpi_hest_generic_status *estatus)
 {
 	struct acpi_hest_generic_data *gdata;
+	struct acpi_hest_generic_data_v300 *gdata_v3 = NULL;
 	unsigned int data_len, gedata_len;
 	int sec_no = 0;
 	char newpfx[64];
@@ -451,12 +497,22 @@ void cper_estatus_print(const char *pfx,
 	printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
 	data_len = estatus->data_length;
 	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
+	if ((gdata->revision >> 8) >= 0x03)
+		gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
+
 	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
+
 	while (data_len >= sizeof(*gdata)) {
 		gedata_len = gdata->error_data_length;
 		cper_estatus_print_section(newpfx, gdata, sec_no);
-		data_len -= gedata_len + sizeof(*gdata);
-		gdata = (void *)(gdata + 1) + gedata_len;
+		if(gdata_v3) {
+			data_len -= gedata_len + sizeof(*gdata_v3);
+			gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
+			gdata = (struct acpi_hest_generic_data *)gdata_v3;
+		} else {
+			data_len -= gedata_len + sizeof(*gdata);
+			gdata = (void *)(gdata + 1) + gedata_len;
+		}
 		sec_no++;
 	}
 }
@@ -478,6 +534,7 @@ EXPORT_SYMBOL_GPL(cper_estatus_check_header);
 int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
 {
 	struct acpi_hest_generic_data *gdata;
+	struct acpi_hest_generic_data_v300 *gdata_v3 = NULL;
 	unsigned int data_len, gedata_len;
 	int rc;
 
@@ -486,15 +543,29 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
 		return rc;
 	data_len = estatus->data_length;
 	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
-	while (data_len >= sizeof(*gdata)) {
-		gedata_len = gdata->error_data_length;
-		if (gedata_len > data_len - sizeof(*gdata))
+
+	if ((gdata->revision >> 8) >= 0x03) {
+		gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
+		while (data_len >= sizeof(*gdata_v3)) {
+			gedata_len = gdata_v3->error_data_length;
+			if (gedata_len > data_len - sizeof(*gdata_v3))
+				return -EINVAL;
+			data_len -= gedata_len + sizeof(*gdata_v3);
+			gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
+		}
+		if (data_len)
+			return -EINVAL;
+	} else {
+		while (data_len >= sizeof(*gdata)) {
+			gedata_len = gdata->error_data_length;
+			if (gedata_len > data_len - sizeof(*gdata))
+				return -EINVAL;
+			data_len -= gedata_len + sizeof(*gdata);
+			gdata = (void *)(gdata + 1) + gedata_len;
+		}
+		if (data_len)
 			return -EINVAL;
-		data_len -= gedata_len + sizeof(*gdata);
-		gdata = (void *)(gdata + 1) + gedata_len;
 	}
-	if (data_len)
-		return -EINVAL;
 
 	return 0;
 }
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 03/10] efi: parse ARMv8 processor error

[Tyler Baicar]

Add support for ARMv8 Common Platform Error Record (CPER).
UEFI 2.6 specification adds support for ARMv8 specific
processor error information to be reported as part of the
CPER records. This provides more detail on for processor error logs.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Tyler Baicar <tbaicar-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Naveen Kaje <nkaje-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/firmware/efi/cper.c | 135 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cper.h        |  72 +++++++++++++++++++++++
 2 files changed, 207 insertions(+)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 9fa1317..2594245 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -112,12 +112,15 @@ void cper_print_bits(const char *pfx, unsigned int bits,
 static const char * const proc_type_strs[] = {
 	"IA32/X64",
 	"IA64",
+	"ARMv8",
 };
 
 static const char * const proc_isa_strs[] = {
 	"IA32",
 	"IA64",
 	"X64",
+	"ARM A32/T32",
+	"ARM A64",
 };
 
 static const char * const proc_error_type_strs[] = {
@@ -186,6 +189,129 @@ static void cper_print_proc_generic(const char *pfx,
 		printk("%s""IP: 0x%016llx\n", pfx, proc->ip);
 }
 
+static void cper_print_proc_armv8(const char *pfx,
+				  const struct cper_sec_proc_armv8 *proc)
+{
+	int i, len;
+	struct cper_armv8_err_info *err_info;
+	__u64 *qword = NULL;
+	char newpfx[64];
+
+	printk("%ssection length: %d\n", pfx, proc->section_length);
+	printk("%sMIDR: 0x%016llx\n", pfx, proc->midr);
+
+	len = proc->section_length - (sizeof(*proc) +
+		proc->err_info_num * (sizeof(*err_info)));
+	if (len < 0) {
+		printk("%ssection length is too small.\n", pfx);
+		printk("%sERR_INFO_NUM is %d.\n", pfx, proc->err_info_num);
+		return;
+	}
+
+	if (proc->validation_bits & CPER_ARMV8_VALID_MPIDR)
+		printk("%sMPIDR: 0x%016llx\n", pfx, proc->mpidr);
+	if (proc->validation_bits & CPER_ARMV8_VALID_AFFINITY_LEVEL)
+		printk("%serror affinity level: %d\n", pfx,
+			proc->affinity_level);
+	if (proc->validation_bits & CPER_ARMV8_VALID_RUNNING_STATE) {
+		printk("%srunning state: %d\n", pfx, proc->running_state);
+		printk("%sPSCI state: %d\n", pfx, proc->psci_state);
+	}
+
+	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
+
+	err_info = (struct cper_armv8_err_info *)(proc + 1);
+	for (i = 0; i < proc->err_info_num; i++) {
+		printk("%sError info structure %d:\n", pfx, i);
+		printk("%sversion:%d\n", newpfx, err_info->version);
+		printk("%slength:%d\n", newpfx, err_info->length);
+		if (err_info->validation_bits &
+		    CPER_ARMV8_INFO_VALID_MULTI_ERR) {
+			if (err_info->multiple_error == 0)
+				printk("%ssingle error.\n", newpfx);
+			else if (err_info->multiple_error == 1)
+				printk("%smultiple errors.\n", newpfx);
+			else
+				printk("%smultiple errors count:%d.\n",
+				newpfx, err_info->multiple_error);
+		}
+		if (err_info->validation_bits & CPER_ARMV8_INFO_VALID_FLAGS) {
+			if (err_info->flags & CPER_ARMV8_INFO_FLAGS_FIRST)
+				printk("%sfirst error captured.\n", newpfx);
+			if (err_info->flags & CPER_ARMV8_INFO_FLAGS_LAST)
+				printk("%slast error captured.\n", newpfx);
+			if (err_info->flags & CPER_ARMV8_INFO_FLAGS_PROPAGATED)
+				printk("%spropagated error captured.\n",
+				       newpfx);
+		}
+		printk("%serror_type: %d, %s\n", newpfx, err_info->type,
+			err_info->type < ARRAY_SIZE(proc_error_type_strs) ?
+			proc_error_type_strs[err_info->type] : "unknown");
+		printk("%serror_info: 0x%016llx\n", newpfx,
+		       err_info->error_info);
+		if (err_info->validation_bits & CPER_ARMV8_INFO_VALID_VIRT_ADDR)
+			printk("%svirtual fault address: 0x%016llx\n",
+				newpfx, err_info->virt_fault_addr);
+		if (err_info->validation_bits &
+		    CPER_ARMV8_INFO_VALID_PHYSICAL_ADDR)
+			printk("%sphysical fault address: 0x%016llx\n",
+				newpfx, err_info->physical_fault_addr);
+		err_info += 1;
+	}
+
+	if (len < sizeof(*qword) && proc->context_info_num > 0) {
+		printk("%ssection length is too small.\n", pfx);
+		printk("%sCTX_INFO_NUM is %d.\n", pfx, proc->context_info_num);
+		return;
+	}
+	for (i = 0; i < proc->context_info_num; i++) {
+		qword = (__u64 *)err_info;
+		printk("%sProcessor context info structure %d:\n", pfx, i);
+		printk("%sException level %d.\n", newpfx,
+		       (int)((*qword & CPER_ARMV8_CTX_EL_MASK)
+				>> CPER_ARMV8_CTX_EL_SHIFT));
+		printk("%sSecure bit: %d.\n", newpfx,
+		       (int)((*qword & CPER_ARMV8_CTX_NS_MASK)
+				>> CPER_ARMV8_CTX_NS_SHIFT));
+		if ((*qword & CPER_ARMV8_CTX_TYPE_MASK) == 0) {
+			if (len < CPER_AARCH32_CTX_LEN) {
+				printk("%ssection length is too small.\n", pfx);
+				printk("%sremaining length is %d.\n", pfx, len);
+				return;
+			}
+			printk("%sAArch32 execution context.\n", newpfx);
+			qword++;
+			print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4,
+				qword, CPER_AARCH32_CTX_LEN - sizeof(*qword),
+				0);
+			len -= CPER_AARCH32_CTX_LEN;
+		} else if ((*qword & CPER_ARMV8_CTX_TYPE_MASK) == 1) {
+			if (len < CPER_AARCH64_CTX_LEN) {
+				printk("%ssection length is too small.\n", pfx);
+				printk("%sremaining length is %d.\n", pfx, len);
+				return;
+			}
+			printk("%sAArch64 execution context.\n", newpfx);
+			qword++;
+			print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4,
+				qword, CPER_AARCH64_CTX_LEN - sizeof(*qword),
+				0);
+			len -= CPER_AARCH64_CTX_LEN;
+		} else {
+			printk("%scontext type is incorrect 0x%016llx.\n",
+			pfx, *qword);
+			return;
+		}
+	}
+
+	if (len > 0) {
+		printk("%sVendor specific error info has %d bytes.\n", pfx,
+		       len);
+		print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, qword, len,
+			0);
+	}
+}
+
 static const char * const mem_err_type_strs[] = {
 	"unknown",
 	"no error",
@@ -470,6 +596,15 @@ static void cper_estatus_print_section(
 			cper_print_pcie(newpfx, pcie, gdata);
 		else
 			goto err_section_too_small;
+	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_ARMV8)) {
+		struct cper_sec_proc_armv8 *armv8_err;
+
+		armv8_err = acpi_hest_generic_data_payload(gdata);
+		printk("%ssection_type: ARMv8 processor error\n", newpfx);
+		if (gdata->error_data_length >= sizeof(*armv8_err))
+			cper_print_proc_armv8(newpfx, armv8_err);
+		else
+			goto err_section_too_small;
 	} else
 		printk("%s""section type: unknown, %pUl\n", newpfx, sec_type);
 
diff --git a/include/linux/cper.h b/include/linux/cper.h
index dcacb1a..d1efbef 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -162,6 +162,11 @@ enum {
  * corrective action before the data is consumed
  */
 #define CPER_SEC_LATENT_ERROR			0x0020
+/*
+ * If set, the section contains an error that is propagated. The error
+ * did not originate from the hardware associated with this section.
+ */
+#define CPER_SEC_PROPAGATED			0x0040
 
 /*
  * Section type definitions, used in section_type field in struct
@@ -180,6 +185,10 @@ enum {
 #define CPER_SEC_PROC_IPF						\
 	UUID_LE(0xE429FAF1, 0x3CB7, 0x11D4, 0x0B, 0xCA, 0x07, 0x00,	\
 		0x80, 0xC7, 0x3C, 0x88, 0x81)
+/* Processor Specific: ARMv8 */
+#define CPER_SEC_PROC_ARMV8						\
+	UUID_LE(0xE19E3D16, 0xBC11, 0x11E4, 0x9C, 0xAA, 0xC2, 0x05,	\
+		0x1D, 0x5D, 0x46, 0xB0)
 /* Platform Memory */
 #define CPER_SEC_PLATFORM_MEM						\
 	UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83,	\
@@ -255,6 +264,34 @@ enum {
 
 #define CPER_PCIE_SLOT_SHIFT			3
 
+#define CPER_ARMV8_ERR_INFO_NUM_MASK		0x00000000000000FF
+#define CPER_ARMV8_CTX_INFO_NUM_MASK		0x0000000000FFFF00
+#define CPER_ARMV8_CTX_INFO_NUM_SHIFT		8
+
+#define CPER_ARMV8_VALID_MPIDR			0x00000001
+#define CPER_ARMV8_VALID_AFFINITY_LEVEL		0x00000002
+#define CPER_ARMV8_VALID_RUNNING_STATE		0x00000004
+#define CPER_ARMV8_VALID_VENDOR_INFO		0x00000008
+
+#define CPER_ARMV8_INFO_VALID_MULTI_ERR		0x0001
+#define CPER_ARMV8_INFO_VALID_FLAGS		0x0002
+#define CPER_ARMV8_INFO_VALID_ERR_INFO		0x0004
+#define CPER_ARMV8_INFO_VALID_VIRT_ADDR		0x0008
+#define CPER_ARMV8_INFO_VALID_PHYSICAL_ADDR	0x0010
+
+#define CPER_ARMV8_INFO_FLAGS_FIRST		0x0001
+#define CPER_ARMV8_INFO_FLAGS_LAST		0x0002
+#define CPER_ARMV8_INFO_FLAGS_PROPAGATED	0x0004
+
+#define CPER_AARCH64_CTX_LEN			368
+#define CPER_AARCH32_CTX_LEN			256
+
+#define CPER_ARMV8_CTX_TYPE_MASK		0x000000000000000F
+#define CPER_ARMV8_CTX_EL_MASK			0x0000000000000070
+#define CPER_ARMV8_CTX_NS_MASK			0x0000000000000080
+#define CPER_ARMV8_CTX_EL_SHIFT			4
+#define CPER_ARMV8_CTX_NS_SHIFT			7
+
 /*
  * All tables and structs must be byte-packed to match CPER
  * specification, since the tables are provided by the system BIOS
@@ -340,6 +377,41 @@ struct cper_ia_proc_ctx {
 	__u64	mm_reg_addr;
 };
 
+/* ARMv8 Processor Error Section */
+struct cper_sec_proc_armv8 {
+	__u32	validation_bits;
+	__u16	err_info_num; /* Number of Processor Error Info */
+	__u16	context_info_num; /* Number of Processor Context Info Records*/
+	__u32	section_length;
+	__u8	affinity_level;
+	__u8	reserved[3];	/* must be zero */
+	__u64	mpidr;
+	__u64	midr;
+	__u32	running_state; /* Bit 0 set - Processor running. PSCI = 0 */
+	__u32	psci_state;
+};
+
+/* ARMv8 Processor Error Information Structure */
+struct cper_armv8_err_info {
+	__u8	version;
+	__u8	length;
+	__u16	validation_bits;
+	__u8	type;
+	__u16	multiple_error;
+	__u8	flags;
+	__u64	error_info;
+	__u64	virt_fault_addr;
+	__u64	physical_fault_addr;
+};
+
+/* ARMv8 AARCH64 Processor Context Information Structure */
+struct cper_armv8_aarch64_ctx {
+	__u8	type_el_ns;
+	__u8	reserved[7];	/* must be zero */
+	__u8	gpr[288];
+	__u8	spr[68];
+};
+
 /* Old Memory Error Section UEFI 2.1, 2.2 */
 struct cper_sec_mem_err_old {
 	__u64	validation_bits;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 04/10] arm64: exception: handle Synchronous External Abort

[Tyler Baicar]

SEA exceptions are often caused by an uncorrected hardware
error, and are handled when data abort and instruction abort
exception classes have specific values for their Fault Status
Code.
When SEA occurs, before killing the process, go through
the handlers registered in the notification list.
Update fault_info[] with specific SEA faults so that the
new SEA handler is used.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
---
 arch/arm64/include/asm/system_misc.h | 13 ++++++++
 arch/arm64/mm/fault.c                | 58 +++++++++++++++++++++++++++++-------
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index 57f110b..90daf4a 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -64,4 +64,17 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 
 #endif	/* __ASSEMBLY__ */
 
+/*
+ * The functions below are used to register and unregister callbacks
+ * that are to be invoked when a Synchronous External Abort (SEA)
+ * occurs. An SEA is raised by certain fault status codes that have
+ * either data or instruction abort as the exception class, and
+ * callbacks may be registered to parse or handle such hardware errors.
+ *
+ * Registered callbacks are run in an interrupt/atomic context. They
+ * are not allowed to block or sleep.
+ */
+int sea_register_handler_chain(struct notifier_block *nb);
+void sea_unregister_handler_chain(struct notifier_block *nb);
+
 #endif	/* __ASM_SYSTEM_MISC_H */
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 05d2bd7..81cb7ad 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -39,6 +39,22 @@
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
 
+/*
+ * GHES SEA handler code may register a notifier call here to
+ * handle HW error record passed from platform.
+ */
+static ATOMIC_NOTIFIER_HEAD(sea_handler_chain);
+
+int sea_register_handler_chain(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&sea_handler_chain, nb);
+}
+
+void sea_unregister_handler_chain(struct notifier_block *nb)
+{
+	atomic_notifier_chain_unregister(&sea_handler_chain, nb);
+}
+
 static const char *fault_name(unsigned int esr);
 
 #ifdef CONFIG_KPROBES
@@ -480,6 +496,28 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 	return 1;
 }
 
+/*
+ * This abort handler deals with Synchronous External Abort.
+ * It calls notifiers, and then returns "fault".
+ */
+static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
+{
+	struct siginfo info;
+
+	atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
+
+	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
+		 fault_name(esr), esr, addr);
+
+	info.si_signo = SIGBUS;
+	info.si_errno = 0;
+	info.si_code  = 0;
+	info.si_addr  = (void __user *)addr;
+	arm64_notify_die("", regs, &info, esr);
+
+	return 0;
+}
+
 static const struct fault_info {
 	int	(*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
 	int	sig;
@@ -502,22 +540,22 @@ static const struct fault_info {
 	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 1 permission fault"	},
 	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 2 permission fault"	},
 	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 3 permission fault"	},
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort"	},
+	{ do_sea,		SIGBUS,  0,		"synchronous external abort"	},
 	{ do_bad,		SIGBUS,  0,		"unknown 17"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 18"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 19"			},
-	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error"	},
+	{ do_sea,		SIGBUS,  0,		"level 0 SEA (trans tbl walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 1 SEA (trans tbl walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 2 SEA (trans tbl walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 3 SEA (trans tbl walk)"	},
+	{ do_sea,		SIGBUS,  0,		"synchronous parity or ECC err" },
 	{ do_bad,		SIGBUS,  0,		"unknown 25"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 26"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 27"			},
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error (translation table walk)" },
+	{ do_sea,		SIGBUS,  0,		"level 0 synch parity error"	},
+	{ do_sea,		SIGBUS,  0,		"level 1 synch parity error"	},
+	{ do_sea,		SIGBUS,  0,		"level 2 synch parity error"	},
+	{ do_sea,		SIGBUS,  0,		"level 3 synch parity error"	},
 	{ do_bad,		SIGBUS,  0,		"unknown 32"			},
 	{ do_alignment_fault,	SIGBUS,  BUS_ADRALN,	"alignment fault"		},
 	{ do_bad,		SIGBUS,  0,		"unknown 34"			},
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Tyler Baicar]

ARM APEI extension proposal added SEA (Synchrounous External
Abort) notification type for ARMv8.
Add a new GHES error source handling function for SEA. If an error
source's notification type is SEA, then this function can be registered
into the SEA exception handler. That way GHES will parse and report
SEA exceptions when they occur.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Tyler Baicar <tbaicar-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Naveen Kaje <nkaje-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 arch/arm64/Kconfig        |  1 +
 drivers/acpi/apei/Kconfig | 15 +++++++++
 drivers/acpi/apei/ghes.c  | 83 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b380c87..ae34349 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -53,6 +53,7 @@ config ARM64
 	select HANDLE_DOMAIN_IRQ
 	select HARDIRQS_SW_RESEND
 	select HAVE_ACPI_APEI if (ACPI && EFI)
+	select HAVE_ACPI_APEI_SEA if (ACPI && EFI)
 	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_BITREVERSE
diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
index b0140c8..fb99c1c 100644
--- a/drivers/acpi/apei/Kconfig
+++ b/drivers/acpi/apei/Kconfig
@@ -4,6 +4,21 @@ config HAVE_ACPI_APEI
 config HAVE_ACPI_APEI_NMI
 	bool
 
+config HAVE_ACPI_APEI_SEA
+	bool "APEI Synchronous External Abort logging/recovering support"
+	depends on ARM64
+	help
+	  This option should be enabled if the system supports
+	  firmware first handling of SEA (Synchronous External Abort).
+	  SEA happens with certain faults of data abort or instruction
+	  abort synchronous exceptions on ARMv8 systems. If a system
+	  supports firmware first handling of SEA, the platform analyzes
+	  and handles hardware error notifications with SEA, and it may then
+	  form a HW error record for the OS to parse and handle. This
+	  option allows the OS to look for such HW error record, and
+	  take appropriate action.
+
 config ACPI_APEI
 	bool "ACPI Platform Error Interface (APEI)"
 	select MISC_FILESYSTEMS
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index c8488f1..28d5a09 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -50,6 +50,10 @@
 #include <acpi/apei.h>
 #include <asm/tlbflush.h>
 
+#ifdef CONFIG_HAVE_ACPI_APEI_SEA
+#include <asm/system_misc.h>
+#endif
+
 #include "apei-internal.h"
 
 #define GHES_PFX	"GHES: "
@@ -779,6 +783,62 @@ static struct notifier_block ghes_notifier_sci = {
 	.notifier_call = ghes_notify_sci,
 };
 
+#ifdef CONFIG_HAVE_ACPI_APEI_SEA
+static LIST_HEAD(ghes_sea);
+
+static int ghes_notify_sea(struct notifier_block *this,
+				  unsigned long event, void *data)
+{
+	struct ghes *ghes;
+	int ret = NOTIFY_DONE;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
+		if (!ghes_proc(ghes))
+			ret = NOTIFY_OK;
+	}
+	rcu_read_unlock();
+
+	return ret;
+}
+
+static struct notifier_block ghes_notifier_sea = {
+	.notifier_call = ghes_notify_sea,
+};
+
+static int ghes_sea_add(struct ghes *ghes)
+{
+	mutex_lock(&ghes_list_mutex);
+	if (list_empty(&ghes_sea))
+		sea_register_handler_chain(&ghes_notifier_sea);
+	list_add_rcu(&ghes->list, &ghes_sea);
+	mutex_unlock(&ghes_list_mutex);
+	return 0;
+}
+
+static void ghes_sea_remove(struct ghes *ghes)
+{
+	mutex_lock(&ghes_list_mutex);
+	list_del_rcu(&ghes->list);
+	if (list_empty(&ghes_sea))
+		sea_unregister_handler_chain(&ghes_notifier_sea);
+	mutex_unlock(&ghes_list_mutex);
+}
+#else /* CONFIG_HAVE_ACPI_APEI_SEA */
+static inline int ghes_sea_add(struct ghes *ghes)
+{
+	pr_err(GHES_PFX "ID: %d, trying to add SEA notification which is not supported\n",
+	       ghes->generic->header.source_id);
+	return -ENOTSUPP;
+}
+
+static inline void ghes_sea_remove(struct ghes *ghes)
+{
+	pr_err(GHES_PFX "ID: %d, trying to remove SEA notification which is not supported\n",
+	       ghes->generic->header.source_id);
+}
+#endif /* CONFIG_HAVE_ACPI_APEI_SEA */
+
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
 /*
  * printk is not safe in NMI context.  So in NMI handler, we allocate
@@ -1023,6 +1083,14 @@ static int ghes_probe(struct platform_device *ghes_dev)
 	case ACPI_HEST_NOTIFY_EXTERNAL:
 	case ACPI_HEST_NOTIFY_SCI:
 		break;
+	case ACPI_HEST_NOTIFY_SEA:
+		if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_SEA)) {
+			pr_warn(GHES_PFX "Generic hardware error source: %d notified via SEA is not supported\n",
+				generic->header.source_id);
+			rc = -ENOTSUPP;
+			goto err;
+		}
+		break;
 	case ACPI_HEST_NOTIFY_NMI:
 		if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_NMI)) {
 			pr_warn(GHES_PFX "Generic hardware error source: %d notified via NMI interrupt is not supported!\n",
@@ -1034,6 +1102,13 @@ static int ghes_probe(struct platform_device *ghes_dev)
 		pr_warning(GHES_PFX "Generic hardware error source: %d notified via local interrupt is not supported!\n",
 			   generic->header.source_id);
 		goto err;
+	case ACPI_HEST_NOTIFY_GPIO:
+	case ACPI_HEST_NOTIFY_SEI:
+	case ACPI_HEST_NOTIFY_GSIV:
+		pr_warn(GHES_PFX "Generic hardware error source: %d notified via notification type %u is not supported\n",
+			generic->header.source_id, generic->header.source_id);
+		rc = -ENOTSUPP;
+		goto err;
 	default:
 		pr_warning(FW_WARN GHES_PFX "Unknown notification type: %u for generic hardware error source: %d\n",
 			   generic->notify.type, generic->header.source_id);
@@ -1088,6 +1163,11 @@ static int ghes_probe(struct platform_device *ghes_dev)
 		list_add_rcu(&ghes->list, &ghes_sci);
 		mutex_unlock(&ghes_list_mutex);
 		break;
+	case ACPI_HEST_NOTIFY_SEA:
+		rc = ghes_sea_add(ghes);
+		if (rc)
+			goto err_edac_unreg;
+		break;
 	case ACPI_HEST_NOTIFY_NMI:
 		ghes_nmi_add(ghes);
 		break;
@@ -1130,6 +1210,9 @@ static int ghes_remove(struct platform_device *ghes_dev)
 			unregister_acpi_hed_notifier(&ghes_notifier_sci);
 		mutex_unlock(&ghes_list_mutex);
 		break;
+	case ACPI_HEST_NOTIFY_SEA:
+		ghes_sea_remove(ghes);
+		break;
 	case ACPI_HEST_NOTIFY_NMI:
 		ghes_nmi_remove(ghes);
 		break;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 06/10] acpi: apei: panic OS with fatal error status block

[Tyler Baicar]

From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>

Even if an error status block's severity is fatal, the kernel does not
honor the severity level and panic.

With the firmware first model, the platform could inform the OS about a
fatal hardware error through the non-NMI GHES notification type. The OS
should panic when a hardware error record is received with this
severity.

Call panic() after CPER data in error status block is printed if
severity is fatal, before each error section is handled.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 drivers/acpi/apei/ghes.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 28d5a09..36894c8 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -141,6 +141,8 @@ static unsigned long ghes_estatus_pool_size_request;
 static struct ghes_estatus_cache *ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
 static atomic_t ghes_estatus_cache_alloced;
 
+static int ghes_panic_timeout __read_mostly = 30;
+
 static int ghes_ioremap_init(void)
 {
 	ghes_ioremap_area = __get_vm_area(PAGE_SIZE * GHES_IOREMAP_PAGES,
@@ -715,6 +717,12 @@ static int ghes_proc(struct ghes *ghes)
 		if (ghes_print_estatus(NULL, ghes->generic, ghes->estatus))
 			ghes_estatus_cache_add(ghes->generic, ghes->estatus);
 	}
+	if (ghes_severity(ghes->estatus->error_severity) >= GHES_SEV_PANIC) {
+		if (panic_timeout == 0)
+			panic_timeout = ghes_panic_timeout;
+		panic("Fatal hardware error!");
+	}
+
 	ghes_do_proc(ghes, ghes->estatus);
 
 	if (ghes->generic_v2) {
@@ -859,8 +867,6 @@ static atomic_t ghes_in_nmi = ATOMIC_INIT(0);
 
 static LIST_HEAD(ghes_nmi);
 
-static int ghes_panic_timeout	__read_mostly = 30;
-
 static void ghes_proc_in_irq(struct irq_work *irq_work)
 {
 	struct llist_node *llnode, *next;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 07/10] efi: print unrecognized CPER section

[Tyler Baicar]

UEFI spec allows for non-standard section in Common Platform Error
Record. This is defined in section N.2.3 of UEFI version 2.5.

Currently if the CPER section's type (UUID) does not match with
one of the section types that the kernel knows how to parse, the
section is skipped. Therefore, user is not able to see
such CPER data, for instance, error record of non-standard section.

For above mentioned case, this change prints out the raw data in
hex in dmesg buffer. Data length is taken from Error Data length
field of Generic Error Data Entry.

Following is a sample output from dmesg:
[  115.771702] {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 2
[  115.779042] {1}[Hardware Error]: It has been corrected by h/w and requires no further action
[  115.787456] {1}[Hardware Error]: event severity: corrected
[  115.792927] {1}[Hardware Error]:  Error 0, type: corrected
[  115.798415] {1}[Hardware Error]:  fru_id: 00000000-0000-0000-0000-000000000000
[  115.805596] {1}[Hardware Error]:  fru_text:
[  115.816105] {1}[Hardware Error]:  section type: d2e2621c-f936-468d-0d84-15a4ed015c8b
[  115.823880] {1}[Hardware Error]:  section length: 88
[  115.828779] {1}[Hardware Error]:   00000000: 01000001 00000002 5f434345 525f4543
[  115.836153] {1}[Hardware Error]:   00000010: 0000574d 00000000 00000000 00000000
[  115.843531] {1}[Hardware Error]:   00000020: 00000000 00000000 00000000 00000000
[  115.850908] {1}[Hardware Error]:   00000030: 00000000 00000000 00000000 00000000
[  115.858288] {1}[Hardware Error]:   00000040: fe800000 00000000 00000004 5f434345
[  115.865665] {1}[Hardware Error]:   00000050: 525f4543 0000574d

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Tyler Baicar <tbaicar-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/firmware/efi/cper.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 2594245..f9ffba6 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -605,8 +605,16 @@ static void cper_estatus_print_section(
 			cper_print_proc_armv8(newpfx, armv8_err);
 		else
 			goto err_section_too_small;
-	} else
-		printk("%s""section type: unknown, %pUl\n", newpfx, sec_type);
+	} else {
+		const void *unknown_err;
+
+		unknown_err = acpi_hest_generic_data_payload(gdata);
+		printk("%ssection type: %pUl\n", newpfx, sec_type);
+		printk("%ssection length: %d\n", newpfx,
+		       gdata->error_data_length);
+		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4,
+			       unknown_err, gdata->error_data_length, 0);
+	}
 
 	return;
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section

[Tyler Baicar]

UEFI spec allows for non-standard section in Common Platform Error
Record. This is defined in section N.2.3 of UEFI version 2.5.

Currently if the CPER section's type (UUID) does not match with
any section type that the kernel knows how to parse, trace event
is not generated for such section. And thus user is not able to know
happening of such hardware error, including error record of
non-standard section.

This commit generates a trace event which contains raw error data
for unrecognized CPER section.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
---
 drivers/acpi/apei/ghes.c | 18 +++++++++++++++++-
 drivers/ras/ras.c        |  1 +
 include/ras/ras_event.h  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 36894c8..cb4c7f4 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -49,6 +49,7 @@
 #include <acpi/ghes.h>
 #include <acpi/apei.h>
 #include <asm/tlbflush.h>
+#include <ras/ras_event.h>
 
 #ifdef CONFIG_HAVE_ACPI_APEI_SEA
 #include <asm/system_misc.h>
@@ -468,12 +469,21 @@ static void ghes_do_proc(struct ghes *ghes,
 	int sev, sec_sev;
 	struct acpi_hest_generic_data *gdata;
 	uuid_le sec_type;
+	uuid_le *fru_id;
+	char *fru_text = "";
 
 	sev = ghes_severity(estatus->error_severity);
 	apei_estatus_for_each_section(estatus, gdata) {
 		sec_sev = ghes_severity(gdata->error_severity);
 		sec_type = *(uuid_le *)gdata->section_type;
 
+		if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
+			fru_id = (uuid_le *)gdata->fru_id;
+		else
+			fru_id = &NULL_UUID_LE;
+		if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
+			fru_text = gdata->fru_text;
+
 		if (!uuid_le_cmp(sec_type,
 				 CPER_SEC_PLATFORM_MEM)) {
 			struct cper_sec_mem_err *mem_err;
@@ -485,7 +495,7 @@ static void ghes_do_proc(struct ghes *ghes,
 			ghes_handle_memory_failure(gdata, sev);
 		}
 #ifdef CONFIG_ACPI_APEI_PCIEAER
-		else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
+		else if (!uuid_le_cmp(sec_type,
 				      CPER_SEC_PCIE)) {
 			struct cper_sec_pcie *pcie_err;
 
@@ -518,6 +528,12 @@ static void ghes_do_proc(struct ghes *ghes,
 
 		}
 #endif
+		else {
+			void *unknown_err = acpi_hest_generic_data_payload(gdata);
+			trace_unknown_sec_event(&sec_type,
+					fru_id, fru_text, sec_sev,
+					unknown_err, gdata->error_data_length);
+		}
 	}
 }
 
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index b67dd36..fb2500b 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -27,3 +27,4 @@ subsys_initcall(ras_init);
 EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
 #endif
 EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
+EXPORT_TRACEPOINT_SYMBOL_GPL(unknown_sec_event);
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 1791a12..5861b6f 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -162,6 +162,51 @@ TRACE_EVENT(mc_event,
 );
 
 /*
+ * Unknown Section Report
+ *
+ * This event is generated when hardware detected a hardware
+ * error event, which may be of non-standard section as defined
+ * in UEFI spec appendix "Common Platform Error Record", or may
+ * be of sections for which TRACE_EVENT is not defined.
+ *
+ */
+TRACE_EVENT(unknown_sec_event,
+
+	TP_PROTO(const uuid_le *sec_type,
+		 const uuid_le *fru_id,
+		 const char *fru_text,
+		 const u8 sev,
+		 const u8 *err,
+		 const u32 len),
+
+	TP_ARGS(sec_type, fru_id, fru_text, sev, err, len),
+
+	TP_STRUCT__entry(
+		__array(char, sec_type, 16)
+		__array(char, fru_id, 16)
+		__string(fru_text, fru_text)
+		__field(u8, sev)
+		__field(u32, len)
+		__dynamic_array(u8, buf, len)
+	),
+
+	TP_fast_assign(
+		memcpy(__entry->sec_type, sec_type, sizeof(uuid_le));
+		memcpy(__entry->fru_id, fru_id, sizeof(uuid_le));
+		__assign_str(fru_text, fru_text);
+		__entry->sev = sev;
+		__entry->len = len;
+		memcpy(__get_dynamic_array(buf), err, len);
+	),
+
+	TP_printk("severity: %d; sec type:%pU; FRU: %pU %s; data len:%d; raw data:%s",
+		  __entry->sev, __entry->sec_type,
+		  __entry->fru_id, __get_str(fru_text),
+		  __entry->len,
+		  __print_hex(__get_dynamic_array(buf), __entry->len))
+);
+
+/*
  * PCIe AER Trace event
  *
  * These events are generated when hardware detects a corrected or
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 09/10] trace, ras: add ARM processor error trace event

[Tyler Baicar]

Currently there are trace events for the various RAS
errors with the exception of ARM processor type errors.
Add a new trace event for such errors so that the user
will know when they occur. These trace events are
consistent with the ARM processor error section type
defined in UEFI 2.6 spec section N.2.4.4.

Signed-off-by: Tyler Baicar <tbaicar-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/firmware/efi/cper.c |  9 ++++++
 drivers/ras/ras.c           |  1 +
 include/ras/ras_event.h     | 67 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index f9ffba6..21b8a6f 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -34,6 +34,7 @@
 #include <linux/aer.h>
 #include <linux/printk.h>
 #include <linux/bcd.h>
+#include <ras/ras_event.h>
 
 #define INDENT_SP	" "
 
@@ -256,6 +257,14 @@ static void cper_print_proc_armv8(const char *pfx,
 		    CPER_ARMV8_INFO_VALID_PHYSICAL_ADDR)
 			printk("%sphysical fault address: 0x%016llx\n",
 				newpfx, err_info->physical_fault_addr);
+		trace_arm_event(proc->affinity_level, proc->mpidr, proc->midr,
+				proc->running_state, proc->psci_state,
+				err_info->version, err_info->type,
+				err_info->multiple_error,
+				err_info->validation_bits,
+				err_info->error_info,
+				err_info->virt_fault_addr,
+				err_info->physical_fault_addr);
 		err_info += 1;
 	}
 
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index fb2500b..8ba5a94 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -28,3 +28,4 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
 #endif
 EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
 EXPORT_TRACEPOINT_SYMBOL_GPL(unknown_sec_event);
+EXPORT_TRACEPOINT_SYMBOL_GPL(arm_event);
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 5861b6f..eb2719a 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -162,6 +162,73 @@ TRACE_EVENT(mc_event,
 );
 
 /*
+ * ARM Processor Events Report
+ *
+ * This event is generated when hardware detects an ARM processor error
+ * has occurred. UEFI 2.6 spec section N.2.4.4.
+ */
+TRACE_EVENT(arm_event,
+
+	TP_PROTO(const u8 affinity,
+		 const u64 mpidr,
+		 const u64 midr,
+		 const u32 running_state,
+		 const u32 psci_state,
+		 const u8 version,
+		 const u8 type,
+		 const u16 err_count,
+		 const u8 flags,
+		 const u64 info,
+		 const u64 virt_fault_addr,
+		 const u64 phys_fault_addr),
+
+	TP_ARGS(affinity, mpidr, midr, running_state, psci_state,
+		version, type, err_count, flags, info, virt_fault_addr,
+		phys_fault_addr),
+
+	TP_STRUCT__entry(
+		__field(u8, affinity)
+		__field(u64, mpidr)
+		__field(u64, midr)
+		__field(u32, running_state)
+		__field(u32, psci_state)
+		__field(u8, version)
+		__field(u8, type)
+		__field(u16, err_count)
+		__field(u8, flags)
+		__field(u64, info)
+		__field(u64, virt_fault_addr)
+		__field(u64, phys_fault_addr)
+	),
+
+	TP_fast_assign(
+		__entry->affinity = affinity;
+		__entry->mpidr = mpidr;
+		__entry->midr = midr;
+		__entry->running_state = running_state;
+		__entry->psci_state = psci_state;
+		__entry->version = version;
+		__entry->type = type;
+		__entry->err_count = err_count;
+		__entry->flags = flags;
+		__entry->info = info;
+		__entry->virt_fault_addr = virt_fault_addr;
+		__entry->phys_fault_addr = phys_fault_addr;
+	),
+
+	TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
+		  "running state: %d; PSCI state: %d; version: %d; type: %d; "
+		  "error count: 0x%04x; flags: 0x%02x; info: %016llx; "
+		  "virtual fault address: %016llx; "
+		  "physical fault address: %016llx",
+		  __entry->affinity, __entry->mpidr, __entry->midr,
+		  __entry->running_state, __entry->psci_state, __entry->version,
+		  __entry->type, __entry->err_count, __entry->flags,
+		  __entry->info, __entry->virt_fault_addr,
+		  __entry->phys_fault_addr)
+);
+
+/*
  * Unknown Section Report
  *
  * This event is generated when hardware detected a hardware
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH V3 10/10] arm64: KVM: add guest SEA support

[Tyler Baicar]

Currently external aborts are unsupported by the guest abort
handling. Add handling for SEAs so that the host kernel reports
SEAs which occur in the guest kernel.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
---
 arch/arm/include/asm/kvm_arm.h       |  1 +
 arch/arm/include/asm/system_misc.h   |  5 +++++
 arch/arm/kvm/mmu.c                   | 15 +++++++++++++--
 arch/arm64/include/asm/kvm_arm.h     |  1 +
 arch/arm64/include/asm/system_misc.h |  2 ++
 arch/arm64/mm/fault.c                | 13 +++++++++++++
 6 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
index e22089f..33a77509 100644
--- a/arch/arm/include/asm/kvm_arm.h
+++ b/arch/arm/include/asm/kvm_arm.h
@@ -187,6 +187,7 @@
 #define FSC_FAULT	(0x04)
 #define FSC_ACCESS	(0x08)
 #define FSC_PERM	(0x0c)
+#define FSC_EXTABT	(0x10)
 
 /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
 #define HPFAR_MASK	(~0xf)
diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
index a3d61ad..86e1faa 100644
--- a/arch/arm/include/asm/system_misc.h
+++ b/arch/arm/include/asm/system_misc.h
@@ -24,4 +24,9 @@ extern unsigned int user_debug;
 
 #endif /* !__ASSEMBLY__ */
 
+inline int handle_guest_sea(unsigned long addr, unsigned int esr)
+{
+	return -1;
+}
+
 #endif /* __ASM_ARM_SYSTEM_MISC_H */
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index e9a5c0e..24cde84 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -29,6 +29,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_emulate.h>
 #include <asm/virt.h>
+#include <asm/system_misc.h>
 
 #include "trace.h"
 
@@ -1441,8 +1442,18 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 	/* Check the stage-2 fault is trans. fault or write fault */
 	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
-	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
-	    fault_status != FSC_ACCESS) {
+
+	if (fault_status == FSC_EXTABT) {
+		if(handle_guest_sea((unsigned long)fault_ipa,
+				    kvm_vcpu_get_hsr(vcpu))) {
+			kvm_err("Failed to handle guest SEA, FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
+				kvm_vcpu_trap_get_class(vcpu),
+				(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
+				(unsigned long)kvm_vcpu_get_hsr(vcpu));
+			return -EFAULT;
+		}
+	} else if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
+		   fault_status != FSC_ACCESS) {
 		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
 			kvm_vcpu_trap_get_class(vcpu),
 			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 4b5c977..be0efb6 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -201,6 +201,7 @@
 #define FSC_FAULT	ESR_ELx_FSC_FAULT
 #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
 #define FSC_PERM	ESR_ELx_FSC_PERM
+#define FSC_EXTABT	ESR_ELx_FSC_EXTABT
 
 /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
 #define HPFAR_MASK	(~UL(0xf))
diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index 90daf4a..8522fff 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -77,4 +77,6 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 int sea_register_handler_chain(struct notifier_block *nb);
 void sea_unregister_handler_chain(struct notifier_block *nb);
 
+int handle_guest_sea(unsigned long addr, unsigned int esr);
+
 #endif	/* __ASM_SYSTEM_MISC_H */
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 81cb7ad..d714432 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -597,6 +597,19 @@ static const char *fault_name(unsigned int esr)
 }
 
 /*
+ * Handle Synchronous External Aborts that occur in a guest kernel.
+ */
+int handle_guest_sea(unsigned long addr, unsigned int esr)
+{
+	atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
+
+	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
+		fault_name(esr), esr, addr);
+
+	return 0;
+}
+
+/*
  * Dispatch a data abort to the relevant handler.
  */
 asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 09/10] trace, ras: add ARM processor error trace event

[Steven Rostedt]

On Fri,  7 Oct 2016 15:31:21 -0600
Tyler Baicar <tbaicar@codeaurora.org> wrote:

> Currently there are trace events for the various RAS
> errors with the exception of ARM processor type errors.
> Add a new trace event for such errors so that the user
> will know when they occur. These trace events are
> consistent with the ARM processor error section type
> defined in UEFI 2.6 spec section N.2.4.4.
> 
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> ---
>  drivers/firmware/efi/cper.c |  9 ++++++
>  drivers/ras/ras.c           |  1 +
>  include/ras/ras_event.h     | 67 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index f9ffba6..21b8a6f 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -34,6 +34,7 @@
>  #include <linux/aer.h>
>  #include <linux/printk.h>
>  #include <linux/bcd.h>
> +#include <ras/ras_event.h>
>  
>  #define INDENT_SP	" "
>  
> @@ -256,6 +257,14 @@ static void cper_print_proc_armv8(const char *pfx,
>  		    CPER_ARMV8_INFO_VALID_PHYSICAL_ADDR)
>  			printk("%sphysical fault address: 0x%016llx\n",
>  				newpfx, err_info->physical_fault_addr);
> +		trace_arm_event(proc->affinity_level, proc->mpidr, proc->midr,
> +				proc->running_state, proc->psci_state,
> +				err_info->version, err_info->type,
> +				err_info->multiple_error,
> +				err_info->validation_bits,
> +				err_info->error_info,
> +				err_info->virt_fault_addr,
> +				err_info->physical_fault_addr);

Why waste all the effort into passing each individual field. Why not
just pass the structure in and sort it out in the TP_fast_assign()?

>  		err_info += 1;
>  	}
>  
> diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
> index fb2500b..8ba5a94 100644
> --- a/drivers/ras/ras.c
> +++ b/drivers/ras/ras.c
> @@ -28,3 +28,4 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
>  #endif
>  EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(unknown_sec_event);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(arm_event);
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 5861b6f..eb2719a 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -162,6 +162,73 @@ TRACE_EVENT(mc_event,
>  );
>  
>  /*
> + * ARM Processor Events Report
> + *
> + * This event is generated when hardware detects an ARM processor error
> + * has occurred. UEFI 2.6 spec section N.2.4.4.
> + */
> +TRACE_EVENT(arm_event,
> +
> +	TP_PROTO(const u8 affinity,
> +		 const u64 mpidr,
> +		 const u64 midr,
> +		 const u32 running_state,
> +		 const u32 psci_state,
> +		 const u8 version,
> +		 const u8 type,
> +		 const u16 err_count,
> +		 const u8 flags,
> +		 const u64 info,
> +		 const u64 virt_fault_addr,
> +		 const u64 phys_fault_addr),
> +
> +	TP_ARGS(affinity, mpidr, midr, running_state, psci_state,
> +		version, type, err_count, flags, info, virt_fault_addr,
> +		phys_fault_addr),
> +
> +	TP_STRUCT__entry(
> +		__field(u8, affinity)
> +		__field(u64, mpidr)
> +		__field(u64, midr)
> +		__field(u32, running_state)
> +		__field(u32, psci_state)
> +		__field(u8, version)
> +		__field(u8, type)
> +		__field(u16, err_count)
> +		__field(u8, flags)
> +		__field(u64, info)
> +		__field(u64, virt_fault_addr)
> +		__field(u64, phys_fault_addr)

The above creates a structure with lots of holes in it. Pack it better.
You want something like:

	__field(u64, mpidr)
	__field(u64, midr)
	__field(u64, info)
	__field(u64, virt_fault_addr)
	__field(u64, phys_fault_addr)
	__field(u32, running_state)
	__field(u32, psci_state)
	__field(u16, err_count)
	__field(u8, affinity)
	__field(u8, version)
	__field(u8, type)
	__field(u8, flags)

The above is a total of 54 bytes. Your original was at a minimum, 64
bytes.

-- Steve

> +	),
> +
> +	TP_fast_assign(
> +		__entry->affinity = affinity;
> +		__entry->mpidr = mpidr;
> +		__entry->midr = midr;
> +		__entry->running_state = running_state;
> +		__entry->psci_state = psci_state;
> +		__entry->version = version;
> +		__entry->type = type;
> +		__entry->err_count = err_count;
> +		__entry->flags = flags;
> +		__entry->info = info;
> +		__entry->virt_fault_addr = virt_fault_addr;
> +		__entry->phys_fault_addr = phys_fault_addr;
> +	),
> +
> +	TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
> +		  "running state: %d; PSCI state: %d; version: %d; type: %d; "
> +		  "error count: 0x%04x; flags: 0x%02x; info: %016llx; "
> +		  "virtual fault address: %016llx; "
> +		  "physical fault address: %016llx",
> +		  __entry->affinity, __entry->mpidr, __entry->midr,
> +		  __entry->running_state, __entry->psci_state, __entry->version,
> +		  __entry->type, __entry->err_count, __entry->flags,
> +		  __entry->info, __entry->virt_fault_addr,
> +		  __entry->phys_fault_addr)
> +);
> +
> +/*
>   * Unknown Section Report
>   *
>   * This event is generated when hardware detected a hardware

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Suzuki K Poulose]

On 07/10/16 22:31, Tyler Baicar wrote:
> Currently when a RAS error is reported it is not timestamped.
> The ACPI 6.1 spec adds the timestamp field to the generic error
> data entry v3 structure. The timestamp of when the firmware
> generated the error is now being reported.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>

Please could you keep the people who reviewed/commented on your series in the past,
whenever you post a new version ?

> ---
>  drivers/acpi/apei/ghes.c    | 25 ++++++++++--
>  drivers/firmware/efi/cper.c | 97 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 105 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3021f0e..c8488f1 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -80,6 +80,10 @@
>  	((struct acpi_hest_generic_status *)				\
>  	 ((struct ghes_estatus_node *)(estatus_node) + 1))
>
> +#define acpi_hest_generic_data_version(gdata)			\
> +	(gdata->revision >> 8)

...

> +inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
> +{
> +	return acpi_hest_generic_data_version(gdata) >= 3 ?
> +		(void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
> +		gdata + 1;
> +}
> +



> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index d425374..9fa1317 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c

   
> +#define acpi_hest_generic_data_version(gdata)		\
> +	(gdata->revision >> 8)
> +

...

> +static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
> +{
> +	return acpi_hest_generic_data_version(gdata) >= 3 ?
> +		(void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
> +		gdata + 1;
> +}

Could these go to a header file, so that we don't need duplicate definitions of these helpers in
different files ?

> +
> +static void cper_estatus_print_section_v300(const char *pfx,
> +	const struct acpi_hest_generic_data_v300 *gdata)
> +{
> +	__u8 hour, min, sec, day, mon, year, century, *timestamp;
> +
> +	if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
> +		timestamp = (__u8 *)&(gdata->time_stamp);
> +		memcpy(&sec, timestamp, 1);
> +		memcpy(&min, timestamp + 1, 1);
> +		memcpy(&hour, timestamp + 2, 1);
> +		memcpy(&day, timestamp + 4, 1);
> +		memcpy(&mon, timestamp + 5, 1);
> +		memcpy(&year, timestamp + 6, 1);
> +		memcpy(&century, timestamp + 7, 1);
> +		printk("%stime: ", pfx);
> +		printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");

What format is the (timestamp + 3) stored in ? Does it need conversion ?

> +		printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
> +			bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
> +			bcd2bin(century), bcd2bin(year), bcd2bin(mon),
> +			bcd2bin(day));
> +	}

minor nit: Would it be easier to order/parse the error messages if the date
is printed first followed by time ?

i.e,
	17:20:14 2016-09-15 Mon
		vs
	2016-09-15 Mon 17:20:14

e.g, people looking at a huge log, looking for logs from a specific date might
find the latter more useful to skip the messages.

> +}
> +
>  static void cper_estatus_print_section(
> -	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
> +	const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
>  {
>  	uuid_le *sec_type = (uuid_le *)gdata->section_type;
>  	__u16 severity;
>  	char newpfx[64];
>
> +	if ((gdata->revision >> 8) >= 0x03)

Could we use the helper defined above ?

> @@ -451,12 +497,22 @@ void cper_estatus_print(const char *pfx,
>  	printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
>  	data_len = estatus->data_length;
>  	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
> +	if ((gdata->revision >> 8) >= 0x03)

Same as above, use the macro ?

> +		gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> +
>  	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> +
>  	while (data_len >= sizeof(*gdata)) {
>  		gedata_len = gdata->error_data_length;
>  		cper_estatus_print_section(newpfx, gdata, sec_no);
> -		data_len -= gedata_len + sizeof(*gdata);
> -		gdata = (void *)(gdata + 1) + gedata_len;
> +		if(gdata_v3) {
> +			data_len -= gedata_len + sizeof(*gdata_v3);
> +			gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
> +			gdata = (struct acpi_hest_generic_data *)gdata_v3;
> +		} else {
> +			data_len -= gedata_len + sizeof(*gdata);
> +			gdata = (void *)(gdata + 1) + gedata_len;
> +		}
>  		sec_no++;
>  	}

...

>
> @@ -486,15 +543,29 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  		return rc;
>  	data_len = estatus->data_length;
>  	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
> -	while (data_len >= sizeof(*gdata)) {
> -		gedata_len = gdata->error_data_length;
> -		if (gedata_len > data_len - sizeof(*gdata))
> +
> +	if ((gdata->revision >> 8) >= 0x03) {
> +		gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> +		while (data_len >= sizeof(*gdata_v3)) {
> +			gedata_len = gdata_v3->error_data_length;
> +			if (gedata_len > data_len - sizeof(*gdata_v3))
> +				return -EINVAL;
> +			data_len -= gedata_len + sizeof(*gdata_v3);
> +			gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
> +		}
> +		if (data_len)
> +			return -EINVAL;
> +	} else {
> +		while (data_len >= sizeof(*gdata)) {
> +			gedata_len = gdata->error_data_length;
> +			if (gedata_len > data_len - sizeof(*gdata))
> +				return -EINVAL;
> +			data_len -= gedata_len + sizeof(*gdata);
> +			gdata = (void *)(gdata + 1) + gedata_len;
> +		}
> +		if (data_len)

As mentioned in the previous version, would it make sense to add some more
helpers to deal with record versions ? We seem to be doing the version switch and
code duplication at different places.

Does the following help ? Thoughts ?

#define acpi_hest_generic_data_error_length(gdata) (((struct acpi_hest_generic_data *)(gdata))->error_data_length)
#define acpi_hest_generic_data_size(gdata) \
	((acpi_hest_generic_data_version(gdata) >= 3) ? \
	 sizeof(struct acpi_hest_generic_data_v300) :	\
	 sizeof(struct acpi_hest_generic_data))
#define acpi_hest_generic_data_record_size(gdata)
	(acpi_hest_generic_data_size(gdata) + \
	 acpi_hest_generic_data_error_length(gdata))
#define acpi_hest_generic_data_next(gdata) \
	((void *)(gdata) + acpi_hest_generic_data_record_size(gdata))


Suzuki

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Russell King - ARM Linux]

On Fri, Oct 07, 2016 at 03:31:14PM -0600, Tyler Baicar wrote:
> +static void cper_estatus_print_section_v300(const char *pfx,
> +	const struct acpi_hest_generic_data_v300 *gdata)
> +{
> +	__u8 hour, min, sec, day, mon, year, century, *timestamp;
> +
> +	if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
> +		timestamp = (__u8 *)&(gdata->time_stamp);
> +		memcpy(&sec, timestamp, 1);
> +		memcpy(&min, timestamp + 1, 1);
> +		memcpy(&hour, timestamp + 2, 1);
> +		memcpy(&day, timestamp + 4, 1);
> +		memcpy(&mon, timestamp + 5, 1);
> +		memcpy(&year, timestamp + 6, 1);
> +		memcpy(&century, timestamp + 7, 1);

This is utterly silly.  Why are you using memcpy() to access individual
bytes of a u8 pointer?  What's wrong with:

		sec = timestamp[0];
		min = timestamp[1];
		hour = timestamp[2];
		day = timestamp[4];
		mon = timestamp[5];
		year = timestamp[6];
		century = timestamp[7];

or even do the conversion here:

		sec = bcd2bin(timestamp[0]);
... etc ...

> +		printk("%stime: ", pfx);
> +		printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
> +		printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
> +			bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
> +			bcd2bin(century), bcd2bin(year), bcd2bin(mon),
> +			bcd2bin(day));
> +	}

It's also a good idea to (as much as possible) keep to single printk()
statements - which makes the emission of the string more atomic wrt
other CPUs and contexts.  So, this should probably become (with the
conversion being done at the assignment of sec etc):

		printk("%stime: %7s %02d:%02d:%02d %02d%02d-%02d-%02d\n",
			pfx, 0x01 & timestamp[3] ? "precise" : "",
			hour, min, sec, century, year, mon, day);

which, IMHO, looks a lot nicer and doesn't risk some other printk()
getting between each individual part of the line.

> +}
> +
>  static void cper_estatus_print_section(
> -	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
> +	const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
>  {
>  	uuid_le *sec_type = (uuid_le *)gdata->section_type;
>  	__u16 severity;
>  	char newpfx[64];
>  
> +	if ((gdata->revision >> 8) >= 0x03)
> +		cper_estatus_print_section_v300(pfx,
> +			(const struct acpi_hest_generic_data_v300 *)gdata);
> +
>  	severity = gdata->error_severity;
>  	printk("%s""Error %d, type: %s\n", pfx, sec_no,
>  	       cper_severity_str(severity));

Not sure why you have the "" here - %sError works just as well and the
"" is just obfuscation - the compiler will eliminate the double-double
quote and merge the strings anyway.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH V3 01/10] acpi: apei: read ack upon ghes record consumption

[Punit Agrawal]

Hi Tyler,

A few comments below.

Tyler Baicar <tbaicar@codeaurora.org> writes:

> A RAS (Reliability, Availability, Serviceability) controller
> may be a separate processor running in parallel with OS
> execution, and may generate error records for consumption by
> the OS. If the RAS controller produces multiple error records,
> then they may be overwritten before the OS has consumed them.
>
> The Generic Hardware Error Source (GHES) v2 structure
> introduces the capability for the OS to acknowledge the
> consumption of the error record generated by the RAS
> controller. A RAS controller supporting GHESv2 shall wait for
> the acknowledgment before writing a new error record, thus
> eliminating the race condition.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> ---
>  drivers/acpi/apei/ghes.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/apei/hest.c |  7 +++++--
>  include/acpi/ghes.h      |  1 +
>  3 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 60746ef..3021f0e 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -45,6 +45,7 @@
>  #include <linux/aer.h>
>  #include <linux/nmi.h>
>  
> +#include <acpi/actbl1.h>
>  #include <acpi/ghes.h>
>  #include <acpi/apei.h>
>  #include <asm/tlbflush.h>
> @@ -244,10 +245,22 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>  	struct ghes *ghes;
>  	unsigned int error_block_length;
>  	int rc;
> +	struct acpi_hest_header *hest_hdr;
>  
>  	ghes = kzalloc(sizeof(*ghes), GFP_KERNEL);
>  	if (!ghes)
>  		return ERR_PTR(-ENOMEM);
> +
> +	hest_hdr = (struct acpi_hest_header *)generic;
> +	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR_V2) {
> +		ghes->generic_v2 = (struct acpi_hest_generic_v2 *)generic;
> +		rc = apei_map_generic_address(
> +			&ghes->generic_v2->read_ack_register);
> +		if (rc)
> +			goto err_unmap;
> +	} else
> +		ghes->generic_v2 = NULL;

Since you kzalloc ghes, shouldn't ghes->generic_v2 be NULL already?

> +
>  	ghes->generic = generic;
>  	rc = apei_map_generic_address(&generic->error_status_address);
>  	if (rc)
> @@ -270,6 +283,9 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>  
>  err_unmap:
>  	apei_unmap_generic_address(&generic->error_status_address);
> +	if (ghes->generic_v2)
> +		apei_unmap_generic_address(
> +			&ghes->generic_v2->read_ack_register);
>  err_free:
>  	kfree(ghes);
>  	return ERR_PTR(rc);
> @@ -279,6 +295,9 @@ static void ghes_fini(struct ghes *ghes)
>  {
>  	kfree(ghes->estatus);
>  	apei_unmap_generic_address(&ghes->generic->error_status_address);
> +	if (ghes->generic_v2)
> +		apei_unmap_generic_address(
> +			&ghes->generic_v2->read_ack_register);
>  }
>  
>  static inline int ghes_severity(int severity)
> @@ -648,6 +667,22 @@ static void ghes_estatus_cache_add(
>  	rcu_read_unlock();
>  }
>  
> +static int ghes_do_read_ack(struct acpi_hest_generic_v2 *generic_v2)
> +{
> +	int rc;
> +	u64 val = 0;
> +
> +	rc = apei_read(&val, &generic_v2->read_ack_register);
> +	if (rc)
> +		return rc;
> +	val &= generic_v2->read_ack_preserve <<
> +		generic_v2->read_ack_register.bit_offset;
> +	val |= generic_v2->read_ack_write;

Reading the spec, it is not clear whether you need the left shift
above.

Having said that, if you do it for read_ack_preserve, do you also need
to left shift read_ack_write by read_ack_register.bit_offset?

> +	rc = apei_write(val, &generic_v2->read_ack_register);
> +
> +	return rc;
> +}
> +
>  static int ghes_proc(struct ghes *ghes)
>  {
>  	int rc;
> @@ -660,6 +695,12 @@ static int ghes_proc(struct ghes *ghes)
>  			ghes_estatus_cache_add(ghes->generic, ghes->estatus);
>  	}
>  	ghes_do_proc(ghes, ghes->estatus);
> +
> +	if (ghes->generic_v2) {
> +		rc = ghes_do_read_ack(ghes->generic_v2);
> +		if (rc)
> +			return rc;
> +	}
>  out:
>  	ghes_clear_estatus(ghes);
>  	return 0;
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index 792a0d9..ef725a9 100644
> --- a/drivers/acpi/apei/hest.c
> +++ b/drivers/acpi/apei/hest.c
> @@ -52,6 +52,7 @@ static const int hest_esrc_len_tab[ACPI_HEST_TYPE_RESERVED] = {
>  	[ACPI_HEST_TYPE_AER_ENDPOINT] = sizeof(struct acpi_hest_aer),
>  	[ACPI_HEST_TYPE_AER_BRIDGE] = sizeof(struct acpi_hest_aer_bridge),
>  	[ACPI_HEST_TYPE_GENERIC_ERROR] = sizeof(struct acpi_hest_generic),
> +	[ACPI_HEST_TYPE_GENERIC_ERROR_V2] = sizeof(struct acpi_hest_generic_v2),
>  };
>  
>  static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
> @@ -146,7 +147,8 @@ static int __init hest_parse_ghes_count(struct acpi_hest_header *hest_hdr, void
>  {
>  	int *count = data;
>  
> -	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR)
> +	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR ||
> +	    hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR_V2)
>  		(*count)++;
>  	return 0;
>  }
> @@ -157,7 +159,8 @@ static int __init hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
>  	struct ghes_arr *ghes_arr = data;
>  	int rc, i;
>  
> -	if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR)
> +	if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR &&
> +	    hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR_V2)
>  		return 0;
>  
>  	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 720446c..d0108b6 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -14,6 +14,7 @@
>  
>  struct ghes {
>  	struct acpi_hest_generic *generic;
> +	struct acpi_hest_generic_v2 *generic_v2;

You either have a GHES or a GHESv2 structure. Instead of duplication,
could this be represented as a union?

Thanks,
Punit

>  	struct acpi_hest_generic_status *estatus;
>  	u64 buffer_paddr;
>  	unsigned long flags;

Re: [PATCH V3 04/10] arm64: exception: handle Synchronous External Abort

[Punit Agrawal]

Hi Tyler,

A couple of hopefully not bike shedding comments below.

Tyler Baicar <tbaicar@codeaurora.org> writes:

> SEA exceptions are often caused by an uncorrected hardware
> error, and are handled when data abort and instruction abort
> exception classes have specific values for their Fault Status
> Code.
> When SEA occurs, before killing the process, go through
> the handlers registered in the notification list.
> Update fault_info[] with specific SEA faults so that the
> new SEA handler is used.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> ---
>  arch/arm64/include/asm/system_misc.h | 13 ++++++++
>  arch/arm64/mm/fault.c                | 58 +++++++++++++++++++++++++++++-------
>  2 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index 57f110b..90daf4a 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -64,4 +64,17 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>  
>  #endif	/* __ASSEMBLY__ */
>  
> +/*
> + * The functions below are used to register and unregister callbacks
> + * that are to be invoked when a Synchronous External Abort (SEA)
> + * occurs. An SEA is raised by certain fault status codes that have
> + * either data or instruction abort as the exception class, and
> + * callbacks may be registered to parse or handle such hardware errors.
> + *
> + * Registered callbacks are run in an interrupt/atomic context. They
> + * are not allowed to block or sleep.
> + */
> +int sea_register_handler_chain(struct notifier_block *nb);
> +void sea_unregister_handler_chain(struct notifier_block *nb);
> +
>  #endif	/* __ASM_SYSTEM_MISC_H */
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 05d2bd7..81cb7ad 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -39,6 +39,22 @@
>  #include <asm/pgtable.h>
>  #include <asm/tlbflush.h>
>  
> +/*
> + * GHES SEA handler code may register a notifier call here to
> + * handle HW error record passed from platform.
> + */
> +static ATOMIC_NOTIFIER_HEAD(sea_handler_chain);
> +
> +int sea_register_handler_chain(struct notifier_block *nb)
> +{
> +	return atomic_notifier_chain_register(&sea_handler_chain, nb);
> +}
> +
> +void sea_unregister_handler_chain(struct notifier_block *nb)
> +{
> +	atomic_notifier_chain_unregister(&sea_handler_chain, nb);
> +}
> +

What do you think of naming the above functions as
[un]register_synchonous_ext_abort_notifier?

For an API, I find "sea" doesn't quite convey the message.

One more comment below.

>  static const char *fault_name(unsigned int esr);
>  
>  #ifdef CONFIG_KPROBES
> @@ -480,6 +496,28 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  	return 1;
>  }
>  
> +/*
> + * This abort handler deals with Synchronous External Abort.
> + * It calls notifiers, and then returns "fault".
> + */
> +static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
> +{
> +	struct siginfo info;
> +
> +	atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
> +
> +	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
> +		 fault_name(esr), esr, addr);
> +
> +	info.si_signo = SIGBUS;
> +	info.si_errno = 0;
> +	info.si_code  = 0;
> +	info.si_addr  = (void __user *)addr;
> +	arm64_notify_die("", regs, &info, esr);
> +
> +	return 0;
> +}
> +
>  static const struct fault_info {
>  	int	(*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
>  	int	sig;
> @@ -502,22 +540,22 @@ static const struct fault_info {
>  	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 1 permission fault"	},
>  	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 2 permission fault"	},
>  	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 3 permission fault"	},
> -	{ do_bad,		SIGBUS,  0,		"synchronous external abort"	},
> +	{ do_sea,		SIGBUS,  0,		"synchronous external abort"	},
>  	{ do_bad,		SIGBUS,  0,		"unknown 17"			},
>  	{ do_bad,		SIGBUS,  0,		"unknown 18"			},
>  	{ do_bad,		SIGBUS,  0,		"unknown 19"			},
> -	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
> -	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
> -	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
> -	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
> -	{ do_bad,		SIGBUS,  0,		"synchronous parity error"	},
> +	{ do_sea,		SIGBUS,  0,		"level 0 SEA (trans tbl walk)"	},
> +	{ do_sea,		SIGBUS,  0,		"level 1 SEA (trans tbl walk)"	},
> +	{ do_sea,		SIGBUS,  0,		"level 2 SEA (trans tbl walk)"	},
> +	{ do_sea,		SIGBUS,  0,		"level 3 SEA (trans tbl walk)"	},
                                                                 ^^^
The comment about naming applies here as well.

Thanks,
Punit

[...]

Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Punit Agrawal]

Tyler Baicar <tbaicar@codeaurora.org> writes:

> ARM APEI extension proposal added SEA (Synchrounous External
> Abort) notification type for ARMv8.
> Add a new GHES error source handling function for SEA. If an error
> source's notification type is SEA, then this function can be registered
> into the SEA exception handler. That way GHES will parse and report
> SEA exceptions when they occur.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>

This patch fails to apply for me on v4.8. Is there a different tree this
is based on?

One comment below.

[...]

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index c8488f1..28d5a09 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -50,6 +50,10 @@
>  #include <acpi/apei.h>
>  #include <asm/tlbflush.h>
>  
> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
> +#include <asm/system_misc.h>
> +#endif
> +
>  #include "apei-internal.h"
>  
>  #define GHES_PFX	"GHES: "
> @@ -779,6 +783,62 @@ static struct notifier_block ghes_notifier_sci = {
>  	.notifier_call = ghes_notify_sci,
>  };
>  
> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
> +static LIST_HEAD(ghes_sea);
> +
> +static int ghes_notify_sea(struct notifier_block *this,
> +				  unsigned long event, void *data)
> +{
> +	struct ghes *ghes;
> +	int ret = NOTIFY_DONE;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
> +		if (!ghes_proc(ghes))
> +			ret = NOTIFY_OK;

Not something you've introduced but it looks like ghes_proc erroneously
never returns anything other than 0. I plan to post the below fix to
address it.

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 60746ef..caea575 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes)
        ghes_do_proc(ghes, ghes->estatus);
 out:
        ghes_clear_estatus(ghes);
-   return 0;
+ return rc;
 }

> +	}
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +

[...]

Re: [PATCH V3 09/10] trace, ras: add ARM processor error trace event

[Baicar, Tyler]

Hello Steve,

Thank you for your feedback! Responses below.

On 10/7/2016 3:39 PM, Steven Rostedt wrote:
> On Fri,  7 Oct 2016 15:31:21 -0600
> Tyler Baicar <tbaicar@codeaurora.org> wrote:
>
>> Currently there are trace events for the various RAS
>> errors with the exception of ARM processor type errors.
>> Add a new trace event for such errors so that the user
>> will know when they occur. These trace events are
>> consistent with the ARM processor error section type
>> defined in UEFI 2.6 spec section N.2.4.4.
>>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> ---
>>   drivers/firmware/efi/cper.c |  9 ++++++
>>   drivers/ras/ras.c           |  1 +
>>   include/ras/ras_event.h     | 67 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 77 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> index f9ffba6..21b8a6f 100644
>> --- a/drivers/firmware/efi/cper.c
>> +++ b/drivers/firmware/efi/cper.c
>> @@ -34,6 +34,7 @@
>>   #include <linux/aer.h>
>>   #include <linux/printk.h>
>>   #include <linux/bcd.h>
>> +#include <ras/ras_event.h>
>>   
>>   #define INDENT_SP	" "
>>   
>> @@ -256,6 +257,14 @@ static void cper_print_proc_armv8(const char *pfx,
>>   		    CPER_ARMV8_INFO_VALID_PHYSICAL_ADDR)
>>   			printk("%sphysical fault address: 0x%016llx\n",
>>   				newpfx, err_info->physical_fault_addr);
>> +		trace_arm_event(proc->affinity_level, proc->mpidr, proc->midr,
>> +				proc->running_state, proc->psci_state,
>> +				err_info->version, err_info->type,
>> +				err_info->multiple_error,
>> +				err_info->validation_bits,
>> +				err_info->error_info,
>> +				err_info->virt_fault_addr,
>> +				err_info->physical_fault_addr);
> Why waste all the effort into passing each individual field. Why not
> just pass the structure in and sort it out in the TP_fast_assign()?
That should be a lot cleaner, I will make that change in the next patchset.
>>   		err_info += 1;
>>   	}
>>   
>> diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
>> index fb2500b..8ba5a94 100644
>> --- a/drivers/ras/ras.c
>> +++ b/drivers/ras/ras.c
>> @@ -28,3 +28,4 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
>>   #endif
>>   EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
>>   EXPORT_TRACEPOINT_SYMBOL_GPL(unknown_sec_event);
>> +EXPORT_TRACEPOINT_SYMBOL_GPL(arm_event);
>> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
>> index 5861b6f..eb2719a 100644
>> --- a/include/ras/ras_event.h
>> +++ b/include/ras/ras_event.h
>> @@ -162,6 +162,73 @@ TRACE_EVENT(mc_event,
>>   );
>>   
>>   /*
>> + * ARM Processor Events Report
>> + *
>> + * This event is generated when hardware detects an ARM processor error
>> + * has occurred. UEFI 2.6 spec section N.2.4.4.
>> + */
>> +TRACE_EVENT(arm_event,
>> +
>> +	TP_PROTO(const u8 affinity,
>> +		 const u64 mpidr,
>> +		 const u64 midr,
>> +		 const u32 running_state,
>> +		 const u32 psci_state,
>> +		 const u8 version,
>> +		 const u8 type,
>> +		 const u16 err_count,
>> +		 const u8 flags,
>> +		 const u64 info,
>> +		 const u64 virt_fault_addr,
>> +		 const u64 phys_fault_addr),
>> +
>> +	TP_ARGS(affinity, mpidr, midr, running_state, psci_state,
>> +		version, type, err_count, flags, info, virt_fault_addr,
>> +		phys_fault_addr),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(u8, affinity)
>> +		__field(u64, mpidr)
>> +		__field(u64, midr)
>> +		__field(u32, running_state)
>> +		__field(u32, psci_state)
>> +		__field(u8, version)
>> +		__field(u8, type)
>> +		__field(u16, err_count)
>> +		__field(u8, flags)
>> +		__field(u64, info)
>> +		__field(u64, virt_fault_addr)
>> +		__field(u64, phys_fault_addr)
> The above creates a structure with lots of holes in it. Pack it better.
> You want something like:
>
> 	__field(u64, mpidr)
> 	__field(u64, midr)
> 	__field(u64, info)
> 	__field(u64, virt_fault_addr)
> 	__field(u64, phys_fault_addr)
> 	__field(u32, running_state)
> 	__field(u32, psci_state)
> 	__field(u16, err_count)
> 	__field(u8, affinity)
> 	__field(u8, version)
> 	__field(u8, type)
> 	__field(u8, flags)
>
> The above is a total of 54 bytes. Your original was at a minimum, 64
> bytes.
>
> -- Steve
I will reorder the structure in the next patchset. I originally used 
this order because that is the order the entries appear in the spec 
(table 260 and 261 of UEFI spec 2.6). It makes more sense to save the 
space though.

Thanks,
Tyler
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->affinity = affinity;
>> +		__entry->mpidr = mpidr;
>> +		__entry->midr = midr;
>> +		__entry->running_state = running_state;
>> +		__entry->psci_state = psci_state;
>> +		__entry->version = version;
>> +		__entry->type = type;
>> +		__entry->err_count = err_count;
>> +		__entry->flags = flags;
>> +		__entry->info = info;
>> +		__entry->virt_fault_addr = virt_fault_addr;
>> +		__entry->phys_fault_addr = phys_fault_addr;
>> +	),
>> +
>> +	TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
>> +		  "running state: %d; PSCI state: %d; version: %d; type: %d; "
>> +		  "error count: 0x%04x; flags: 0x%02x; info: %016llx; "
>> +		  "virtual fault address: %016llx; "
>> +		  "physical fault address: %016llx",
>> +		  __entry->affinity, __entry->mpidr, __entry->midr,
>> +		  __entry->running_state, __entry->psci_state, __entry->version,
>> +		  __entry->type, __entry->err_count, __entry->flags,
>> +		  __entry->info, __entry->virt_fault_addr,
>> +		  __entry->phys_fault_addr)
>> +);
>> +
>> +/*
>>    * Unknown Section Report
>>    *
>>    * This event is generated when hardware detected a hardware

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Baicar, Tyler]

Hello Suzuki,

Thank you for the feedback! Responses below.


On 10/11/2016 11:28 AM, Suzuki K Poulose wrote:
> On 07/10/16 22:31, Tyler Baicar wrote:
>> Currently when a RAS error is reported it is not timestamped.
>> The ACPI 6.1 spec adds the timestamp field to the generic error
>> data entry v3 structure. The timestamp of when the firmware
>> generated the error is now being reported.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>
> Please could you keep the people who reviewed/commented on your series 
> in the past,
> whenever you post a new version ?
Do you mean to just send the new version to their e-mail directly in 
addition to the lists? If so, I will do that next time.

I know you provided good feedback on the previous patchset, but I did 
not have anyone specifically respond to add "reviewed-by:...". I don't 
think I should add reviewed-by for someone unless they specifically add 
it in a response :)
>
>> ---
>>  drivers/acpi/apei/ghes.c    | 25 ++++++++++--
>>  drivers/firmware/efi/cper.c | 97 
>> +++++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 105 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 3021f0e..c8488f1 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -80,6 +80,10 @@
>>      ((struct acpi_hest_generic_status *)                \
>>       ((struct ghes_estatus_node *)(estatus_node) + 1))
>>
>> +#define acpi_hest_generic_data_version(gdata)            \
>> +    (gdata->revision >> 8)
>
> ...
>
>> +inline void *acpi_hest_generic_data_payload(struct 
>> acpi_hest_generic_data *gdata)
>> +{
>> +    return acpi_hest_generic_data_version(gdata) >= 3 ?
>> +        (void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
>> +        gdata + 1;
>> +}
>> +
>
>
>
>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> index d425374..9fa1317 100644
>> --- a/drivers/firmware/efi/cper.c
>> +++ b/drivers/firmware/efi/cper.c
>
>> +#define acpi_hest_generic_data_version(gdata)        \
>> +    (gdata->revision >> 8)
>> +
>
> ...
>
>> +static inline void *acpi_hest_generic_data_payload(struct 
>> acpi_hest_generic_data *gdata)
>> +{
>> +    return acpi_hest_generic_data_version(gdata) >= 3 ?
>> +        (void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
>> +        gdata + 1;
>> +}
>
> Could these go to a header file, so that we don't need duplicate 
> definitions of these helpers in
> different files ?
>
I think that should work to avoid duplication. I will move them to a 
header file in the next patchset.
>> +
>> +static void cper_estatus_print_section_v300(const char *pfx,
>> +    const struct acpi_hest_generic_data_v300 *gdata)
>> +{
>> +    __u8 hour, min, sec, day, mon, year, century, *timestamp;
>> +
>> +    if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>> +        timestamp = (__u8 *)&(gdata->time_stamp);
>> +        memcpy(&sec, timestamp, 1);
>> +        memcpy(&min, timestamp + 1, 1);
>> +        memcpy(&hour, timestamp + 2, 1);
>> +        memcpy(&day, timestamp + 4, 1);
>> +        memcpy(&mon, timestamp + 5, 1);
>> +        memcpy(&year, timestamp + 6, 1);
>> +        memcpy(&century, timestamp + 7, 1);
>> +        printk("%stime: ", pfx);
>> +        printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>
> What format is the (timestamp + 3) stored in ? Does it need conversion ?
The third byte of the timestamp is currently only used to determine if 
the time is precise or not. Bit 0 is used to specify that and the other 
bits in this byte are marked as reserved. This is shown in table 247 of 
the UEFI spec 2.6:

Byte 3:
   Bit 0 – Timestamp is precise if this bit is set and correlates to the 
time of the error event.
   Bit 7:1 – Reserved
>
>> +        printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
>> +            bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
>> +            bcd2bin(century), bcd2bin(year), bcd2bin(mon),
>> +            bcd2bin(day));
>> +    }
>
> minor nit: Would it be easier to order/parse the error messages if the 
> date
> is printed first followed by time ?
>
> i.e,
>     17:20:14 2016-09-15 Mon
>         vs
>     2016-09-15 Mon 17:20:14
>
> e.g, people looking at a huge log, looking for logs from a specific 
> date might
> find the latter more useful to skip the messages.
>
The latter does seem like it would be better for parsing large logs. I 
can rearrange the order in the next patchset.
>> +}
>> +
>>  static void cper_estatus_print_section(
>> -    const char *pfx, const struct acpi_hest_generic_data *gdata, int 
>> sec_no)
>> +    const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
>>  {
>>      uuid_le *sec_type = (uuid_le *)gdata->section_type;
>>      __u16 severity;
>>      char newpfx[64];
>>
>> +    if ((gdata->revision >> 8) >= 0x03)
>
> Could we use the helper defined above ?
Yes, I'll change this to use acpi_hest_generic_data_version(gdata) instead.
>
>> @@ -451,12 +497,22 @@ void cper_estatus_print(const char *pfx,
>>      printk("%s""event severity: %s\n", pfx, 
>> cper_severity_str(severity));
>>      data_len = estatus->data_length;
>>      gdata = (struct acpi_hest_generic_data *)(estatus + 1);
>> +    if ((gdata->revision >> 8) >= 0x03)
>
> Same as above, use the macro ?
Yes, I'll change this to use acpi_hest_generic_data_version(gdata) instead.
>
>> +        gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
>> +
>>      snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
>> +
>>      while (data_len >= sizeof(*gdata)) {
>>          gedata_len = gdata->error_data_length;
>>          cper_estatus_print_section(newpfx, gdata, sec_no);
>> -        data_len -= gedata_len + sizeof(*gdata);
>> -        gdata = (void *)(gdata + 1) + gedata_len;
>> +        if(gdata_v3) {
>> +            data_len -= gedata_len + sizeof(*gdata_v3);
>> +            gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
>> +            gdata = (struct acpi_hest_generic_data *)gdata_v3;
>> +        } else {
>> +            data_len -= gedata_len + sizeof(*gdata);
>> +            gdata = (void *)(gdata + 1) + gedata_len;
>> +        }
>>          sec_no++;
>>      }
>
> ...
>
>>
>> @@ -486,15 +543,29 @@ int cper_estatus_check(const struct 
>> acpi_hest_generic_status *estatus)
>>          return rc;
>>      data_len = estatus->data_length;
>>      gdata = (struct acpi_hest_generic_data *)(estatus + 1);
>> -    while (data_len >= sizeof(*gdata)) {
>> -        gedata_len = gdata->error_data_length;
>> -        if (gedata_len > data_len - sizeof(*gdata))
>> +
>> +    if ((gdata->revision >> 8) >= 0x03) {
>> +        gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
>> +        while (data_len >= sizeof(*gdata_v3)) {
>> +            gedata_len = gdata_v3->error_data_length;
>> +            if (gedata_len > data_len - sizeof(*gdata_v3))
>> +                return -EINVAL;
>> +            data_len -= gedata_len + sizeof(*gdata_v3);
>> +            gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
>> +        }
>> +        if (data_len)
>> +            return -EINVAL;
>> +    } else {
>> +        while (data_len >= sizeof(*gdata)) {
>> +            gedata_len = gdata->error_data_length;
>> +            if (gedata_len > data_len - sizeof(*gdata))
>> +                return -EINVAL;
>> +            data_len -= gedata_len + sizeof(*gdata);
>> +            gdata = (void *)(gdata + 1) + gedata_len;
>> +        }
>> +        if (data_len)
>
> As mentioned in the previous version, would it make sense to add some 
> more
> helpers to deal with record versions ? We seem to be doing the version 
> switch and
> code duplication at different places.
>
> Does the following help ? Thoughts ?
>
> #define acpi_hest_generic_data_error_length(gdata) (((struct 
> acpi_hest_generic_data *)(gdata))->error_data_length)
> #define acpi_hest_generic_data_size(gdata) \
>     ((acpi_hest_generic_data_version(gdata) >= 3) ? \
>      sizeof(struct acpi_hest_generic_data_v300) :    \
>      sizeof(struct acpi_hest_generic_data))
> #define acpi_hest_generic_data_record_size(gdata)
>     (acpi_hest_generic_data_size(gdata) + \
>      acpi_hest_generic_data_error_length(gdata))
> #define acpi_hest_generic_data_next(gdata) \
>     ((void *)(gdata) + acpi_hest_generic_data_record_size(gdata))
>
>
> Suzuki
These helpers will definitely help consolidate this code. I will use 
these in the next version to remove the code duplication here.

Thanks,
Tyler

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Baicar, Tyler]

Hello Russell,

Thank you for the feedback! Responses below


On 10/11/2016 12:52 PM, Russell King - ARM Linux wrote:
> On Fri, Oct 07, 2016 at 03:31:14PM -0600, Tyler Baicar wrote:
>> +static void cper_estatus_print_section_v300(const char *pfx,
>> +	const struct acpi_hest_generic_data_v300 *gdata)
>> +{
>> +	__u8 hour, min, sec, day, mon, year, century, *timestamp;
>> +
>> +	if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>> +		timestamp = (__u8 *)&(gdata->time_stamp);
>> +		memcpy(&sec, timestamp, 1);
>> +		memcpy(&min, timestamp + 1, 1);
>> +		memcpy(&hour, timestamp + 2, 1);
>> +		memcpy(&day, timestamp + 4, 1);
>> +		memcpy(&mon, timestamp + 5, 1);
>> +		memcpy(&year, timestamp + 6, 1);
>> +		memcpy(&century, timestamp + 7, 1);
> This is utterly silly.  Why are you using memcpy() to access individual
> bytes of a u8 pointer?  What's wrong with:
>
> 		sec = timestamp[0];
> 		min = timestamp[1];
> 		hour = timestamp[2];
> 		day = timestamp[4];
> 		mon = timestamp[5];
> 		year = timestamp[6];
> 		century = timestamp[7];
>
> or even do the conversion here:
>
> 		sec = bcd2bin(timestamp[0]);
> ... etc ...
Yes, that will be a lot cleaner especially with moving the conversion here.
>
>> +		printk("%stime: ", pfx);
>> +		printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>> +		printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
>> +			bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
>> +			bcd2bin(century), bcd2bin(year), bcd2bin(mon),
>> +			bcd2bin(day));
>> +	}
> It's also a good idea to (as much as possible) keep to single printk()
> statements - which makes the emission of the string more atomic wrt
> other CPUs and contexts.  So, this should probably become (with the
> conversion being done at the assignment of sec etc):
>
> 		printk("%stime: %7s %02d:%02d:%02d %02d%02d-%02d-%02d\n",
> 			pfx, 0x01 & timestamp[3] ? "precise" : "",
> 			hour, min, sec, century, year, mon, day);
>
> which, IMHO, looks a lot nicer and doesn't risk some other printk()
> getting between each individual part of the line.
I will make this change in the next version. This printk does look a lot 
nicer and avoids other prints from getting in the middle (I actually 
just saw that happen in testing a couple days ago)
>> +}
>> +
>>   static void cper_estatus_print_section(
>> -	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
>> +	const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
>>   {
>>   	uuid_le *sec_type = (uuid_le *)gdata->section_type;
>>   	__u16 severity;
>>   	char newpfx[64];
>>   
>> +	if ((gdata->revision >> 8) >= 0x03)
>> +		cper_estatus_print_section_v300(pfx,
>> +			(const struct acpi_hest_generic_data_v300 *)gdata);
>> +
>>   	severity = gdata->error_severity;
>>   	printk("%s""Error %d, type: %s\n", pfx, sec_no,
>>   	       cper_severity_str(severity));
> Not sure why you have the "" here - %sError works just as well and the
> "" is just obfuscation - the compiler will eliminate the double-double
> quote and merge the strings anyway.
>
I will remove the "" in the next version.

Thanks,
Tyler

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Suzuki K Poulose]

On 12/10/16 23:10, Baicar, Tyler wrote:
> Hello Suzuki,
>
> Thank you for the feedback! Responses below.
>
>
> On 10/11/2016 11:28 AM, Suzuki K Poulose wrote:
>> On 07/10/16 22:31, Tyler Baicar wrote:
>>> Currently when a RAS error is reported it is not timestamped.
>>> The ACPI 6.1 spec adds the timestamp field to the generic error
>>> data entry v3 structure. The timestamp of when the firmware
>>> generated the error is now being reported.
>>>
>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>>> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>>
>> Please could you keep the people who reviewed/commented on your series in the past,
>> whenever you post a new version ?
> Do you mean to just send the new version to their e-mail directly in addition to the lists? If so, I will do that next time.

If you send a new version of a series to the list, it is a good idea to keep
the people who commented (significantly) on your previous version in Cc, especially
when you have addressed their feedback. That will help them to keep track of the
series. People can always see the new version in the list, but then it is so easy
to miss something in the 100s of emails you get each day. I am sure people have
special filters for the emails based on if they are in Cc/To etc.

>
> I know you provided good feedback on the previous patchset, but I did not have anyone specifically respond to add "reviewed-by:...". I don't think I should add reviewed-by for someone unless they specifically add it in a response :)

No, I haven't yet "Reviewed-by" your patches. I had some comments on it, which means
I expected it to be addressed as you committed in your response.

>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index 3021f0e..c8488f1 100644
>>> --- a/drivers/acpi/apei/ghes.c
>>> +++ b/drivers/acpi/apei/ghes.c
>>> @@ -80,6 +80,10 @@

> I think that should work to avoid duplication. I will move them to a header file in the next patchset.
>>> +
>>> +static void cper_estatus_print_section_v300(const char *pfx,
>>> +    const struct acpi_hest_generic_data_v300 *gdata)
>>> +{
>>> +    __u8 hour, min, sec, day, mon, year, century, *timestamp;
>>> +
>>> +    if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>>> +        timestamp = (__u8 *)&(gdata->time_stamp);
>>> +        memcpy(&sec, timestamp, 1);
>>> +        memcpy(&min, timestamp + 1, 1);
>>> +        memcpy(&hour, timestamp + 2, 1);
>>> +        memcpy(&day, timestamp + 4, 1);
>>> +        memcpy(&mon, timestamp + 5, 1);
>>> +        memcpy(&year, timestamp + 6, 1);
>>> +        memcpy(&century, timestamp + 7, 1);
>>> +        printk("%stime: ", pfx);
>>> +        printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>>
>> What format is the (timestamp + 3) stored in ? Does it need conversion ?
> The third byte of the timestamp is currently only used to determine if the time is precise or not. Bit 0 is used to specify that and the other bits in this byte are marked as reserved. This is shown in table 247 of the UEFI spec 2.6:
>
> Byte 3:
>   Bit 0 – Timestamp is precise if this bit is set and correlates to the time of the error event.
>   Bit 7:1 – Reserved

Is it always the same endianness as that of the CPU ?

Cheers
Suzuki

Re: [PATCH V3 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section

[Punit Agrawal]

Hi Tyler,

One last comment...

Tyler Baicar <tbaicar@codeaurora.org> writes:

> UEFI spec allows for non-standard section in Common Platform Error
> Record. This is defined in section N.2.3 of UEFI version 2.5.
>
> Currently if the CPER section's type (UUID) does not match with
> any section type that the kernel knows how to parse, trace event
> is not generated for such section. And thus user is not able to know
> happening of such hardware error, including error record of
> non-standard section.
>
> This commit generates a trace event which contains raw error data
> for unrecognized CPER section.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> ---
>  drivers/acpi/apei/ghes.c | 18 +++++++++++++++++-
>  drivers/ras/ras.c        |  1 +
>  include/ras/ras_event.h  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 36894c8..cb4c7f4 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -49,6 +49,7 @@
>  #include <acpi/ghes.h>
>  #include <acpi/apei.h>
>  #include <asm/tlbflush.h>
> +#include <ras/ras_event.h>
>  
>  #ifdef CONFIG_HAVE_ACPI_APEI_SEA
>  #include <asm/system_misc.h>
> @@ -468,12 +469,21 @@ static void ghes_do_proc(struct ghes *ghes,
>  	int sev, sec_sev;
>  	struct acpi_hest_generic_data *gdata;
>  	uuid_le sec_type;
> +	uuid_le *fru_id;
> +	char *fru_text = "";
>  
>  	sev = ghes_severity(estatus->error_severity);
>  	apei_estatus_for_each_section(estatus, gdata) {
>  		sec_sev = ghes_severity(gdata->error_severity);
>  		sec_type = *(uuid_le *)gdata->section_type;
>  
> +		if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
> +			fru_id = (uuid_le *)gdata->fru_id;
> +		else
> +			fru_id = &NULL_UUID_LE;

fru_id can be initialised at declaration and drop the else here. The
same is already being done for fru_text.

Thanks,
Punit

> +		if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
> +			fru_text = gdata->fru_text;
> +
>  		if (!uuid_le_cmp(sec_type,
>  				 CPER_SEC_PLATFORM_MEM)) {
>  			struct cper_sec_mem_err *mem_err;

[...]

Re: [PATCH V3 06/10] acpi: apei: panic OS with fatal error status block

[Suzuki K Poulose]

On 07/10/16 22:31, Tyler Baicar wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
>
> Even if an error status block's severity is fatal, the kernel does not
> honor the severity level and panic.
>
> With the firmware first model, the platform could inform the OS about a
> fatal hardware error through the non-NMI GHES notification type. The OS
> should panic when a hardware error record is received with this
> severity.
>
> Call panic() after CPER data in error status block is printed if
> severity is fatal, before each error section is handled.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> ---
>  drivers/acpi/apei/ghes.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 28d5a09..36894c8 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -141,6 +141,8 @@ static unsigned long ghes_estatus_pool_size_request;
>  static struct ghes_estatus_cache *ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
>  static atomic_t ghes_estatus_cache_alloced;
>
> +static int ghes_panic_timeout __read_mostly = 30;
> +
>  static int ghes_ioremap_init(void)
>  {
>  	ghes_ioremap_area = __get_vm_area(PAGE_SIZE * GHES_IOREMAP_PAGES,
> @@ -715,6 +717,12 @@ static int ghes_proc(struct ghes *ghes)
>  		if (ghes_print_estatus(NULL, ghes->generic, ghes->estatus))
>  			ghes_estatus_cache_add(ghes->generic, ghes->estatus);
>  	}
> +	if (ghes_severity(ghes->estatus->error_severity) >= GHES_SEV_PANIC) {
> +		if (panic_timeout == 0)
> +			panic_timeout = ghes_panic_timeout;
> +		panic("Fatal hardware error!");

I think there is a chance that we might miss the o/p of ghes_print_estatus() as we use
no pfx, and it could default to the normal loglevel and would never get printed
if panic() is encountered before it. On the other hand, there is already a
__ghes_panic() which does similar stuff. Is there a way we could reuse
(may be even parts of) it ? Or at least use KERN_EMERG for the ghes_print_estatus(),
if the severity could result in panic() ?

Cheers
Suzuki

Re: [PATCH V3 10/10] arm64: KVM: add guest SEA support

[Punit Agrawal]

Hi Tyler,

I know I've had my last comment already ;), but I thought I'd rather
raise the question than stay confused...

Tyler Baicar <tbaicar@codeaurora.org> writes:

> Currently external aborts are unsupported by the guest abort
> handling. Add handling for SEAs so that the host kernel reports
> SEAs which occur in the guest kernel.
>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> ---
>  arch/arm/include/asm/kvm_arm.h       |  1 +
>  arch/arm/include/asm/system_misc.h   |  5 +++++
>  arch/arm/kvm/mmu.c                   | 15 +++++++++++++--
>  arch/arm64/include/asm/kvm_arm.h     |  1 +
>  arch/arm64/include/asm/system_misc.h |  2 ++
>  arch/arm64/mm/fault.c                | 13 +++++++++++++
>  6 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index e22089f..33a77509 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -187,6 +187,7 @@
>  #define FSC_FAULT	(0x04)
>  #define FSC_ACCESS	(0x08)
>  #define FSC_PERM	(0x0c)
> +#define FSC_EXTABT	(0x10)
>  
>  /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>  #define HPFAR_MASK	(~0xf)
> diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
> index a3d61ad..86e1faa 100644
> --- a/arch/arm/include/asm/system_misc.h
> +++ b/arch/arm/include/asm/system_misc.h
> @@ -24,4 +24,9 @@ extern unsigned int user_debug;
>  
>  #endif /* !__ASSEMBLY__ */
>  
> +inline int handle_guest_sea(unsigned long addr, unsigned int esr)
> +{
> +	return -1;
> +}
> +
>  #endif /* __ASM_ARM_SYSTEM_MISC_H */
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index e9a5c0e..24cde84 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -29,6 +29,7 @@
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/virt.h>
> +#include <asm/system_misc.h>
>  
>  #include "trace.h"
>  
> @@ -1441,8 +1442,18 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  	/* Check the stage-2 fault is trans. fault or write fault */
>  	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
> -	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
> -	    fault_status != FSC_ACCESS) {
> +
> +	if (fault_status == FSC_EXTABT) {
> +		if(handle_guest_sea((unsigned long)fault_ipa,
> +				    kvm_vcpu_get_hsr(vcpu))) {
> +			kvm_err("Failed to handle guest SEA, FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
> +				kvm_vcpu_trap_get_class(vcpu),
> +				(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
> +				(unsigned long)kvm_vcpu_get_hsr(vcpu));
> +			return -EFAULT;
> +		}
> +	} else if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
> +		   fault_status != FSC_ACCESS) {
>  		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>  			kvm_vcpu_trap_get_class(vcpu),
>  			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 4b5c977..be0efb6 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -201,6 +201,7 @@
>  #define FSC_FAULT	ESR_ELx_FSC_FAULT
>  #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
>  #define FSC_PERM	ESR_ELx_FSC_PERM
> +#define FSC_EXTABT	ESR_ELx_FSC_EXTABT
>  
>  /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>  #define HPFAR_MASK	(~UL(0xf))
> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index 90daf4a..8522fff 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -77,4 +77,6 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>  int sea_register_handler_chain(struct notifier_block *nb);
>  void sea_unregister_handler_chain(struct notifier_block *nb);
>  
> +int handle_guest_sea(unsigned long addr, unsigned int esr);
> +
>  #endif	/* __ASM_SYSTEM_MISC_H */
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 81cb7ad..d714432 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -597,6 +597,19 @@ static const char *fault_name(unsigned int esr)
>  }
>  
>  /*
> + * Handle Synchronous External Aborts that occur in a guest kernel.
> + */
> +int handle_guest_sea(unsigned long addr, unsigned int esr)
> +{
> +	atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
> +
> +	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
> +		fault_name(esr), esr, addr);
> +
> +	return 0;
> +}

Don't we need to pass the abort to the guest?

Thanks,
Punit

> +
> +/*
>   * Dispatch a data abort to the relevant handler.
>   */
>  asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,

Re: [PATCH V3 01/10] acpi: apei: read ack upon ghes record consumption

[Baicar, Tyler]

Hello Punit,

Thank you for the feedback! Responses below

On 10/12/2016 9:39 AM, Punit Agrawal wrote:
> Hi Tyler,
>
> A few comments below.
>
> Tyler Baicar <tbaicar@codeaurora.org> writes:
>
>> A RAS (Reliability, Availability, Serviceability) controller
>> may be a separate processor running in parallel with OS
>> execution, and may generate error records for consumption by
>> the OS. If the RAS controller produces multiple error records,
>> then they may be overwritten before the OS has consumed them.
>>
>> The Generic Hardware Error Source (GHES) v2 structure
>> introduces the capability for the OS to acknowledge the
>> consumption of the error record generated by the RAS
>> controller. A RAS controller supporting GHESv2 shall wait for
>> the acknowledgment before writing a new error record, thus
>> eliminating the race condition.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>> ---
>>   drivers/acpi/apei/ghes.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>   drivers/acpi/apei/hest.c |  7 +++++--
>>   include/acpi/ghes.h      |  1 +
>>   3 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 60746ef..3021f0e 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -45,6 +45,7 @@
>>   #include <linux/aer.h>
>>   #include <linux/nmi.h>
>>   
>> +#include <acpi/actbl1.h>
>>   #include <acpi/ghes.h>
>>   #include <acpi/apei.h>
>>   #include <asm/tlbflush.h>
>> @@ -244,10 +245,22 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>>   	struct ghes *ghes;
>>   	unsigned int error_block_length;
>>   	int rc;
>> +	struct acpi_hest_header *hest_hdr;
>>   
>>   	ghes = kzalloc(sizeof(*ghes), GFP_KERNEL);
>>   	if (!ghes)
>>   		return ERR_PTR(-ENOMEM);
>> +
>> +	hest_hdr = (struct acpi_hest_header *)generic;
>> +	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR_V2) {
>> +		ghes->generic_v2 = (struct acpi_hest_generic_v2 *)generic;
>> +		rc = apei_map_generic_address(
>> +			&ghes->generic_v2->read_ack_register);
>> +		if (rc)
>> +			goto err_unmap;
>> +	} else
>> +		ghes->generic_v2 = NULL;
> Since you kzalloc ghes, shouldn't ghes->generic_v2 be NULL already?
Yes, the documentation says kzalloc returns memory set to zero, so I 
will remove this else statement.
>> +
>>   	ghes->generic = generic;
>>   	rc = apei_map_generic_address(&generic->error_status_address);
>>   	if (rc)
>> @@ -270,6 +283,9 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>>   
>>   err_unmap:
>>   	apei_unmap_generic_address(&generic->error_status_address);
>> +	if (ghes->generic_v2)
>> +		apei_unmap_generic_address(
>> +			&ghes->generic_v2->read_ack_register);
>>   err_free:
>>   	kfree(ghes);
>>   	return ERR_PTR(rc);
>> @@ -279,6 +295,9 @@ static void ghes_fini(struct ghes *ghes)
>>   {
>>   	kfree(ghes->estatus);
>>   	apei_unmap_generic_address(&ghes->generic->error_status_address);
>> +	if (ghes->generic_v2)
>> +		apei_unmap_generic_address(
>> +			&ghes->generic_v2->read_ack_register);
>>   }
>>   
>>   static inline int ghes_severity(int severity)
>> @@ -648,6 +667,22 @@ static void ghes_estatus_cache_add(
>>   	rcu_read_unlock();
>>   }
>>   
>> +static int ghes_do_read_ack(struct acpi_hest_generic_v2 *generic_v2)
>> +{
>> +	int rc;
>> +	u64 val = 0;
>> +
>> +	rc = apei_read(&val, &generic_v2->read_ack_register);
>> +	if (rc)
>> +		return rc;
>> +	val &= generic_v2->read_ack_preserve <<
>> +		generic_v2->read_ack_register.bit_offset;
>> +	val |= generic_v2->read_ack_write;
> Reading the spec, it is not clear whether you need the left shift
> above.
>
> Having said that, if you do it for read_ack_preserve, do you also need
> to left shift read_ack_write by read_ack_register.bit_offset?
Good catch, it looks like the read_ack_write should also get this shift. 
I'm using a shift of 0 so I didn't catch this in testing :)
>> +	rc = apei_write(val, &generic_v2->read_ack_register);
>> +
>> +	return rc;
>> +}
>> +
>>   static int ghes_proc(struct ghes *ghes)
>>   {
>>   	int rc;
>> @@ -660,6 +695,12 @@ static int ghes_proc(struct ghes *ghes)
>>   			ghes_estatus_cache_add(ghes->generic, ghes->estatus);
>>   	}
>>   	ghes_do_proc(ghes, ghes->estatus);
>> +
>> +	if (ghes->generic_v2) {
>> +		rc = ghes_do_read_ack(ghes->generic_v2);
>> +		if (rc)
>> +			return rc;
>> +	}
>>   out:
>>   	ghes_clear_estatus(ghes);
>>   	return 0;
>> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
>> index 792a0d9..ef725a9 100644
>> --- a/drivers/acpi/apei/hest.c
>> +++ b/drivers/acpi/apei/hest.c
>> @@ -52,6 +52,7 @@ static const int hest_esrc_len_tab[ACPI_HEST_TYPE_RESERVED] = {
>>   	[ACPI_HEST_TYPE_AER_ENDPOINT] = sizeof(struct acpi_hest_aer),
>>   	[ACPI_HEST_TYPE_AER_BRIDGE] = sizeof(struct acpi_hest_aer_bridge),
>>   	[ACPI_HEST_TYPE_GENERIC_ERROR] = sizeof(struct acpi_hest_generic),
>> +	[ACPI_HEST_TYPE_GENERIC_ERROR_V2] = sizeof(struct acpi_hest_generic_v2),
>>   };
>>   
>>   static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
>> @@ -146,7 +147,8 @@ static int __init hest_parse_ghes_count(struct acpi_hest_header *hest_hdr, void
>>   {
>>   	int *count = data;
>>   
>> -	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR)
>> +	if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR ||
>> +	    hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR_V2)
>>   		(*count)++;
>>   	return 0;
>>   }
>> @@ -157,7 +159,8 @@ static int __init hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
>>   	struct ghes_arr *ghes_arr = data;
>>   	int rc, i;
>>   
>> -	if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR)
>> +	if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR &&
>> +	    hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR_V2)
>>   		return 0;
>>   
>>   	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
>> index 720446c..d0108b6 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -14,6 +14,7 @@
>>   
>>   struct ghes {
>>   	struct acpi_hest_generic *generic;
>> +	struct acpi_hest_generic_v2 *generic_v2;
> You either have a GHES or a GHESv2 structure. Instead of duplication,
> could this be represented as a union?
>
> Thanks,
> Punit
I think that should be doable. I'll make these changes in the next version.

Thanks,
Tyler
>>   	struct acpi_hest_generic_status *estatus;
>>   	u64 buffer_paddr;
>>   	unsigned long flags;

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 04/10] arm64: exception: handle Synchronous External Abort

[Baicar, Tyler]

Hello Punit,

On 10/12/2016 11:46 AM, Punit Agrawal wrote:
> Hi Tyler,
>
> A couple of hopefully not bike shedding comments below.
>
> Tyler Baicar <tbaicar@codeaurora.org> writes:
>
>> SEA exceptions are often caused by an uncorrected hardware
>> error, and are handled when data abort and instruction abort
>> exception classes have specific values for their Fault Status
>> Code.
>> When SEA occurs, before killing the process, go through
>> the handlers registered in the notification list.
>> Update fault_info[] with specific SEA faults so that the
>> new SEA handler is used.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>> ---
>>   arch/arm64/include/asm/system_misc.h | 13 ++++++++
>>   arch/arm64/mm/fault.c                | 58 +++++++++++++++++++++++++++++-------
>>   2 files changed, 61 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
>> index 57f110b..90daf4a 100644
>> --- a/arch/arm64/include/asm/system_misc.h
>> +++ b/arch/arm64/include/asm/system_misc.h
>> @@ -64,4 +64,17 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>>   
>>   #endif	/* __ASSEMBLY__ */
>>   
>> +/*
>> + * The functions below are used to register and unregister callbacks
>> + * that are to be invoked when a Synchronous External Abort (SEA)
>> + * occurs. An SEA is raised by certain fault status codes that have
>> + * either data or instruction abort as the exception class, and
>> + * callbacks may be registered to parse or handle such hardware errors.
>> + *
>> + * Registered callbacks are run in an interrupt/atomic context. They
>> + * are not allowed to block or sleep.
>> + */
>> +int sea_register_handler_chain(struct notifier_block *nb);
>> +void sea_unregister_handler_chain(struct notifier_block *nb);
>> +
>>   #endif	/* __ASM_SYSTEM_MISC_H */
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 05d2bd7..81cb7ad 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -39,6 +39,22 @@
>>   #include <asm/pgtable.h>
>>   #include <asm/tlbflush.h>
>>   
>> +/*
>> + * GHES SEA handler code may register a notifier call here to
>> + * handle HW error record passed from platform.
>> + */
>> +static ATOMIC_NOTIFIER_HEAD(sea_handler_chain);
>> +
>> +int sea_register_handler_chain(struct notifier_block *nb)
>> +{
>> +	return atomic_notifier_chain_register(&sea_handler_chain, nb);
>> +}
>> +
>> +void sea_unregister_handler_chain(struct notifier_block *nb)
>> +{
>> +	atomic_notifier_chain_unregister(&sea_handler_chain, nb);
>> +}
>> +
> What do you think of naming the above functions as
> [un]register_synchonous_ext_abort_notifier?
>
> For an API, I find "sea" doesn't quite convey the message.
>
> One more comment below.
Yes, those names seem easier to understand.
>>   static const char *fault_name(unsigned int esr);
>>   
>>   #ifdef CONFIG_KPROBES
>> @@ -480,6 +496,28 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   	return 1;
>>   }
>>   
>> +/*
>> + * This abort handler deals with Synchronous External Abort.
>> + * It calls notifiers, and then returns "fault".
>> + */
>> +static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>> +{
>> +	struct siginfo info;
>> +
>> +	atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
>> +
>> +	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>> +		 fault_name(esr), esr, addr);
>> +
>> +	info.si_signo = SIGBUS;
>> +	info.si_errno = 0;
>> +	info.si_code  = 0;
>> +	info.si_addr  = (void __user *)addr;
>> +	arm64_notify_die("", regs, &info, esr);
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct fault_info {
>>   	int	(*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
>>   	int	sig;
>> @@ -502,22 +540,22 @@ static const struct fault_info {
>>   	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 1 permission fault"	},
>>   	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 2 permission fault"	},
>>   	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 3 permission fault"	},
>> -	{ do_bad,		SIGBUS,  0,		"synchronous external abort"	},
>> +	{ do_sea,		SIGBUS,  0,		"synchronous external abort"	},
>>   	{ do_bad,		SIGBUS,  0,		"unknown 17"			},
>>   	{ do_bad,		SIGBUS,  0,		"unknown 18"			},
>>   	{ do_bad,		SIGBUS,  0,		"unknown 19"			},
>> -	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
>> -	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
>> -	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
>> -	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
>> -	{ do_bad,		SIGBUS,  0,		"synchronous parity error"	},
>> +	{ do_sea,		SIGBUS,  0,		"level 0 SEA (trans tbl walk)"	},
>> +	{ do_sea,		SIGBUS,  0,		"level 1 SEA (trans tbl walk)"	},
>> +	{ do_sea,		SIGBUS,  0,		"level 2 SEA (trans tbl walk)"	},
>> +	{ do_sea,		SIGBUS,  0,		"level 3 SEA (trans tbl walk)"	},
>                                                                   ^^^
> The comment about naming applies here as well.
>
> Thanks,
> Punit
I'll expand sea here as well. This should make it easier to understand 
without knowing the code.

Thanks,
Tyler
>
> [...]
>

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Baicar, Tyler]

Hello Punit,


On 10/12/2016 12:00 PM, Punit Agrawal wrote:
> Tyler Baicar <tbaicar@codeaurora.org> writes:
>
>> ARM APEI extension proposal added SEA (Synchrounous External
>> Abort) notification type for ARMv8.
>> Add a new GHES error source handling function for SEA. If an error
>> source's notification type is SEA, then this function can be registered
>> into the SEA exception handler. That way GHES will parse and report
>> SEA exceptions when they occur.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> This patch fails to apply for me on v4.8. Is there a different tree this
> is based on?
This patch was giving me some grief as well. I'm not sure why that is 
because this patchset was based on the 4.8 kernel with the dependent 
patch for initial APEI support.
> One comment below.
>
> [...]
>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index c8488f1..28d5a09 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -50,6 +50,10 @@
>>   #include <acpi/apei.h>
>>   #include <asm/tlbflush.h>
>>   
>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
>> +#include <asm/system_misc.h>
>> +#endif
>> +
>>   #include "apei-internal.h"
>>   
>>   #define GHES_PFX	"GHES: "
>> @@ -779,6 +783,62 @@ static struct notifier_block ghes_notifier_sci = {
>>   	.notifier_call = ghes_notify_sci,
>>   };
>>   
>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
>> +static LIST_HEAD(ghes_sea);
>> +
>> +static int ghes_notify_sea(struct notifier_block *this,
>> +				  unsigned long event, void *data)
>> +{
>> +	struct ghes *ghes;
>> +	int ret = NOTIFY_DONE;
>> +
>> +	rcu_read_lock();
>> +	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
>> +		if (!ghes_proc(ghes))
>> +			ret = NOTIFY_OK;
> Not something you've introduced but it looks like ghes_proc erroneously
> never returns anything other than 0. I plan to post the below fix to
> address it.
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 60746ef..caea575 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes)
>          ghes_do_proc(ghes, ghes->estatus);
>   out:
>          ghes_clear_estatus(ghes);
> -   return 0;
> + return rc;
>   }
Yes, this definitely should be fixed :)

Thanks,
Tyler
>> +	}
>> +	rcu_read_unlock();
>> +
>> +	return ret;
>> +}
>> +
> [...]
>

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Baicar, Tyler]

Hello Suzuki,

On 10/13/2016 2:50 AM, Suzuki K Poulose wrote:
> On 12/10/16 23:10, Baicar, Tyler wrote:
>> Hello Suzuki,
>>
>> Thank you for the feedback! Responses below.
>>
>> On 10/11/2016 11:28 AM, Suzuki K Poulose wrote:
>>> On 07/10/16 22:31, Tyler Baicar wrote:
>>>> Currently when a RAS error is reported it is not timestamped.
>>>> The ACPI 6.1 spec adds the timestamp field to the generic error
>>>> data entry v3 structure. The timestamp of when the firmware
>>>> generated the error is now being reported.
>>>>
>>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>>>> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
>>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>>>
>>> Please could you keep the people who reviewed/commented on your 
>>> series in the past,
>>> whenever you post a new version ?
>> Do you mean to just send the new version to their e-mail directly in 
>> addition to the lists? If so, I will do that next time.
>
> If you send a new version of a series to the list, it is a good idea 
> to keep
> the people who commented (significantly) on your previous version in 
> Cc, especially
> when you have addressed their feedback. That will help them to keep 
> track of the
> series. People can always see the new version in the list, but then it 
> is so easy
> to miss something in the 100s of emails you get each day. I am sure 
> people have
> special filters for the emails based on if they are in Cc/To etc.
>
Okay, understood. I'll make sure to add those who have commented in the 
cc/to list to avoid the e-mail filters.
>>
>> I know you provided good feedback on the previous patchset, but I did 
>> not have anyone specifically respond to add "reviewed-by:...". I 
>> don't think I should add reviewed-by for someone unless they 
>> specifically add it in a response :)
>
> No, I haven't yet "Reviewed-by" your patches. I had some comments on 
> it, which means
> I expected it to be addressed as you committed in your response.
>
>>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>>> index 3021f0e..c8488f1 100644
>>>> --- a/drivers/acpi/apei/ghes.c
>>>> +++ b/drivers/acpi/apei/ghes.c
>>>> @@ -80,6 +80,10 @@
>
>> I think that should work to avoid duplication. I will move them to a 
>> header file in the next patchset.
>>>> +
>>>> +static void cper_estatus_print_section_v300(const char *pfx,
>>>> +    const struct acpi_hest_generic_data_v300 *gdata)
>>>> +{
>>>> +    __u8 hour, min, sec, day, mon, year, century, *timestamp;
>>>> +
>>>> +    if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>>>> +        timestamp = (__u8 *)&(gdata->time_stamp);
>>>> +        memcpy(&sec, timestamp, 1);
>>>> +        memcpy(&min, timestamp + 1, 1);
>>>> +        memcpy(&hour, timestamp + 2, 1);
>>>> +        memcpy(&day, timestamp + 4, 1);
>>>> +        memcpy(&mon, timestamp + 5, 1);
>>>> +        memcpy(&year, timestamp + 6, 1);
>>>> +        memcpy(&century, timestamp + 7, 1);
>>>> +        printk("%stime: ", pfx);
>>>> +        printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>>>
>>> What format is the (timestamp + 3) stored in ? Does it need 
>>> conversion ?
>> The third byte of the timestamp is currently only used to determine 
>> if the time is precise or not. Bit 0 is used to specify that and the 
>> other bits in this byte are marked as reserved. This is shown in 
>> table 247 of the UEFI spec 2.6:
>>
>> Byte 3:
>>   Bit 0 – Timestamp is precise if this bit is set and correlates to 
>> the time of the error event.
>>   Bit 7:1 – Reserved
>
> Is it always the same endianness as that of the CPU ?
It is a fair assumption that the firmware populating this record will 
use a CPU of the same endianness. There is no mechanism in the spec to 
indicate otherwise.

Thanks,
Tyler
>
> Cheers
> Suzuki
>
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 10/10] arm64: KVM: add guest SEA support

[Baicar, Tyler]

Hello Punit,

On 10/13/2016 7:14 AM, Punit Agrawal wrote:
> Hi Tyler,
>
> I know I've had my last comment already ;), but I thought I'd rather
> raise the question than stay confused...
>
> Tyler Baicar <tbaicar-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>
>> Currently external aborts are unsupported by the guest abort
>> handling. Add handling for SEAs so that the host kernel reports
>> SEAs which occur in the guest kernel.
>>
>> Signed-off-by: Tyler Baicar <tbaicar-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>   arch/arm/include/asm/kvm_arm.h       |  1 +
>>   arch/arm/include/asm/system_misc.h   |  5 +++++
>>   arch/arm/kvm/mmu.c                   | 15 +++++++++++++--
>>   arch/arm64/include/asm/kvm_arm.h     |  1 +
>>   arch/arm64/include/asm/system_misc.h |  2 ++
>>   arch/arm64/mm/fault.c                | 13 +++++++++++++
>>   6 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
>> index e22089f..33a77509 100644
>> --- a/arch/arm/include/asm/kvm_arm.h
>> +++ b/arch/arm/include/asm/kvm_arm.h
>> @@ -187,6 +187,7 @@
>>   #define FSC_FAULT	(0x04)
>>   #define FSC_ACCESS	(0x08)
>>   #define FSC_PERM	(0x0c)
>> +#define FSC_EXTABT	(0x10)
>>   
>>   /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>   #define HPFAR_MASK	(~0xf)
>> diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
>> index a3d61ad..86e1faa 100644
>> --- a/arch/arm/include/asm/system_misc.h
>> +++ b/arch/arm/include/asm/system_misc.h
>> @@ -24,4 +24,9 @@ extern unsigned int user_debug;
>>   
>>   #endif /* !__ASSEMBLY__ */
>>   
>> +inline int handle_guest_sea(unsigned long addr, unsigned int esr)
>> +{
>> +	return -1;
>> +}
>> +
>>   #endif /* __ASM_ARM_SYSTEM_MISC_H */
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index e9a5c0e..24cde84 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -29,6 +29,7 @@
>>   #include <asm/kvm_asm.h>
>>   #include <asm/kvm_emulate.h>
>>   #include <asm/virt.h>
>> +#include <asm/system_misc.h>
>>   
>>   #include "trace.h"
>>   
>> @@ -1441,8 +1442,18 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>   
>>   	/* Check the stage-2 fault is trans. fault or write fault */
>>   	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>> -	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
>> -	    fault_status != FSC_ACCESS) {
>> +
>> +	if (fault_status == FSC_EXTABT) {
>> +		if(handle_guest_sea((unsigned long)fault_ipa,
>> +				    kvm_vcpu_get_hsr(vcpu))) {
>> +			kvm_err("Failed to handle guest SEA, FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>> +				kvm_vcpu_trap_get_class(vcpu),
>> +				(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
>> +				(unsigned long)kvm_vcpu_get_hsr(vcpu));
>> +			return -EFAULT;
>> +		}
>> +	} else if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
>> +		   fault_status != FSC_ACCESS) {
>>   		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>>   			kvm_vcpu_trap_get_class(vcpu),
>>   			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>> index 4b5c977..be0efb6 100644
>> --- a/arch/arm64/include/asm/kvm_arm.h
>> +++ b/arch/arm64/include/asm/kvm_arm.h
>> @@ -201,6 +201,7 @@
>>   #define FSC_FAULT	ESR_ELx_FSC_FAULT
>>   #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
>>   #define FSC_PERM	ESR_ELx_FSC_PERM
>> +#define FSC_EXTABT	ESR_ELx_FSC_EXTABT
>>   
>>   /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>   #define HPFAR_MASK	(~UL(0xf))
>> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
>> index 90daf4a..8522fff 100644
>> --- a/arch/arm64/include/asm/system_misc.h
>> +++ b/arch/arm64/include/asm/system_misc.h
>> @@ -77,4 +77,6 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>>   int sea_register_handler_chain(struct notifier_block *nb);
>>   void sea_unregister_handler_chain(struct notifier_block *nb);
>>   
>> +int handle_guest_sea(unsigned long addr, unsigned int esr);
>> +
>>   #endif	/* __ASM_SYSTEM_MISC_H */
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 81cb7ad..d714432 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -597,6 +597,19 @@ static const char *fault_name(unsigned int esr)
>>   }
>>   
>>   /*
>> + * Handle Synchronous External Aborts that occur in a guest kernel.
>> + */
>> +int handle_guest_sea(unsigned long addr, unsigned int esr)
>> +{
>> +	atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
>> +
>> +	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>> +		fault_name(esr), esr, addr);
>> +
>> +	return 0;
>> +}
> Don't we need to pass the abort to the guest?
This requires some infrastructure to implement virtual "ACPI platform 
error interface" to expose the details of the abort to the guest. This 
patchset does not cover that and focuses on feature parity with other 
architectures that support APEI. There are discussions among Linaro 
partners about how this can be achieved in the long term, but that work 
is outside the scope of this patchset. This patch will ensure that if a 
guest encounters one of these errors then it will be reported before 
getting killed. Before this patch we would just get an unsupported FSC 
print out and then the guest would be killed.

Thanks,
Tyler
>
> Thanks,
> Punit
>
>> +
>> +/*
>>    * Dispatch a data abort to the relevant handler.
>>    */
>>   asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section

[Baicar, Tyler]

Hello Punit,

On 10/13/2016 4:54 AM, Punit Agrawal wrote:
> Hi Tyler,
>
> One last comment...
>
> Tyler Baicar <tbaicar@codeaurora.org> writes:
>
>> UEFI spec allows for non-standard section in Common Platform Error
>> Record. This is defined in section N.2.3 of UEFI version 2.5.
>>
>> Currently if the CPER section's type (UUID) does not match with
>> any section type that the kernel knows how to parse, trace event
>> is not generated for such section. And thus user is not able to know
>> happening of such hardware error, including error record of
>> non-standard section.
>>
>> This commit generates a trace event which contains raw error data
>> for unrecognized CPER section.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> ---
>>   drivers/acpi/apei/ghes.c | 18 +++++++++++++++++-
>>   drivers/ras/ras.c        |  1 +
>>   include/ras/ras_event.h  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 36894c8..cb4c7f4 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -49,6 +49,7 @@
>>   #include <acpi/ghes.h>
>>   #include <acpi/apei.h>
>>   #include <asm/tlbflush.h>
>> +#include <ras/ras_event.h>
>>   
>>   #ifdef CONFIG_HAVE_ACPI_APEI_SEA
>>   #include <asm/system_misc.h>
>> @@ -468,12 +469,21 @@ static void ghes_do_proc(struct ghes *ghes,
>>   	int sev, sec_sev;
>>   	struct acpi_hest_generic_data *gdata;
>>   	uuid_le sec_type;
>> +	uuid_le *fru_id;
>> +	char *fru_text = "";
>>   
>>   	sev = ghes_severity(estatus->error_severity);
>>   	apei_estatus_for_each_section(estatus, gdata) {
>>   		sec_sev = ghes_severity(gdata->error_severity);
>>   		sec_type = *(uuid_le *)gdata->section_type;
>>   
>> +		if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
>> +			fru_id = (uuid_le *)gdata->fru_id;
>> +		else
>> +			fru_id = &NULL_UUID_LE;
> fru_id can be initialised at declaration and drop the else here. The
> same is already being done for fru_text.
Yes, I will make this change in the next version.

Thanks,
Tyler
> Thanks,
> Punit
>
>> +		if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
>> +			fru_text = gdata->fru_text;
>> +
>>   		if (!uuid_le_cmp(sec_type,
>>   				 CPER_SEC_PLATFORM_MEM)) {
>>   			struct cper_sec_mem_err *mem_err;
> [...]
>

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 06/10] acpi: apei: panic OS with fatal error status block

[Baicar, Tyler]

Hello Suzuki,

On 10/13/2016 7:00 AM, Suzuki K Poulose wrote:
> On 07/10/16 22:31, Tyler Baicar wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
>>
>> Even if an error status block's severity is fatal, the kernel does not
>> honor the severity level and panic.
>>
>> With the firmware first model, the platform could inform the OS about a
>> fatal hardware error through the non-NMI GHES notification type. The OS
>> should panic when a hardware error record is received with this
>> severity.
>>
>> Call panic() after CPER data in error status block is printed if
>> severity is fatal, before each error section is handled.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> ---
>>  drivers/acpi/apei/ghes.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 28d5a09..36894c8 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -141,6 +141,8 @@ static unsigned long ghes_estatus_pool_size_request;
>>  static struct ghes_estatus_cache 
>> *ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
>>  static atomic_t ghes_estatus_cache_alloced;
>>
>> +static int ghes_panic_timeout __read_mostly = 30;
>> +
>>  static int ghes_ioremap_init(void)
>>  {
>>      ghes_ioremap_area = __get_vm_area(PAGE_SIZE * GHES_IOREMAP_PAGES,
>> @@ -715,6 +717,12 @@ static int ghes_proc(struct ghes *ghes)
>>          if (ghes_print_estatus(NULL, ghes->generic, ghes->estatus))
>>              ghes_estatus_cache_add(ghes->generic, ghes->estatus);
>>      }
>> +    if (ghes_severity(ghes->estatus->error_severity) >= 
>> GHES_SEV_PANIC) {
>> +        if (panic_timeout == 0)
>> +            panic_timeout = ghes_panic_timeout;
>> +        panic("Fatal hardware error!");
>
> I think there is a chance that we might miss the o/p of 
> ghes_print_estatus() as we use
> no pfx, and it could default to the normal loglevel and would never 
> get printed
> if panic() is encountered before it. On the other hand, there is 
> already a
> __ghes_panic() which does similar stuff. Is there a way we could reuse
> (may be even parts of) it ? Or at least use KERN_EMERG for the 
> ghes_print_estatus(),
> if the severity could result in panic() ?
__ghes_panic() does additional handling which we do not want to do here. 
I could make the following a helper function so it is not duplicated though:

if (panic_timeout == 0)
     panic_timeout = ghes_panic_timeout;
panic("Fatal hardware error!");

The pfx is actually being calculated already in __ghes_print_estatus():

         if (pfx == NULL) {
                 if (ghes_severity(estatus->error_severity) <=
                     GHES_SEV_CORRECTED)
                         pfx = KERN_WARNING;
                 else
                         pfx = KERN_ERR;
         }

 From ghes.h:

enum {
         GHES_SEV_NO = 0x0,
         GHES_SEV_CORRECTED = 0x1,
         GHES_SEV_RECOVERABLE = 0x2,
         GHES_SEV_PANIC = 0x3,
};

This will make the pfx KERN_ERR for the case of a panic.

Thanks,
Tyler
>
> Cheers
> Suzuki
>

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 10/10] arm64: KVM: add guest SEA support

[Punit Agrawal]

"Baicar, Tyler" <tbaicar@codeaurora.org> writes:

> Hello Punit,
>
> On 10/13/2016 7:14 AM, Punit Agrawal wrote:
>> Hi Tyler,
>>
>> I know I've had my last comment already ;), but I thought I'd rather
>> raise the question than stay confused...
>>
>> Tyler Baicar <tbaicar@codeaurora.org> writes:
>>
>>> Currently external aborts are unsupported by the guest abort
>>> handling. Add handling for SEAs so that the host kernel reports
>>> SEAs which occur in the guest kernel.
>>>
>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>> ---
>>>   arch/arm/include/asm/kvm_arm.h       |  1 +
>>>   arch/arm/include/asm/system_misc.h   |  5 +++++
>>>   arch/arm/kvm/mmu.c                   | 15 +++++++++++++--
>>>   arch/arm64/include/asm/kvm_arm.h     |  1 +
>>>   arch/arm64/include/asm/system_misc.h |  2 ++
>>>   arch/arm64/mm/fault.c                | 13 +++++++++++++
>>>   6 files changed, 35 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
>>> index e22089f..33a77509 100644
>>> --- a/arch/arm/include/asm/kvm_arm.h
>>> +++ b/arch/arm/include/asm/kvm_arm.h
>>> @@ -187,6 +187,7 @@
>>>   #define FSC_FAULT	(0x04)
>>>   #define FSC_ACCESS	(0x08)
>>>   #define FSC_PERM	(0x0c)
>>> +#define FSC_EXTABT	(0x10)
>>>     /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>>   #define HPFAR_MASK	(~0xf)
>>> diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
>>> index a3d61ad..86e1faa 100644
>>> --- a/arch/arm/include/asm/system_misc.h
>>> +++ b/arch/arm/include/asm/system_misc.h
>>> @@ -24,4 +24,9 @@ extern unsigned int user_debug;
>>>     #endif /* !__ASSEMBLY__ */
>>>   +inline int handle_guest_sea(unsigned long addr, unsigned int
>>> esr)
>>> +{
>>> +	return -1;
>>> +}
>>> +
>>>   #endif /* __ASM_ARM_SYSTEM_MISC_H */
>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>>> index e9a5c0e..24cde84 100644
>>> --- a/arch/arm/kvm/mmu.c
>>> +++ b/arch/arm/kvm/mmu.c
>>> @@ -29,6 +29,7 @@
>>>   #include <asm/kvm_asm.h>
>>>   #include <asm/kvm_emulate.h>
>>>   #include <asm/virt.h>
>>> +#include <asm/system_misc.h>
>>>     #include "trace.h"
>>>   @@ -1441,8 +1442,18 @@ int kvm_handle_guest_abort(struct kvm_vcpu
>>> *vcpu, struct kvm_run *run)
>>>     	/* Check the stage-2 fault is trans. fault or write fault */
>>>   	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>>> -	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
>>> -	    fault_status != FSC_ACCESS) {
>>> +
>>> +	if (fault_status == FSC_EXTABT) {
>>> +		if(handle_guest_sea((unsigned long)fault_ipa,
>>> +				    kvm_vcpu_get_hsr(vcpu))) {
>>> +			kvm_err("Failed to handle guest SEA, FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>>> +				kvm_vcpu_trap_get_class(vcpu),
>>> +				(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
>>> +				(unsigned long)kvm_vcpu_get_hsr(vcpu));
>>> +			return -EFAULT;
>>> +		}
>>> +	} else if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
>>> +		   fault_status != FSC_ACCESS) {
>>>   		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>>>   			kvm_vcpu_trap_get_class(vcpu),
>>>   			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>> index 4b5c977..be0efb6 100644
>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>> @@ -201,6 +201,7 @@
>>>   #define FSC_FAULT	ESR_ELx_FSC_FAULT
>>>   #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
>>>   #define FSC_PERM	ESR_ELx_FSC_PERM
>>> +#define FSC_EXTABT	ESR_ELx_FSC_EXTABT
>>>     /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>>   #define HPFAR_MASK	(~UL(0xf))
>>> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
>>> index 90daf4a..8522fff 100644
>>> --- a/arch/arm64/include/asm/system_misc.h
>>> +++ b/arch/arm64/include/asm/system_misc.h
>>> @@ -77,4 +77,6 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>>>   int sea_register_handler_chain(struct notifier_block *nb);
>>>   void sea_unregister_handler_chain(struct notifier_block *nb);
>>>   +int handle_guest_sea(unsigned long addr, unsigned int esr);
>>> +
>>>   #endif	/* __ASM_SYSTEM_MISC_H */
>>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>>> index 81cb7ad..d714432 100644
>>> --- a/arch/arm64/mm/fault.c
>>> +++ b/arch/arm64/mm/fault.c
>>> @@ -597,6 +597,19 @@ static const char *fault_name(unsigned int esr)
>>>   }
>>>     /*
>>> + * Handle Synchronous External Aborts that occur in a guest kernel.
>>> + */
>>> +int handle_guest_sea(unsigned long addr, unsigned int esr)
>>> +{
>>> +	atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
>>> +
>>> +	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>>> +		fault_name(esr), esr, addr);
>>> +
>>> +	return 0;
>>> +}
>> Don't we need to pass the abort to the guest?
> This requires some infrastructure to implement virtual "ACPI platform
> error interface" to expose the details of the abort to the guest. This
> patchset does not cover that and focuses on feature parity with other
> architectures that support APEI. There are discussions among Linaro
> partners about how this can be achieved in the long term, but that
> work is outside the scope of this patchset. This patch will ensure
> that if a guest encounters one of these errors then it will be
> reported before getting killed. Before this patch we would just get an
> unsupported FSC print out and then the guest would be killed.

OK.

I think we might be talking about different things though.

I am referring to the injection of the synchronous external abort into
the guest - similar to what's been done for prefetch abort in the
kvm_guest_handle_abort.

Maybe there is no benefit in passing the abort to the guest. In that
case, can you please add a comment where you handle external abort
(FSC_EXTABT) in kvm_guest_handle_abort.

>
> Thanks,
> Tyler
>>
>> Thanks,
>> Punit
>>
>>> +
>>> +/*
>>>    * Dispatch a data abort to the relevant handler.
>>>    */
>>>   asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,

Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Punit Agrawal]

"Baicar, Tyler" <tbaicar@codeaurora.org> writes:

> Hello Punit,
>
>
> On 10/12/2016 12:00 PM, Punit Agrawal wrote:
>> Tyler Baicar <tbaicar@codeaurora.org> writes:
>>
>>> ARM APEI extension proposal added SEA (Synchrounous External
>>> Abort) notification type for ARMv8.
>>> Add a new GHES error source handling function for SEA. If an error
>>> source's notification type is SEA, then this function can be registered
>>> into the SEA exception handler. That way GHES will parse and report
>>> SEA exceptions when they occur.
>>>
>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>> This patch fails to apply for me on v4.8. Is there a different tree this
>> is based on?
> This patch was giving me some grief as well. I'm not sure why that is
> because this patchset was based on the 4.8 kernel with the dependent
> patch for initial APEI support.

That explains it!. I've missed out the dependency called out in the
cover letter.


>> One comment below.
>>
>> [...]
>>
>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index c8488f1..28d5a09 100644
>>> --- a/drivers/acpi/apei/ghes.c
>>> +++ b/drivers/acpi/apei/ghes.c
>>> @@ -50,6 +50,10 @@
>>>   #include <acpi/apei.h>
>>>   #include <asm/tlbflush.h>
>>>   +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
>>> +#include <asm/system_misc.h>
>>> +#endif
>>> +
>>>   #include "apei-internal.h"
>>>     #define GHES_PFX	"GHES: "
>>> @@ -779,6 +783,62 @@ static struct notifier_block ghes_notifier_sci = {
>>>   	.notifier_call = ghes_notify_sci,
>>>   };
>>>   +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
>>> +static LIST_HEAD(ghes_sea);
>>> +
>>> +static int ghes_notify_sea(struct notifier_block *this,
>>> +				  unsigned long event, void *data)
>>> +{
>>> +	struct ghes *ghes;
>>> +	int ret = NOTIFY_DONE;
>>> +
>>> +	rcu_read_lock();
>>> +	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
>>> +		if (!ghes_proc(ghes))
>>> +			ret = NOTIFY_OK;
>> Not something you've introduced but it looks like ghes_proc erroneously
>> never returns anything other than 0. I plan to post the below fix to
>> address it.
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 60746ef..caea575 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes)
>>          ghes_do_proc(ghes, ghes->estatus);
>>   out:
>>          ghes_clear_estatus(ghes);
>> -   return 0;
>> + return rc;
>>   }
> Yes, this definitely should be fixed :)
>
> Thanks,
> Tyler
>>> +	}
>>> +	rcu_read_unlock();
>>> +
>>> +	return ret;
>>> +}
>>> +
>> [...]
>>

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Suzuki K Poulose]

On 13/10/16 20:37, Baicar, Tyler wrote:
> Hello Suzuki,
>
> On 10/13/2016 2:50 AM, Suzuki K Poulose wrote:
>> On 12/10/16 23:10, Baicar, Tyler wrote:

>>>> Please could you keep the people who reviewed/commented on your series in the past,
>>>> whenever you post a new version ?
>>> Do you mean to just send the new version to their e-mail directly in addition to the lists? If so, I will do that next time.
>>
>> If you send a new version of a series to the list, it is a good idea to keep
>> the people who commented (significantly) on your previous version in Cc, especially
>> when you have addressed their feedback. That will help them to keep track of the
>> series. People can always see the new version in the list, but then it is so easy
>> to miss something in the 100s of emails you get each day. I am sure people have
>> special filters for the emails based on if they are in Cc/To etc.
>>
> Okay, understood. I'll make sure to add those who have commented in the cc/to list to avoid the e-mail filters.

Thanks !

>> Is it always the same endianness as that of the CPU ?
> It is a fair assumption that the firmware populating this record will use a CPU of the same endianness. There is no mechanism in the spec to indicate otherwise.

Yep, you are right. The EFI expects the EL2/EL1 to be of the same endianness

Cheers
Suzuki

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Mark Rutland]

On Fri, Oct 14, 2016 at 05:28:58PM +0100, Suzuki K Poulose wrote:
> On 13/10/16 20:37, Baicar, Tyler wrote:
> >On 10/13/2016 2:50 AM, Suzuki K Poulose wrote:
> >>Is it always the same endianness as that of the CPU ?
> >
> >It is a fair assumption that the firmware populating this record will
> >use a CPU of the same endianness. There is no mechanism in the spec
> >to indicate otherwise.
> 
> Yep, you are right. The EFI expects the EL2/EL1 to be of the same endianness

To be clear, it is specifically required in the ACPI spec that elements
are in little-endian. Per the ACPI 6.1 spec, page 109:

	All numeric values in ACPI-defined tables, blocks, and
	structures are always encoded in little endian
	format.

Given that CPER, HEST, etc are defined within the ACPI specification,
they are covered by this requirement.

Elements outside of the ACPI spec are not necessarily covered by this
requirement, but their specifications should state their endianness.

Thanks,
Mark.

Re: [PATCH V3 10/10] arm64: KVM: add guest SEA support

[Baicar, Tyler]

On 10/14/2016 2:38 AM, Punit Agrawal wrote:
> "Baicar, Tyler" <tbaicar@codeaurora.org> writes:
>
>> Hello Punit,
>>
>> On 10/13/2016 7:14 AM, Punit Agrawal wrote:
>>> Hi Tyler,
>>>
>>> I know I've had my last comment already ;), but I thought I'd rather
>>> raise the question than stay confused...
>>>
>>> Tyler Baicar <tbaicar@codeaurora.org> writes:
>>>
>>>> Currently external aborts are unsupported by the guest abort
>>>> handling. Add handling for SEAs so that the host kernel reports
>>>> SEAs which occur in the guest kernel.
>>>>
>>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>>> ---
>>>>    arch/arm/include/asm/kvm_arm.h       |  1 +
>>>>    arch/arm/include/asm/system_misc.h   |  5 +++++
>>>>    arch/arm/kvm/mmu.c                   | 15 +++++++++++++--
>>>>    arch/arm64/include/asm/kvm_arm.h     |  1 +
>>>>    arch/arm64/include/asm/system_misc.h |  2 ++
>>>>    arch/arm64/mm/fault.c                | 13 +++++++++++++
>>>>    6 files changed, 35 insertions(+), 2 deletions(-)
[...]
>>>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>>>> index 81cb7ad..d714432 100644
>>>> --- a/arch/arm64/mm/fault.c
>>>> +++ b/arch/arm64/mm/fault.c
>>>> @@ -597,6 +597,19 @@ static const char *fault_name(unsigned int esr)
>>>>    }
>>>>      /*
>>>> + * Handle Synchronous External Aborts that occur in a guest kernel.
>>>> + */
>>>> +int handle_guest_sea(unsigned long addr, unsigned int esr)
>>>> +{
>>>> +	atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
>>>> +
>>>> +	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>>>> +		fault_name(esr), esr, addr);
>>>> +
>>>> +	return 0;
>>>> +}
>>> Don't we need to pass the abort to the guest?
>> This requires some infrastructure to implement virtual "ACPI platform
>> error interface" to expose the details of the abort to the guest. This
>> patchset does not cover that and focuses on feature parity with other
>> architectures that support APEI. There are discussions among Linaro
>> partners about how this can be achieved in the long term, but that
>> work is outside the scope of this patchset. This patch will ensure
>> that if a guest encounters one of these errors then it will be
>> reported before getting killed. Before this patch we would just get an
>> unsupported FSC print out and then the guest would be killed.
> OK.
>
> I think we might be talking about different things though.
>
> I am referring to the injection of the synchronous external abort into
> the guest - similar to what's been done for prefetch abort in the
> kvm_guest_handle_abort.
>
> Maybe there is no benefit in passing the abort to the guest. In that
> case, can you please add a comment where you handle external abort
> (FSC_EXTABT) in kvm_guest_handle_abort.
Yes, I will add a comment there in the next version.

Thanks,
Tyler

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Hanjun Guo]

Hi Tyler,

On 2016/10/8 5:31, Tyler Baicar wrote:
> ARM APEI extension proposal added SEA (Synchrounous External
> Abort) notification type for ARMv8.
> Add a new GHES error source handling function for SEA. If an error
> source's notification type is SEA, then this function can be registered
> into the SEA exception handler. That way GHES will parse and report
> SEA exceptions when they occur.

Does this SEA is replayed by the firmware (firmware first handling)
or directly triggered by the hardware when error is happened?

Thanks
Hanjun

Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Hanjun Guo]

On 2016/10/8 5:31, Tyler Baicar wrote:
> ARM APEI extension proposal added SEA (Synchrounous External
> Abort) notification type for ARMv8.
> Add a new GHES error source handling function for SEA. If an error
> source's notification type is SEA, then this function can be registered
> into the SEA exception handler. That way GHES will parse and report
> SEA exceptions when they occur.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Tyler Baicar <tbaicar-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Naveen Kaje <nkaje-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  arch/arm64/Kconfig        |  1 +
>  drivers/acpi/apei/Kconfig | 15 +++++++++
>  drivers/acpi/apei/ghes.c  | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 99 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b380c87..ae34349 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -53,6 +53,7 @@ config ARM64
>  	select HANDLE_DOMAIN_IRQ
>  	select HARDIRQS_SW_RESEND
>  	select HAVE_ACPI_APEI if (ACPI && EFI)
> +	select HAVE_ACPI_APEI_SEA if (ACPI && EFI)
>  	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
>  	select HAVE_ARCH_AUDITSYSCALL
>  	select HAVE_ARCH_BITREVERSE
> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
> index b0140c8..fb99c1c 100644
> --- a/drivers/acpi/apei/Kconfig
> +++ b/drivers/acpi/apei/Kconfig
> @@ -4,6 +4,21 @@ config HAVE_ACPI_APEI
>  config HAVE_ACPI_APEI_NMI
>  	bool
>
> +config HAVE_ACPI_APEI_SEA
> +	bool "APEI Synchronous External Abort logging/recovering support"
> +	depends on ARM64
> +	help
> +	  This option should be enabled if the system supports
> +	  firmware first handling of SEA (Synchronous External Abort).
> +	  SEA happens with certain faults of data abort or instruction
> +	  abort synchronous exceptions on ARMv8 systems. If a system
> +	  supports firmware first handling of SEA, the platform analyzes
> +	  and handles hardware error notifications with SEA, and it may then
> +	  form a HW error record for the OS to parse and handle. This
> +	  option allows the OS to look for such HW error record, and
> +	  take appropriate action.

OK, I can see that it's firmware first handling, so it's triggered
by firmware to me, correct me if I'm wrong.

[...]
>  #ifdef CONFIG_HAVE_ACPI_APEI_NMI
>  /*
>   * printk is not safe in NMI context.  So in NMI handler, we allocate
> @@ -1023,6 +1083,14 @@ static int ghes_probe(struct platform_device *ghes_dev)
>  	case ACPI_HEST_NOTIFY_EXTERNAL:
>  	case ACPI_HEST_NOTIFY_SCI:
>  		break;
> +	case ACPI_HEST_NOTIFY_SEA:
> +		if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_SEA)) {
> +			pr_warn(GHES_PFX "Generic hardware error source: %d notified via SEA is not supported\n",
> +				generic->header.source_id);
> +			rc = -ENOTSUPP;
> +			goto err;
> +		}
> +		break;
>  	case ACPI_HEST_NOTIFY_NMI:
>  		if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_NMI)) {
>  			pr_warn(GHES_PFX "Generic hardware error source: %d notified via NMI interrupt is not supported!\n",
> @@ -1034,6 +1102,13 @@ static int ghes_probe(struct platform_device *ghes_dev)
>  		pr_warning(GHES_PFX "Generic hardware error source: %d notified via local interrupt is not supported!\n",
>  			   generic->header.source_id);
>  		goto err;
> +	case ACPI_HEST_NOTIFY_GPIO:
> +	case ACPI_HEST_NOTIFY_SEI:
> +	case ACPI_HEST_NOTIFY_GSIV:
> +		pr_warn(GHES_PFX "Generic hardware error source: %d notified via notification type %u is not supported\n",
> +			generic->header.source_id, generic->header.source_id);

Hmm, some platform may trigger a interrupt to OS for firmware handling
and it's in the ACPI 6.1 spec, is it a limitation now, or we need to
add code later to support it?

Thanks
Hanjun

Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Abdulhamid, Harb]

On 10/18/2016 8:44 AM, Hanjun Guo wrote:
> Hi Tyler,
> 
> On 2016/10/8 5:31, Tyler Baicar wrote:
>> ARM APEI extension proposal added SEA (Synchrounous External
>> Abort) notification type for ARMv8.
>> Add a new GHES error source handling function for SEA. If an error
>> source's notification type is SEA, then this function can be registered
>> into the SEA exception handler. That way GHES will parse and report
>> SEA exceptions when they occur.
> 
> Does this SEA is replayed by the firmware (firmware first handling)
> or directly triggered by the hardware when error is happened?

Architecturally, an SEA must be synchronous and *precise*, so if you
take an SEA on a particular load instruction, firmware/hardware should
not be corrupting the context/state of the PE to allow software to
determine which thread/process encountered the abort.  GHES error status
block will be expose to software with information about the type,
severity, physical address impacted.

Generally the error status block is populated by firmware.  However, as
long as the above requirement is met, I don't think the spec precludes
error status block being populated by hardware.  Those details must be
completely transparent to software.

Finally, to answer your more specific question:  If the implementation
of firmware-first involves trapping the SEA in EL3 to do some firmware
first handling, firmware must maintain the context of the offending ELx,
generate an error record, and then "replay" the exception to normal
(non-secure) software at the appropriate vector base address.

Thanks,
Harb
-- 
-- 
Qualcomm Datacenter Technologies, Inc.
as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Abdulhamid, Harb]

On 10/18/2016 9:04 AM, Hanjun Guo wrote:
> On 2016/10/8 5:31, Tyler Baicar wrote:
>> ARM APEI extension proposal added SEA (Synchrounous External
>> Abort) notification type for ARMv8.
>> Add a new GHES error source handling function for SEA. If an error
>> source's notification type is SEA, then this function can be registered
>> into the SEA exception handler. That way GHES will parse and report
>> SEA exceptions when they occur.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Signed-off-by: Tyler Baicar <tbaicar-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Signed-off-by: Naveen Kaje <nkaje-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>  arch/arm64/Kconfig        |  1 +
>>  drivers/acpi/apei/Kconfig | 15 +++++++++
>>  drivers/acpi/apei/ghes.c  | 83
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 99 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index b380c87..ae34349 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -53,6 +53,7 @@ config ARM64
>>      select HANDLE_DOMAIN_IRQ
>>      select HARDIRQS_SW_RESEND
>>      select HAVE_ACPI_APEI if (ACPI && EFI)
>> +    select HAVE_ACPI_APEI_SEA if (ACPI && EFI)
>>      select HAVE_ALIGNED_STRUCT_PAGE if SLUB
>>      select HAVE_ARCH_AUDITSYSCALL
>>      select HAVE_ARCH_BITREVERSE
>> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
>> index b0140c8..fb99c1c 100644
>> --- a/drivers/acpi/apei/Kconfig
>> +++ b/drivers/acpi/apei/Kconfig
>> @@ -4,6 +4,21 @@ config HAVE_ACPI_APEI
>>  config HAVE_ACPI_APEI_NMI
>>      bool
>>
>> +config HAVE_ACPI_APEI_SEA
>> +    bool "APEI Synchronous External Abort logging/recovering support"
>> +    depends on ARM64
>> +    help
>> +      This option should be enabled if the system supports
>> +      firmware first handling of SEA (Synchronous External Abort).
>> +      SEA happens with certain faults of data abort or instruction
>> +      abort synchronous exceptions on ARMv8 systems. If a system
>> +      supports firmware first handling of SEA, the platform analyzes
>> +      and handles hardware error notifications with SEA, and it may then
>> +      form a HW error record for the OS to parse and handle. This
>> +      option allows the OS to look for such HW error record, and
>> +      take appropriate action.
> 
> OK, I can see that it's firmware first handling, so it's triggered
> by firmware to me, correct me if I'm wrong.

Not exactly... the exception itself is *initially* triggered by the
processor itself (e.g. ECC error on a particular load causes a data
abort), but then may be intercepted by firmware (e.g. EL3) to generate
the error record and then be *replayed* back to software (e.g. jump to
appropriate EL and vector that originally caused the exception).

The reason we use the term "platform" here is because platform can be
hardware/firmware, and this can be implemented in different ways
depending on the preference of the platform vendor.  This is consistent
with the language in the UEFI/ACPI spec when describing the "thing" that
is not normal software (i.e. OS/Hypervisor).

> 
> [...]
>>  #ifdef CONFIG_HAVE_ACPI_APEI_NMI
>>  /*
>>   * printk is not safe in NMI context.  So in NMI handler, we allocate
>> @@ -1023,6 +1083,14 @@ static int ghes_probe(struct platform_device
>> *ghes_dev)
>>      case ACPI_HEST_NOTIFY_EXTERNAL:
>>      case ACPI_HEST_NOTIFY_SCI:
>>          break;
>> +    case ACPI_HEST_NOTIFY_SEA:
>> +        if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_SEA)) {
>> +            pr_warn(GHES_PFX "Generic hardware error source: %d
>> notified via SEA is not supported\n",
>> +                generic->header.source_id);
>> +            rc = -ENOTSUPP;
>> +            goto err;
>> +        }
>> +        break;
>>      case ACPI_HEST_NOTIFY_NMI:
>>          if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_NMI)) {
>>              pr_warn(GHES_PFX "Generic hardware error source: %d
>> notified via NMI interrupt is not supported!\n",
>> @@ -1034,6 +1102,13 @@ static int ghes_probe(struct platform_device
>> *ghes_dev)
>>          pr_warning(GHES_PFX "Generic hardware error source: %d
>> notified via local interrupt is not supported!\n",
>>                 generic->header.source_id);
>>          goto err;
>> +    case ACPI_HEST_NOTIFY_GPIO:
>> +    case ACPI_HEST_NOTIFY_SEI:
>> +    case ACPI_HEST_NOTIFY_GSIV:
>> +        pr_warn(GHES_PFX "Generic hardware error source: %d notified
>> via notification type %u is not supported\n",
>> +            generic->header.source_id, generic->header.source_id);
> 
> Hmm, some platform may trigger a interrupt to OS for firmware handling
> and it's in the ACPI 6.1 spec, is it a limitation now, or we need to
> add code later to support it?

On the current platforms we know of, we only leverage "emulated SCI",
which essentially maps to a GPIO interrupt (via ACPI event - mapped to
particular GPIO).  We will need to add support for other options
available in the spec (e.g. GSIV and SEI) later as platforms that use
those notification types become available.

Thanks,
--Harb
-- 
Qualcomm Datacenter Technologies, Inc.
as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Hanjun Guo]

Hi Harb,

On 2016/10/20 0:59, Abdulhamid, Harb wrote:
> On 10/18/2016 8:44 AM, Hanjun Guo wrote:
>> Hi Tyler,
>>
>> On 2016/10/8 5:31, Tyler Baicar wrote:
>>> ARM APEI extension proposal added SEA (Synchrounous External
>>> Abort) notification type for ARMv8.
>>> Add a new GHES error source handling function for SEA. If an error
>>> source's notification type is SEA, then this function can be registered
>>> into the SEA exception handler. That way GHES will parse and report
>>> SEA exceptions when they occur.
>> Does this SEA is replayed by the firmware (firmware first handling)
>> or directly triggered by the hardware when error is happened?
> Architecturally, an SEA must be synchronous and *precise*, so if you
> take an SEA on a particular load instruction, firmware/hardware should
> not be corrupting the context/state of the PE to allow software to
> determine which thread/process encountered the abort.  GHES error status

That's my concern too, and that's why I raised my question :)

> block will be expose to software with information about the type,
> severity, physical address impacted.
>
> Generally the error status block is populated by firmware.  However, as
> long as the above requirement is met, I don't think the spec precludes
> error status block being populated by hardware.  Those details must be
> completely transparent to software.
>
> Finally, to answer your more specific question:  If the implementation
> of firmware-first involves trapping the SEA in EL3 to do some firmware
> first handling, firmware must maintain the context of the offending ELx,
> generate an error record, and then "replay" the exception to normal
> (non-secure) software at the appropriate vector base address.
>

Thank you for your answer, it clears my confusion now, I will try something
similar on ARM64 platform, will get back to you if I get blocks.

Thanks
Hanjun