Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1625707 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH V15 00/11] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64 Tyler Baicar <tbaicar@codeaurora.org> - 2017-04-19 01:10 +0200
    [PATCH V15 04/11] efi: parse ARM processor error Tyler Baicar <tbaicar@codeaurora.org> - 2017-04-19 01:10 +0200
      Re: [PATCH V15 04/11] efi: parse ARM processor error Borislav Petkov <bp@alien8.de> - 2017-04-21 20:30 +0200
        Re: [PATCH V15 04/11] efi: parse ARM processor error "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-04-21 20:30 +0200
          Re: [PATCH V15 04/11] efi: parse ARM processor error Borislav Petkov <bp@alien8.de> - 2017-04-24 20:00 +0200
            Re: [PATCH V15 04/11] efi: parse ARM processor error "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-04-25 18:10 +0200
              Re: [PATCH V15 04/11] efi: parse ARM processor error Borislav Petkov <bp@alien8.de> - 2017-04-25 18:40 +0200
    [PATCH V15 05/11] arm64: exception: handle Synchronous External Abort Tyler Baicar <tbaicar@codeaurora.org> - 2017-04-19 01:10 +0200
    [PATCH V15 03/11] cper: add timestamp print to CPER status printing Tyler Baicar <tbaicar@codeaurora.org> - 2017-04-19 01:20 +0200
      Re: [PATCH V15 03/11] cper: add timestamp print to CPER status  printing Borislav Petkov <bp@alien8.de> - 2017-04-21 14:30 +0200
        Re: [PATCH V15 03/11] cper: add timestamp print to CPER status  printing Borislav Petkov <bp@alien8.de> - 2017-04-21 20:20 +0200
          Re: [PATCH V15 03/11] cper: add timestamp print to CPER status  printing "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-04-21 20:20 +0200
            Re: [PATCH V15 03/11] cper: add timestamp print to CPER status  printing Borislav Petkov <bp@alien8.de> - 2017-04-21 20:30 +0200
        Re: [PATCH V15 03/11] cper: add timestamp print to CPER status  printing "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-04-21 20:50 +0200

#1625707 — [PATCH V15 00/11] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

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.

V15:Rebase on 4.11-rc7
    Use wrapper functions for [un]mapping kernel acknowledgement register
    Spacing and name changes to make code cleaner
    Break up timestamp print to be more readable
    Break generic error data v3 structure handling code into seperate patch
     and have timestamp handling in it's own patch
    Put ARM CPER handling into ifdef for ARM systems
    Add braces and missing space to KVM patch

V14:Make sure function prototypes are in the __ASSEMBLY__ block
    Change is_abort_synchronous to is_abort_sea
    Use phys_addr_t for SEA address
    Return after successful SEA handling in handle_guest_abort()

V13:Rebase on 4.11rc2
    Print decimal and hex sizes for unknown CPER section errors
    Use proper CONFIG_* when using IS_ENABLED
    Move handle_guest_sea call prior to SEI check
    Add a return value to handle_guest_sea
    Move RCU locking into ghes_notify_sea
    Add valid bit checks to ARM trace event
    Remove GPIO, SEI, and GSIV cases in GHES
    Add ARCH_HAVE_NMI_SAFE_CMPXCHG since we added NMI usage

V12:Remove double quotes from CPER code
    Add helper function to check all SEA cases in KVM patch
    Replace nmi_enter/exit with rcu_read_lock/unlock for KVM SEA
    Change HAVE_ACPI_APEI_SEA to ACPI_APEI_SEA in KVM SEA case

V11:Change print_hex_dump calls to include ASCII output
    Change HAVE_ACPI_APEI_SEA to ACPI_APEI_SEA and make it 'default y'
    Add unknown print back when printing unknown CPER section
    Make sure to use "%s"" in CPER code
    Spacing fix when checking if SEA is enabled

V10:Fix spacing of trace event enabled if statement

V9: Move SEA_FnV_MASK to ESR_ELx_FnV
    Move HAVE_NMI into alphabetical order
    Remove duplicate hardirq.h include
    Only call ghes_notify_sea if HAVE_ACPI_APEI_SEA
    Make ACPI_APEI_SEA depend on ACPI_APEI_GHES
    Use phys_addr_t for physical address variable
    Make ghes_sea_add() return void
    Add include guard to ghes.h
    Verify HAVE_RAS before calling ras trace events
    Call __ghes_print_estatus() before __ghes_call_panic()
    Add trace_*_event_enabled() checks for both new trace events

V8: Remove SEA notifier
    Add FAR not valid bit check when populating the SEA error address
    Move nmi_enter/exit() to architecture specific code
    Add synchronize_rcu() usage to SEA handling
    Make GHES_IOREMAP_PAGES always 2
    Update ghes_ioremap_pfn_nmi() to work like ghes_ioremap_pfn_irq()
    Remove the SEA print from handle_guest_sea()

V7: Update a couple prints for ARM processor errors
    Add Print notifying if overflow occurred for ARM processor errors
    Check for ARM configuration to allow the compiler to ignore ARM code
     on non-ARM systems
    Use SEA acronym instead of spelling it out
    Update fault_info prints to be more clear
    Add NMI locking to SEA notification
    Remove error info structure from ARM trace event since there can be
     a variable amount of these structures

V6: Change HEST_TYPE_GENERIC_V2 to IS_HEST_TYPE_GENERIC_V2 for readability
    Move APEI helper defines from cper.h to ghes.h
    Add data_len decrement back into print loop
    Change references to ARMv8 to just ARM
    Rewrite ARM processor context info parsing
    Check valid bit of ARM error info field before printing it
    Add include of linux/uuid.h in ghes.c

V5: Fix GHES goto logic for error conditions
    Change ghes_do_read_ack to ghes_ack_error
    Make sure data version check is >= 3
    Use CPER helper functions in print functions
    Make handle_guest_sea() dummy function static for arm
    Add arm to subject line for KVM patch

V4: Add bit offset left shift to read_ack_write value
    Make HEST generic and generic_v2 structures a union in the ghes structure
    Move gdata v3 helper functions into ghes.h to avoid duplication
    Reorder the timestamp print and avoid memcpy
    Add helper functions for gdata size checking
    Rename the SEA functions
    Add helper function for GHES panics
    Set fru_id to NULL UUID at variable declaration
    Limit ARM trace event parameters to the needed structures
    Reorder the ARM trace event variables to save space
    Add comment for why we don't pass SEAs to the guest when it aborts
    Move ARM trace event call into GHES driver instead of CPER

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 (10):
  acpi: apei: read ack upon ghes record consumption
  ras: acpi/apei: cper: add support for generic data v3 structure
  cper: add timestamp print to CPER status printing
  efi: parse ARM 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
  arm/arm64: KVM: add guest SEA support

 arch/arm/include/asm/kvm_arm.h       |  10 ++
 arch/arm/include/asm/system_misc.h   |   5 +
 arch/arm/kvm/mmu.c                   |  35 +++++-
 arch/arm64/Kconfig                   |   2 +
 arch/arm64/include/asm/esr.h         |   1 +
 arch/arm64/include/asm/kvm_arm.h     |  10 ++
 arch/arm64/include/asm/system_misc.h |   2 +
 arch/arm64/mm/fault.c                |  77 +++++++++++--
 drivers/acpi/apei/Kconfig            |  15 +++
 drivers/acpi/apei/ghes.c             | 184 ++++++++++++++++++++++++++----
 drivers/acpi/apei/hest.c             |   7 +-
 drivers/firmware/efi/cper.c          | 213 ++++++++++++++++++++++++++++++++---
 drivers/ras/ras.c                    |   2 +
 include/acpi/ghes.h                  |  34 +++++-
 include/linux/cper.h                 |  54 +++++++++
 include/ras/ras_event.h              |  90 +++++++++++++++
 16 files changed, 687 insertions(+), 54 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.

[toc] | [next] | [standalone]

#1625708 — [PATCH V15 04/11] efi: parse ARM processor error

Add support for ARM Common Platform Error Record (CPER).
UEFI 2.6 specification adds support for ARM 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: Tyler Baicar <tbaicar@codeaurora.org>
CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Reviewed-by: James Morse <james.morse@arm.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/cper.c | 135 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cper.h        |  54 ++++++++++++++++++
 2 files changed, 189 insertions(+)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 46585f9..f959185 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -110,12 +110,15 @@ void cper_print_bits(const char *pfx, unsigned int bits,
 static const char * const proc_type_strs[] = {
 	"IA32/X64",
 	"IA64",
+	"ARM",
 };
 
 static const char * const proc_isa_strs[] = {
 	"IA32",
 	"IA64",
 	"X64",
+	"ARM A32/T32",
+	"ARM A64",
 };
 
 static const char * const proc_error_type_strs[] = {
@@ -184,6 +187,128 @@ static void cper_print_proc_generic(const char *pfx,
 		printk("%s""IP: 0x%016llx\n", pfx, proc->ip);
 }
 
+#if defined(CONFIG_ARM64) || defined(CONFIG_ARM)
+static const char * const arm_reg_ctx_strs[] = {
+	"AArch32 general purpose registers",
+	"AArch32 EL1 context registers",
+	"AArch32 EL2 context registers",
+	"AArch32 secure context registers",
+	"AArch64 general purpose registers",
+	"AArch64 EL1 context registers",
+	"AArch64 EL2 context registers",
+	"AArch64 EL3 context registers",
+	"Misc. system register structure",
+};
+
+static void cper_print_proc_arm(const char *pfx,
+				const struct cper_sec_proc_arm *proc)
+{
+	int i, len, max_ctx_type;
+	struct cper_arm_err_info *err_info;
+	struct cper_arm_ctx_info *ctx_info;
+	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("%sfirmware-generated error record is incorrect\n", pfx);
+		printk("%sERR_INFO_NUM is %d\n", pfx, proc->err_info_num);
+		return;
+	}
+
+	if (proc->validation_bits & CPER_ARM_VALID_MPIDR)
+		printk("%sMPIDR: 0x%016llx\n", pfx, proc->mpidr);
+	if (proc->validation_bits & CPER_ARM_VALID_AFFINITY_LEVEL)
+		printk("%serror affinity level: %d\n", pfx,
+			proc->affinity_level);
+	if (proc->validation_bits & CPER_ARM_VALID_RUNNING_STATE) {
+		printk("%srunning state: 0x%x\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_arm_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_ARM_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:%u\n",
+				newpfx, err_info->multiple_error);
+		}
+		if (err_info->validation_bits & CPER_ARM_INFO_VALID_FLAGS) {
+			if (err_info->flags & CPER_ARM_INFO_FLAGS_FIRST)
+				printk("%sfirst error captured\n", newpfx);
+			if (err_info->flags & CPER_ARM_INFO_FLAGS_LAST)
+				printk("%slast error captured\n", newpfx);
+			if (err_info->flags & CPER_ARM_INFO_FLAGS_PROPAGATED)
+				printk("%spropagated error captured\n",
+				       newpfx);
+			if (err_info->flags & CPER_ARM_INFO_FLAGS_OVERFLOW)
+				printk("%soverflow occurred, error info is incomplete\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");
+		if (err_info->validation_bits & CPER_ARM_INFO_VALID_ERR_INFO)
+			printk("%serror_info: 0x%016llx\n", newpfx,
+			       err_info->error_info);
+		if (err_info->validation_bits & CPER_ARM_INFO_VALID_VIRT_ADDR)
+			printk("%svirtual fault address: 0x%016llx\n",
+				newpfx, err_info->virt_fault_addr);
+		if (err_info->validation_bits &
+		    CPER_ARM_INFO_VALID_PHYSICAL_ADDR)
+			printk("%sphysical fault address: 0x%016llx\n",
+				newpfx, err_info->physical_fault_addr);
+		err_info += 1;
+	}
+	ctx_info = (struct cper_arm_ctx_info *)err_info;
+	max_ctx_type = ARRAY_SIZE(arm_reg_ctx_strs) - 1;
+	for (i = 0; i < proc->context_info_num; i++) {
+		int size = sizeof(*ctx_info) + ctx_info->size;
+
+		printk("%sContext info structure %d:\n", pfx, i);
+		if (len < size) {
+			printk("%ssection length is too small\n", newpfx);
+			printk("%sfirmware-generated error record is incorrect\n", pfx);
+			return;
+		}
+		if (ctx_info->type > max_ctx_type) {
+			printk("%sInvalid context type: %d\n", newpfx,
+							ctx_info->type);
+			printk("%sMax context type: %d\n", newpfx,
+							max_ctx_type);
+			return;
+		}
+		printk("%sregister context type %d: %s\n", newpfx,
+			ctx_info->type, arm_reg_ctx_strs[ctx_info->type]);
+		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4,
+				(ctx_info + 1), ctx_info->size, 0);
+		len -= size;
+		ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + size);
+	}
+
+	if (len > 0) {
+		printk("%sVendor specific error info has %u bytes:\n", pfx,
+		       len);
+		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4, ctx_info,
+				len, true);
+	}
+}
+#endif
+
 static const char * const mem_err_type_strs[] = {
 	"unknown",
 	"no error",
@@ -461,6 +586,16 @@ static void cper_estatus_timestamp(const char *pfx,
 			cper_print_pcie(newpfx, pcie, gdata);
 		else
 			goto err_section_too_small;
+	} else if ((IS_ENABLED(CONFIG_ARM64) || IS_ENABLED(CONFIG_ARM)) &&
+		   !uuid_le_cmp(*sec_type, CPER_SEC_PROC_ARM)) {
+		struct cper_sec_proc_arm *arm_err;
+
+		arm_err = acpi_hest_get_payload(gdata);
+		printk("%ssection_type: ARM processor error\n", newpfx);
+		if (gdata->error_data_length >= sizeof(*arm_err))
+			cper_print_proc_arm(newpfx, arm_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..85450f3 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -180,6 +180,10 @@ enum {
 #define CPER_SEC_PROC_IPF						\
 	UUID_LE(0xE429FAF1, 0x3CB7, 0x11D4, 0x0B, 0xCA, 0x07, 0x00,	\
 		0x80, 0xC7, 0x3C, 0x88, 0x81)
+/* Processor Specific: ARM */
+#define CPER_SEC_PROC_ARM						\
+	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 +259,22 @@ enum {
 
 #define CPER_PCIE_SLOT_SHIFT			3
 
+#define CPER_ARM_VALID_MPIDR			0x00000001
+#define CPER_ARM_VALID_AFFINITY_LEVEL		0x00000002
+#define CPER_ARM_VALID_RUNNING_STATE		0x00000004
+#define CPER_ARM_VALID_VENDOR_INFO		0x00000008
+
+#define CPER_ARM_INFO_VALID_MULTI_ERR		0x0001
+#define CPER_ARM_INFO_VALID_FLAGS		0x0002
+#define CPER_ARM_INFO_VALID_ERR_INFO		0x0004
+#define CPER_ARM_INFO_VALID_VIRT_ADDR		0x0008
+#define CPER_ARM_INFO_VALID_PHYSICAL_ADDR	0x0010
+
+#define CPER_ARM_INFO_FLAGS_FIRST		0x0001
+#define CPER_ARM_INFO_FLAGS_LAST		0x0002
+#define CPER_ARM_INFO_FLAGS_PROPAGATED		0x0004
+#define CPER_ARM_INFO_FLAGS_OVERFLOW		0x0008
+
 /*
  * All tables and structs must be byte-packed to match CPER
  * specification, since the tables are provided by the system BIOS
@@ -340,6 +360,40 @@ struct cper_ia_proc_ctx {
 	__u64	mm_reg_addr;
 };
 
+/* ARM Processor Error Section */
+struct cper_sec_proc_arm {
+	__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;
+};
+
+/* ARM Processor Error Information Structure */
+struct cper_arm_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;
+};
+
+/* ARM Processor Context Information Structure */
+struct cper_arm_ctx_info {
+	__u16	version;
+	__u16	type;
+	__u32	size;
+};
+
 /* 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.

[toc] | [prev] | [next] | [standalone]

#1628441 — Re: [PATCH V15 04/11] efi: parse ARM processor error

On Tue, Apr 18, 2017 at 05:05:16PM -0600, Tyler Baicar wrote:
> Add support for ARM Common Platform Error Record (CPER).
> UEFI 2.6 specification adds support for ARM 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: Tyler Baicar <tbaicar@codeaurora.org>
> CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Reviewed-by: James Morse <james.morse@arm.com>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/cper.c | 135 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cper.h        |  54 ++++++++++++++++++
>  2 files changed, 189 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 46585f9..f959185 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -110,12 +110,15 @@ void cper_print_bits(const char *pfx, unsigned int bits,
>  static const char * const proc_type_strs[] = {
>  	"IA32/X64",
>  	"IA64",
> +	"ARM",
>  };
>  
>  static const char * const proc_isa_strs[] = {
>  	"IA32",
>  	"IA64",
>  	"X64",
> +	"ARM A32/T32",
> +	"ARM A64",
>  };
>  
>  static const char * const proc_error_type_strs[] = {
> @@ -184,6 +187,128 @@ static void cper_print_proc_generic(const char *pfx,
>  		printk("%s""IP: 0x%016llx\n", pfx, proc->ip);
>  }
>  
> +#if defined(CONFIG_ARM64) || defined(CONFIG_ARM)
> +static const char * const arm_reg_ctx_strs[] = {
> +	"AArch32 general purpose registers",
> +	"AArch32 EL1 context registers",
> +	"AArch32 EL2 context registers",
> +	"AArch32 secure context registers",
> +	"AArch64 general purpose registers",
> +	"AArch64 EL1 context registers",
> +	"AArch64 EL2 context registers",
> +	"AArch64 EL3 context registers",
> +	"Misc. system register structure",
> +};
> +
> +static void cper_print_proc_arm(const char *pfx,
> +				const struct cper_sec_proc_arm *proc)
> +{
> +	int i, len, max_ctx_type;
> +	struct cper_arm_err_info *err_info;
> +	struct cper_arm_ctx_info *ctx_info;
> +	char newpfx[64];
> +
> +	printk("%ssection length: %d\n", pfx, proc->section_length);

We need to dump section length because?

> +	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);

Now here we *can* dump it.

> +		printk("%sfirmware-generated error record is incorrect\n", pfx);
> +		printk("%sERR_INFO_NUM is %d\n", pfx, proc->err_info_num);
> +		return;
> +	}
> +
> +	if (proc->validation_bits & CPER_ARM_VALID_MPIDR)
> +		printk("%sMPIDR: 0x%016llx\n", pfx, proc->mpidr);


<---- newline here.

Also, what is MPIDR and can it be written in a more user-friendly manner
and not be an abbreviation?

> +	if (proc->validation_bits & CPER_ARM_VALID_AFFINITY_LEVEL)
> +		printk("%serror affinity level: %d\n", pfx,
> +			proc->affinity_level);
> +	if (proc->validation_bits & CPER_ARM_VALID_RUNNING_STATE) {
> +		printk("%srunning state: 0x%x\n", pfx, proc->running_state);
> +		printk("%sPSCI state: %d\n", pfx, proc->psci_state);

One more abbreviation. Please consider whether having the abbreviations
or actually writing them out is more user-friendly.

> +	}
> +
> +	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);

That INDENT_SP thing is just silly, someone should kill it.

> +
> +	err_info = (struct cper_arm_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);

<---- newline here.

Why do we even dump version and info for *every* err_info structure?

> +		if (err_info->validation_bits &
> +		    CPER_ARM_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:%u\n",
> +				newpfx, err_info->multiple_error);

So this can be simply: "num errors: %d", err_info->multiple_error+1...

Without checking CPER_ARM_INFO_VALID_MULTI_ERR.

> +		}

<---- newline here.

> +		if (err_info->validation_bits & CPER_ARM_INFO_VALID_FLAGS) {
> +			if (err_info->flags & CPER_ARM_INFO_FLAGS_FIRST)
> +				printk("%sfirst error captured\n", newpfx);
> +			if (err_info->flags & CPER_ARM_INFO_FLAGS_LAST)
> +				printk("%slast error captured\n", newpfx);
> +			if (err_info->flags & CPER_ARM_INFO_FLAGS_PROPAGATED)
> +				printk("%spropagated error captured\n",
> +				       newpfx);
> +			if (err_info->flags & CPER_ARM_INFO_FLAGS_OVERFLOW)
> +				printk("%soverflow occurred, error info is incomplete\n",
> +				       newpfx);
> +		}

<---- newline here.

> +		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");
> +		if (err_info->validation_bits & CPER_ARM_INFO_VALID_ERR_INFO)
> +			printk("%serror_info: 0x%016llx\n", newpfx,
> +			       err_info->error_info);

err_info->error_info ?

What is that supposed to mean? A u64 value of some sorts.

> +		if (err_info->validation_bits & CPER_ARM_INFO_VALID_VIRT_ADDR)
> +			printk("%svirtual fault address: 0x%016llx\n",
> +				newpfx, err_info->virt_fault_addr);
> +		if (err_info->validation_bits &
> +		    CPER_ARM_INFO_VALID_PHYSICAL_ADDR)

Just let that line stick out.

> +			printk("%sphysical fault address: 0x%016llx\n",
> +				newpfx, err_info->physical_fault_addr);
> +		err_info += 1;
> +	}

<---- newline here.

That function is kinda missing newlines.

> +	ctx_info = (struct cper_arm_ctx_info *)err_info;
> +	max_ctx_type = ARRAY_SIZE(arm_reg_ctx_strs) - 1;
> +	for (i = 0; i < proc->context_info_num; i++) {
> +		int size = sizeof(*ctx_info) + ctx_info->size;
> +
> +		printk("%sContext info structure %d:\n", pfx, i);
> +		if (len < size) {
> +			printk("%ssection length is too small\n", newpfx);
> +			printk("%sfirmware-generated error record is incorrect\n", pfx);
> +			return;
> +		}
> +		if (ctx_info->type > max_ctx_type) {
> +			printk("%sInvalid context type: %d\n", newpfx,
> +							ctx_info->type);
> +			printk("%sMax context type: %d\n", newpfx,
> +							max_ctx_type);
> +			return;

You can combine those into:

                        printk("%sInvalid context type: %d (max: %d)\n",
				newpfx, ctx_info->type, max_ctx_type);


> +		}
> +		printk("%sregister context type %d: %s\n", newpfx,
> +			ctx_info->type, arm_reg_ctx_strs[ctx_info->type]);

Why dump the type as %d and as a string too? String should be enough, no?

> +		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4,
> +				(ctx_info + 1), ctx_info->size, 0);
> +		len -= size;
> +		ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + size);
> +	}
> +
> +	if (len > 0) {
> +		printk("%sVendor specific error info has %u bytes:\n", pfx,
> +		       len);
> +		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4, ctx_info,
> +				len, true);

That looks like it should be a debug printk...

> +	}
> +}
> +#endif
> +
>  static const char * const mem_err_type_strs[] = {
>  	"unknown",
>  	"no error",
> @@ -461,6 +586,16 @@ static void cper_estatus_timestamp(const char *pfx,
>  			cper_print_pcie(newpfx, pcie, gdata);
>  		else
>  			goto err_section_too_small;
> +	} else if ((IS_ENABLED(CONFIG_ARM64) || IS_ENABLED(CONFIG_ARM)) &&
> +		   !uuid_le_cmp(*sec_type, CPER_SEC_PROC_ARM)) {
> +		struct cper_sec_proc_arm *arm_err;
> +
> +		arm_err = acpi_hest_get_payload(gdata);

		struct cper_sec_proc_arm *arm_err = acpi_hest_get_payload(gdata);

> +		printk("%ssection_type: ARM processor error\n", newpfx);
> +		if (gdata->error_data_length >= sizeof(*arm_err))
> +			cper_print_proc_arm(newpfx, arm_err);
> +		else
> +			goto err_section_too_small;

You need to build-test your patches before submitting:

drivers/firmware/efi/cper.c: In function ‘cper_estatus_print_section’:
drivers/firmware/efi/cper.c:596:4: error: implicit declaration of function ‘cper_print_proc_arm’ [-Werror=implicit-function-declaration]
    cper_print_proc_arm(newpfx, arm_err);
    ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[3]: *** [drivers/firmware/efi/cper.o] Error 1
make[2]: *** [drivers/firmware/efi] Error 2
make[1]: *** [drivers/firmware] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2
make: *** Waiting for unfinished jobs....

this is a x86 build.

>  	} 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..85450f3 100644
> --- a/include/linux/cper.h
> +++ b/include/linux/cper.h
> @@ -180,6 +180,10 @@ enum {
>  #define CPER_SEC_PROC_IPF						\
>  	UUID_LE(0xE429FAF1, 0x3CB7, 0x11D4, 0x0B, 0xCA, 0x07, 0x00,	\
>  		0x80, 0xC7, 0x3C, 0x88, 0x81)
> +/* Processor Specific: ARM */
> +#define CPER_SEC_PROC_ARM						\
> +	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 +259,22 @@ enum {
>  
>  #define CPER_PCIE_SLOT_SHIFT			3
>  
> +#define CPER_ARM_VALID_MPIDR			0x00000001
> +#define CPER_ARM_VALID_AFFINITY_LEVEL		0x00000002
> +#define CPER_ARM_VALID_RUNNING_STATE		0x00000004
> +#define CPER_ARM_VALID_VENDOR_INFO		0x00000008
> +
> +#define CPER_ARM_INFO_VALID_MULTI_ERR		0x0001
> +#define CPER_ARM_INFO_VALID_FLAGS		0x0002
> +#define CPER_ARM_INFO_VALID_ERR_INFO		0x0004
> +#define CPER_ARM_INFO_VALID_VIRT_ADDR		0x0008
> +#define CPER_ARM_INFO_VALID_PHYSICAL_ADDR	0x0010
> +
> +#define CPER_ARM_INFO_FLAGS_FIRST		0x0001
> +#define CPER_ARM_INFO_FLAGS_LAST		0x0002
> +#define CPER_ARM_INFO_FLAGS_PROPAGATED		0x0004
> +#define CPER_ARM_INFO_FLAGS_OVERFLOW		0x0008

For all of the above use BIT().

> +
>  /*
>   * All tables and structs must be byte-packed to match CPER
>   * specification, since the tables are provided by the system BIOS
> @@ -340,6 +360,40 @@ struct cper_ia_proc_ctx {
>  	__u64	mm_reg_addr;
>  };
>  
> +/* ARM Processor Error Section */
> +struct cper_sec_proc_arm {
> +	__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;

Align comments vertically pls.

> +};
> +
> +/* ARM Processor Error Information Structure */
> +struct cper_arm_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;
> +};
> +
> +/* ARM Processor Context Information Structure */
> +struct cper_arm_ctx_info {
> +	__u16	version;
> +	__u16	type;
> +	__u32	size;
> +};

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1628446 — Re: [PATCH V15 04/11] efi: parse ARM processor error

On 4/21/2017 11:55 AM, Borislav Petkov wrote:
> On Tue, Apr 18, 2017 at 05:05:16PM -0600, Tyler Baicar wrote:
>> Add support for ARM Common Platform Error Record (CPER).
>> UEFI 2.6 specification adds support for ARM 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: Tyler Baicar <tbaicar@codeaurora.org>
>> CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Reviewed-by: James Morse <james.morse@arm.com>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   drivers/firmware/efi/cper.c | 135 ++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/cper.h        |  54 ++++++++++++++++++
>>   2 files changed, 189 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> index 46585f9..f959185 100644
>> --- a/drivers/firmware/efi/cper.c
>> +++ b/drivers/firmware/efi/cper.c
>> @@ -110,12 +110,15 @@ void cper_print_bits(const char *pfx, unsigned int bits,
>>   static const char * const proc_type_strs[] = {
>>   	"IA32/X64",
>>   	"IA64",
>> +	"ARM",
>>   };
>>   
>>   static const char * const proc_isa_strs[] = {
>>   	"IA32",
>>   	"IA64",
>>   	"X64",
>> +	"ARM A32/T32",
>> +	"ARM A64",
>>   };
>>   
>>   static const char * const proc_error_type_strs[] = {
>> @@ -184,6 +187,128 @@ static void cper_print_proc_generic(const char *pfx,
>>   		printk("%s""IP: 0x%016llx\n", pfx, proc->ip);
>>   }
>>   
>> +#if defined(CONFIG_ARM64) || defined(CONFIG_ARM)
>> +static const char * const arm_reg_ctx_strs[] = {
>> +	"AArch32 general purpose registers",
>> +	"AArch32 EL1 context registers",
>> +	"AArch32 EL2 context registers",
>> +	"AArch32 secure context registers",
>> +	"AArch64 general purpose registers",
>> +	"AArch64 EL1 context registers",
>> +	"AArch64 EL2 context registers",
>> +	"AArch64 EL3 context registers",
>> +	"Misc. system register structure",
>> +};
>> +
>> +static void cper_print_proc_arm(const char *pfx,
>> +				const struct cper_sec_proc_arm *proc)
>> +{
>> +	int i, len, max_ctx_type;
>> +	struct cper_arm_err_info *err_info;
>> +	struct cper_arm_ctx_info *ctx_info;
>> +	char newpfx[64];
>> +
>> +	printk("%ssection length: %d\n", pfx, proc->section_length);
> We need to dump section length because?
I guess it's not really needed. It just may be useful considering there 
can be numerous error info structures, numerous context info structures, 
and a variable length vendor information section. I can move this print 
to only in the length check failure cases.
>
>> +	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);
> Now here we *can* dump it.
>
>> +		printk("%sfirmware-generated error record is incorrect\n", pfx);
>> +		printk("%sERR_INFO_NUM is %d\n", pfx, proc->err_info_num);
>> +		return;
>> +	}
>> +
>> +	if (proc->validation_bits & CPER_ARM_VALID_MPIDR)
>> +		printk("%sMPIDR: 0x%016llx\n", pfx, proc->mpidr);
>
> <---- newline here.
>
> Also, what is MPIDR and can it be written in a more user-friendly manner
> and not be an abbreviation?
>
>> +	if (proc->validation_bits & CPER_ARM_VALID_AFFINITY_LEVEL)
>> +		printk("%serror affinity level: %d\n", pfx,
>> +			proc->affinity_level);
>> +	if (proc->validation_bits & CPER_ARM_VALID_RUNNING_STATE) {
>> +		printk("%srunning state: 0x%x\n", pfx, proc->running_state);
>> +		printk("%sPSCI state: %d\n", pfx, proc->psci_state);
> One more abbreviation. Please consider whether having the abbreviations
> or actually writing them out is more user-friendly.
>
>> +	}
>> +
>> +	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> That INDENT_SP thing is just silly, someone should kill it.
>
>> +
>> +	err_info = (struct cper_arm_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);
> <---- newline here.
>
> Why do we even dump version and info for *every* err_info structure?
Because these are part of the error information structure. I wouldn't 
think FW would populate error information structures that are different 
versions in the same processor error, but it could be possible from the 
spec (at least once there are different versions of the table).
>
>> +		if (err_info->validation_bits &
>> +		    CPER_ARM_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:%u\n",
>> +				newpfx, err_info->multiple_error);
> So this can be simply: "num errors: %d", err_info->multiple_error+1...
>
> Without checking CPER_ARM_INFO_VALID_MULTI_ERR.
>
>> +		}
> <---- newline here.
>
>> +		if (err_info->validation_bits & CPER_ARM_INFO_VALID_FLAGS) {
>> +			if (err_info->flags & CPER_ARM_INFO_FLAGS_FIRST)
>> +				printk("%sfirst error captured\n", newpfx);
>> +			if (err_info->flags & CPER_ARM_INFO_FLAGS_LAST)
>> +				printk("%slast error captured\n", newpfx);
>> +			if (err_info->flags & CPER_ARM_INFO_FLAGS_PROPAGATED)
>> +				printk("%spropagated error captured\n",
>> +				       newpfx);
>> +			if (err_info->flags & CPER_ARM_INFO_FLAGS_OVERFLOW)
>> +				printk("%soverflow occurred, error info is incomplete\n",
>> +				       newpfx);
>> +		}
> <---- newline here.
>
>> +		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");
>> +		if (err_info->validation_bits & CPER_ARM_INFO_VALID_ERR_INFO)
>> +			printk("%serror_info: 0x%016llx\n", newpfx,
>> +			       err_info->error_info);
> err_info->error_info ?
>
> What is that supposed to mean? A u64 value of some sorts.
There is an error information 64 bit value in the ARM processor error 
information structure. (UEFI spec 2.6 table 261)
>
>> +		if (err_info->validation_bits & CPER_ARM_INFO_VALID_VIRT_ADDR)
>> +			printk("%svirtual fault address: 0x%016llx\n",
>> +				newpfx, err_info->virt_fault_addr);
>> +		if (err_info->validation_bits &
>> +		    CPER_ARM_INFO_VALID_PHYSICAL_ADDR)
> Just let that line stick out.
>
>> +			printk("%sphysical fault address: 0x%016llx\n",
>> +				newpfx, err_info->physical_fault_addr);
>> +		err_info += 1;
>> +	}
> <---- newline here.
>
> That function is kinda missing newlines.
>
>> +	ctx_info = (struct cper_arm_ctx_info *)err_info;
>> +	max_ctx_type = ARRAY_SIZE(arm_reg_ctx_strs) - 1;
>> +	for (i = 0; i < proc->context_info_num; i++) {
>> +		int size = sizeof(*ctx_info) + ctx_info->size;
>> +
>> +		printk("%sContext info structure %d:\n", pfx, i);
>> +		if (len < size) {
>> +			printk("%ssection length is too small\n", newpfx);
>> +			printk("%sfirmware-generated error record is incorrect\n", pfx);
>> +			return;
>> +		}
>> +		if (ctx_info->type > max_ctx_type) {
>> +			printk("%sInvalid context type: %d\n", newpfx,
>> +							ctx_info->type);
>> +			printk("%sMax context type: %d\n", newpfx,
>> +							max_ctx_type);
>> +			return;
> You can combine those into:
>
>                          printk("%sInvalid context type: %d (max: %d)\n",
> 				newpfx, ctx_info->type, max_ctx_type);
>
>
>> +		}
>> +		printk("%sregister context type %d: %s\n", newpfx,
>> +			ctx_info->type, arm_reg_ctx_strs[ctx_info->type]);
> Why dump the type as %d and as a string too? String should be enough, no?
Yes, the string should be sufficient.
>
>> +		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4,
>> +				(ctx_info + 1), ctx_info->size, 0);
>> +		len -= size;
>> +		ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + size);
>> +	}
>> +
>> +	if (len > 0) {
>> +		printk("%sVendor specific error info has %u bytes:\n", pfx,
>> +		       len);
>> +		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4, ctx_info,
>> +				len, true);
> That looks like it should be a debug printk...
Why's that? Dumping this vendor specific error information is similar to 
the unrecognized CPER section reporting which is also meant for vendor 
specific information https://lkml.org/lkml/2017/4/18/751

Thanks,
Tyler
>
>> +	}
>> +}
>> +#endif
>> +
>>   static const char * const mem_err_type_strs[] = {
>>   	"unknown",
>>   	"no error",
>> @@ -461,6 +586,16 @@ static void cper_estatus_timestamp(const char *pfx,
>>   			cper_print_pcie(newpfx, pcie, gdata);
>>   		else
>>   			goto err_section_too_small;
>> +	} else if ((IS_ENABLED(CONFIG_ARM64) || IS_ENABLED(CONFIG_ARM)) &&
>> +		   !uuid_le_cmp(*sec_type, CPER_SEC_PROC_ARM)) {
>> +		struct cper_sec_proc_arm *arm_err;
>> +
>> +		arm_err = acpi_hest_get_payload(gdata);
> 		struct cper_sec_proc_arm *arm_err = acpi_hest_get_payload(gdata);
>
>> +		printk("%ssection_type: ARM processor error\n", newpfx);
>> +		if (gdata->error_data_length >= sizeof(*arm_err))
>> +			cper_print_proc_arm(newpfx, arm_err);
>> +		else
>> +			goto err_section_too_small;
> You need to build-test your patches before submitting:
>
> drivers/firmware/efi/cper.c: In function ‘cper_estatus_print_section’:
> drivers/firmware/efi/cper.c:596:4: error: implicit declaration of function ‘cper_print_proc_arm’ [-Werror=implicit-function-declaration]
>      cper_print_proc_arm(newpfx, arm_err);
>      ^~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
> make[3]: *** [drivers/firmware/efi/cper.o] Error 1
> make[2]: *** [drivers/firmware/efi] Error 2
> make[1]: *** [drivers/firmware] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [drivers] Error 2
> make: *** Waiting for unfinished jobs....
>
> this is a x86 build.
>
>>   	} 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..85450f3 100644
>> --- a/include/linux/cper.h
>> +++ b/include/linux/cper.h
>> @@ -180,6 +180,10 @@ enum {
>>   #define CPER_SEC_PROC_IPF						\
>>   	UUID_LE(0xE429FAF1, 0x3CB7, 0x11D4, 0x0B, 0xCA, 0x07, 0x00,	\
>>   		0x80, 0xC7, 0x3C, 0x88, 0x81)
>> +/* Processor Specific: ARM */
>> +#define CPER_SEC_PROC_ARM						\
>> +	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 +259,22 @@ enum {
>>   
>>   #define CPER_PCIE_SLOT_SHIFT			3
>>   
>> +#define CPER_ARM_VALID_MPIDR			0x00000001
>> +#define CPER_ARM_VALID_AFFINITY_LEVEL		0x00000002
>> +#define CPER_ARM_VALID_RUNNING_STATE		0x00000004
>> +#define CPER_ARM_VALID_VENDOR_INFO		0x00000008
>> +
>> +#define CPER_ARM_INFO_VALID_MULTI_ERR		0x0001
>> +#define CPER_ARM_INFO_VALID_FLAGS		0x0002
>> +#define CPER_ARM_INFO_VALID_ERR_INFO		0x0004
>> +#define CPER_ARM_INFO_VALID_VIRT_ADDR		0x0008
>> +#define CPER_ARM_INFO_VALID_PHYSICAL_ADDR	0x0010
>> +
>> +#define CPER_ARM_INFO_FLAGS_FIRST		0x0001
>> +#define CPER_ARM_INFO_FLAGS_LAST		0x0002
>> +#define CPER_ARM_INFO_FLAGS_PROPAGATED		0x0004
>> +#define CPER_ARM_INFO_FLAGS_OVERFLOW		0x0008
> For all of the above use BIT().
>
>> +
>>   /*
>>    * All tables and structs must be byte-packed to match CPER
>>    * specification, since the tables are provided by the system BIOS
>> @@ -340,6 +360,40 @@ struct cper_ia_proc_ctx {
>>   	__u64	mm_reg_addr;
>>   };
>>   
>> +/* ARM Processor Error Section */
>> +struct cper_sec_proc_arm {
>> +	__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;
> Align comments vertically pls.
>
>> +};
>> +
>> +/* ARM Processor Error Information Structure */
>> +struct cper_arm_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;
>> +};
>> +
>> +/* ARM Processor Context Information Structure */
>> +struct cper_arm_ctx_info {
>> +	__u16	version;
>> +	__u16	type;
>> +	__u32	size;
>> +};

-- 
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.

[toc] | [prev] | [next] | [standalone]

#1629827 — Re: [PATCH V15 04/11] efi: parse ARM processor error

On Fri, Apr 21, 2017 at 12:22:09PM -0600, Baicar, Tyler wrote:
> I guess it's not really needed. It just may be useful considering there can
> be numerous error info structures, numerous context info structures, and a
> variable length vendor information section. I can move this print to only in
> the length check failure cases.

And? Why does the user care?

I mean, it is good for debugging when you wanna see you're parsing the
error info data properly but otherwise it doesn't improve the error
reporting one bit.

> Because these are part of the error information structure. I wouldn't think
> FW would populate error information structures that are different versions
> in the same processor error, but it could be possible from the spec (at
> least once there are different versions of the table).

Same argument as above.

> There is an error information 64 bit value in the ARM processor error
> information structure. (UEFI spec 2.6 table 261)

So that's IP-dependent and explained in the following tables. Any plans
on decoding that too?

> Why's that? Dumping this vendor specific error information is similar to the
> unrecognized CPER section reporting which is also meant for vendor specific
> information https://lkml.org/lkml/2017/4/18/751

And how do those naked bytes help the user understand the error happening?

Even in your example you have:

[  140.739210] {1}[Hardware Error]:   00000000: 4d415201 4d492031 453a4d45 435f4343  .RAM1 IMEM:ECC_C
[  140.739214] {1}[Hardware Error]:   00000010: 53515f45 44525f42 00000000 00000000  E_QSB_RD........

Which looks like some correctable ECC DRAM error and is actually begging
to be decoded in a human-readable form. So let's do that completely and
not dump partially decoded information.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1630735 — Re: [PATCH V15 04/11] efi: parse ARM processor error

On 4/24/2017 11:52 AM, Borislav Petkov wrote:
> On Fri, Apr 21, 2017 at 12:22:09PM -0600, Baicar, Tyler wrote:
>> I guess it's not really needed. It just may be useful considering there can
>> be numerous error info structures, numerous context info structures, and a
>> variable length vendor information section. I can move this print to only in
>> the length check failure cases.
> And? Why does the user care?
>
> I mean, it is good for debugging when you wanna see you're parsing the
> error info data properly but otherwise it doesn't improve the error
> reporting one bit.
I'll move this to just happen when the length check fails.
>> Because these are part of the error information structure. I wouldn't think
>> FW would populate error information structures that are different versions
>> in the same processor error, but it could be possible from the spec (at
>> least once there are different versions of the table).
> Same argument as above.
I can remove it then.
>
>> There is an error information 64 bit value in the ARM processor error
>> information structure. (UEFI spec 2.6 table 261)
> So that's IP-dependent and explained in the following tables. Any plans
> on decoding that too?
Yes, I do plan on adding further decoding for these values in the future.
>
>> Why's that? Dumping this vendor specific error information is similar to the
>> unrecognized CPER section reporting which is also meant for vendor specific
>> information https://lkml.org/lkml/2017/4/18/751
> And how do those naked bytes help the user understand the error happening?
>
> Even in your example you have:
>
> [  140.739210] {1}[Hardware Error]:   00000000: 4d415201 4d492031 453a4d45 435f4343  .RAM1 IMEM:ECC_C
> [  140.739214] {1}[Hardware Error]:   00000010: 53515f45 44525f42 00000000 00000000  E_QSB_RD........
>
> Which looks like some correctable ECC DRAM error and is actually begging
> to be decoded in a human-readable form. So let's do that completely and
> not dump partially decoded information.
That seems like something that should be done outside of these patches 
(if added to the kernel at all). The decoding for this information would 
all be vendor specific, so I'm not sure if we want to pollute the EFI 
code with vendor specific error decoding. Currently we are using the RAS 
Daemon user space tool for the decoding of this information since 
vendors can easily pick up this tool and add an extension for their 
vendor specific parsing. These prints will only happen when the firmware 
supports the vendor specific error information anyway.

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.

[toc] | [prev] | [next] | [standalone]

#1630777 — Re: [PATCH V15 04/11] efi: parse ARM processor error

On Tue, Apr 25, 2017 at 10:05:31AM -0600, Baicar, Tyler wrote:
> That seems like something that should be done outside of these patches (if
> added to the kernel at all). The decoding for this information would all be
> vendor specific, so I'm not sure if we want to pollute the EFI code with
> vendor specific error decoding. Currently we are using the RAS Daemon user
> space tool for the decoding of this information since vendors can easily
> pick up this tool and add an extension for their vendor specific parsing.
> These prints will only happen when the firmware supports the vendor specific
> error information anyway.

The same questions apply here: what do you do if the machine panics
because the error is fatal and you can't get it to run any userspace?
Wouldn't it be good to decode the whole error?

Right now we photograph screens on Intel x86 and feed the typed MCA info
by hand to mcelog. Hardly an optimal situation.

On AMD, there's a decoder which actually can dump the decoded critical
error. (Or could - that's in flux again :-\).

So yes, if stuff is too vendor-specific then you probably don't
want to decode it (or add a vendor-specific decoding module like
edac_mce_amd.ko, for example). But the tables from the UEFI spec,
documenting IP-specific error types which should be valid for most if
not all ARM64 implementations adhering to the spec, would be useful to
decode.

In general, the more we can decode the error in the kernel and the less
we need an external help to do so, the better.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1625709 — [PATCH V15 05/11] arm64: exception: handle Synchronous External Abort

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, report the error
in the kernel logs.
Update fault_info[] with specific SEA faults so that the
new SEA handler is used.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Reviewed-by: James Morse <james.morse@arm.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/esr.h |  1 +
 arch/arm64/mm/fault.c        | 45 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index d14c478..f20c64a 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -83,6 +83,7 @@
 #define ESR_ELx_WNR		(UL(1) << 6)
 
 /* Shared ISS field definitions for Data/Instruction aborts */
+#define ESR_ELx_FnV		(UL(1) << 10)
 #define ESR_ELx_EA		(UL(1) << 9)
 #define ESR_ELx_S1PTW		(UL(1) << 7)
 
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 1b35b8bd..b74d8b7 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -505,6 +505,31 @@ 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;
+	const struct fault_info *inf;
+
+	inf = esr_to_fault_info(esr);
+	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
+		inf->name, esr, addr);
+
+	info.si_signo = SIGBUS;
+	info.si_errno = 0;
+	info.si_code  = 0;
+	if (esr & ESR_ELx_FnV)
+		info.si_addr = 0;
+	else
+		info.si_addr  = (void __user *)addr;
+	arm64_notify_die("", regs, &info, esr);
+
+	return 0;
+}
+
 static const struct fault_info fault_info[] = {
 	{ do_bad,		SIGBUS,  0,		"ttbr address size fault"	},
 	{ do_bad,		SIGBUS,  0,		"level 1 address size fault"	},
@@ -522,22 +547,22 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 	{ 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 external abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error"	},
+	{ do_sea,		SIGBUS,  0,		"level 0 (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 1 (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 2 (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 3 (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"synchronous parity or ECC error" },
 	{ 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 synchronous parity error (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 1 synchronous parity error (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 2 synchronous parity error (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 3 synchronous parity error (translation table walk)"	},
 	{ 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.

[toc] | [prev] | [next] | [standalone]

#1625713 — [PATCH V15 03/11] cper: add timestamp print to CPER status printing

The ACPI 6.1 spec added a timestamp to the HEST generic data
structure. Print the timestamp out when printing out the error
status information.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Reviewed-by: James Morse <james.morse@arm.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/cper.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 8328a6f..46585f9 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -32,6 +32,8 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/aer.h>
+#include <linux/printk.h>
+#include <linux/bcd.h>
 #include <acpi/ghes.h>
 
 #define INDENT_SP	" "
@@ -387,6 +389,29 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
 	pfx, pcie->bridge.secondary_status, pcie->bridge.control);
 }
 
+static void cper_estatus_timestamp(const char *pfx,
+				   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);
+		sec       = bcd2bin(timestamp[0]);
+		min       = bcd2bin(timestamp[1]);
+		hour      = bcd2bin(timestamp[2]);
+		day       = bcd2bin(timestamp[4]);
+		mon       = bcd2bin(timestamp[5]);
+		year      = bcd2bin(timestamp[6]);
+		century   = bcd2bin(timestamp[7]);
+
+		if (*(timestamp + 3) & 0x1)
+			printk("%stimestamp is precise\n", pfx);
+
+		printk("%stime: %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
+			century, year, mon, day, hour, min, sec);
+	}
+}
+
 static void
 cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata,
 			   int sec_no)
@@ -395,6 +420,9 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
 	__u16 severity;
 	char newpfx[64];
 
+	if (acpi_hest_get_version(gdata) >= 3)
+		cper_estatus_timestamp(pfx, (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));
-- 
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.

[toc] | [prev] | [next] | [standalone]

#1628200 — Re: [PATCH V15 03/11] cper: add timestamp print to CPER status printing

On Tue, Apr 18, 2017 at 05:05:15PM -0600, Tyler Baicar wrote:
> The ACPI 6.1 spec added a timestamp to the HEST generic data

HEST?

I see the timestamp in

Table 18-343 Generic Error Data Entry

where those things are "One or more Generic Error Data Entry structures
may be recorded in the Generic Error Data Entries field of the Generic
Error Status Block structure."

And those are part of the "18.3.2.7 Generic Hardware Error Source",
i.e., GHES. So why do you say "HEST" above?

> structure. Print the timestamp out when printing out the error
> status information.
> 
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Reviewed-by: James Morse <james.morse@arm.com>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Remove those Reviewed-by:s

> ---
>  drivers/firmware/efi/cper.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 8328a6f..46585f9 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -32,6 +32,8 @@
>  #include <linux/acpi.h>
>  #include <linux/pci.h>
>  #include <linux/aer.h>
> +#include <linux/printk.h>
> +#include <linux/bcd.h>
>  #include <acpi/ghes.h>
>  
>  #define INDENT_SP	" "
> @@ -387,6 +389,29 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
>  	pfx, pcie->bridge.secondary_status, pcie->bridge.control);
>  }
>  
> +static void cper_estatus_timestamp(const char *pfx,

cper_print_tstamp()

> +				   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);
> +		sec       = bcd2bin(timestamp[0]);
> +		min       = bcd2bin(timestamp[1]);
> +		hour      = bcd2bin(timestamp[2]);
> +		day       = bcd2bin(timestamp[4]);
> +		mon       = bcd2bin(timestamp[5]);
> +		year      = bcd2bin(timestamp[6]);
> +		century   = bcd2bin(timestamp[7]);
> +
> +		if (*(timestamp + 3) & 0x1)
> +			printk("%stimestamp is precise\n", pfx);
> +
> +		printk("%stime: %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
> +			century, year, mon, day, hour, min, sec);

Yeah, about the precise tstamp, you can do something like this:

---
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 46585f92b741..a649884e2264 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -404,10 +404,8 @@ static void cper_estatus_timestamp(const char *pfx,
 		year      = bcd2bin(timestamp[6]);
 		century   = bcd2bin(timestamp[7]);
 
-		if (*(timestamp + 3) & 0x1)
-			printk("%stimestamp is precise\n", pfx);
-
-		printk("%stime: %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
+		printk("%s%ststamp: %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
+			(timestamp[3] & 0x1 ? "precise " : ""), 
 			century, year, mon, day, hour, min, sec);
 	}
 }

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1628414 — Re: [PATCH V15 03/11] cper: add timestamp print to CPER status printing

On Fri, Apr 21, 2017 at 10:04:35AM -0600, Baicar, Tyler wrote:
> This is basically what I already had in v14...you asked to move it into a
> different if-statement? https://lkml.org/lkml/2017/4/12/397

Well, clearly I've been smoking some nasty potent sh*t. :-\

/me goes and looks at the spec:

"Bit 0 – Timestamp is precise if this bit is set and correlates to the
time of the error event."

So why are we even printing the timestamp when !precise?

IOW, I think we should do:

	if (!(timestamp[3] & 0x1))
		printk("%stimestamp imprecise\n", pfx);
	else {
		sec = ..
		min = ...

		...
	}

and print the actual values only when the timestamp is precise.
Otherwise it has *some* values which could just as well be completely
random. And it's not like we're reporting the error tomorrow - it is
mostly a couple of seconds from logging to the fw pushing it out...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1628417 — Re: [PATCH V15 03/11] cper: add timestamp print to CPER status printing

On 4/21/2017 11:26 AM, Borislav Petkov wrote:
> On Fri, Apr 21, 2017 at 10:04:35AM -0600, Baicar, Tyler wrote:
>> This is basically what I already had in v14...you asked to move it into a
>> different if-statement? https://lkml.org/lkml/2017/4/12/397
> Well, clearly I've been smoking some nasty potent sh*t. :-\
>
> /me goes and looks at the spec:
>
> "Bit 0 – Timestamp is precise if this bit is set and correlates to the
> time of the error event."
>
> So why are we even printing the timestamp when !precise?
>
> IOW, I think we should do:
>
> 	if (!(timestamp[3] & 0x1))
> 		printk("%stimestamp imprecise\n", pfx);
> 	else {
> 		sec = ..
> 		min = ...
>
> 		...
> 	}
>
> and print the actual values only when the timestamp is precise.
> Otherwise it has *some* values which could just as well be completely
> random. And it's not like we're reporting the error tomorrow - it is
> mostly a couple of seconds from logging to the fw pushing it out...
The timestamp may still be useful when it is imprecise. In the polling 
case, you may only poll every minute or so, so the time may be useful. 
Also, I imagine there could be interrupt based errors happening much 
faster than the FW/OS handshake can happen. Maybe we can just use what I 
had before but also specify imprecise so that it is clear:

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

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.

[toc] | [prev] | [next] | [standalone]

#1628442 — Re: [PATCH V15 03/11] cper: add timestamp print to CPER status printing

On Fri, Apr 21, 2017 at 12:08:43PM -0600, Baicar, Tyler wrote:
> The timestamp may still be useful when it is imprecise. In the polling case,
> you may only poll every minute or so, so the time may be useful.

Well, what is in the timestamp when !precise? Some random time or some
timestamp from a couple of seconds ago? How do you differentiate what
timestamp is bollocks and what is from a while ago?

Is the imprecise tstamp really close to the time the error happened or
pointing at 1970 - the beginning of unix time? :-)

I'm sure you've picked up by now that we don't trust the firmware one
bit.

> Also, I imagine there could be interrupt based errors happening much faster than the
> FW/OS handshake can happen. Maybe we can just use what I had before but also
> specify imprecise so that it is clear:
> 
>         printk("%s%ststamp: %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
>             (timestamp[3] & 0x1 ? "precise " : "imprecise "),
>              century, year, mon, day, hour, min, sec);

I guess.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1628485 — Re: [PATCH V15 03/11] cper: add timestamp print to CPER status printing

On 4/21/2017 6:21 AM, Borislav Petkov wrote:
> On Tue, Apr 18, 2017 at 05:05:15PM -0600, Tyler Baicar wrote:
>> The ACPI 6.1 spec added a timestamp to the HEST generic data
> HEST?
>
> I see the timestamp in
>
> Table 18-343 Generic Error Data Entry
>
> where those things are "One or more Generic Error Data Entry structures
> may be recorded in the Generic Error Data Entries field of the Generic
> Error Status Block structure."
>
> And those are part of the "18.3.2.7 Generic Hardware Error Source",
> i.e., GHES. So why do you say "HEST" above?
Ah yes, this should be GHES no HEST. There are too many acronyms 
involved here :)
>
>> structure. Print the timestamp out when printing out the error
>> status information.
>>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Reviewed-by: James Morse <james.morse@arm.com>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Remove those Reviewed-by:s
>
>> ---
>>   drivers/firmware/efi/cper.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> index 8328a6f..46585f9 100644
>> --- a/drivers/firmware/efi/cper.c
>> +++ b/drivers/firmware/efi/cper.c
>> @@ -32,6 +32,8 @@
>>   #include <linux/acpi.h>
>>   #include <linux/pci.h>
>>   #include <linux/aer.h>
>> +#include <linux/printk.h>
>> +#include <linux/bcd.h>
>>   #include <acpi/ghes.h>
>>   
>>   #define INDENT_SP	" "
>> @@ -387,6 +389,29 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
>>   	pfx, pcie->bridge.secondary_status, pcie->bridge.control);
>>   }
>>   
>> +static void cper_estatus_timestamp(const char *pfx,
> cper_print_tstamp()
>
>> +				   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);
>> +		sec       = bcd2bin(timestamp[0]);
>> +		min       = bcd2bin(timestamp[1]);
>> +		hour      = bcd2bin(timestamp[2]);
>> +		day       = bcd2bin(timestamp[4]);
>> +		mon       = bcd2bin(timestamp[5]);
>> +		year      = bcd2bin(timestamp[6]);
>> +		century   = bcd2bin(timestamp[7]);
>> +
>> +		if (*(timestamp + 3) & 0x1)
>> +			printk("%stimestamp is precise\n", pfx);
>> +
>> +		printk("%stime: %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
>> +			century, year, mon, day, hour, min, sec);
> Yeah, about the precise tstamp, you can do something like this:
>
> ---
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 46585f92b741..a649884e2264 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -404,10 +404,8 @@ static void cper_estatus_timestamp(const char *pfx,
>   		year      = bcd2bin(timestamp[6]);
>   		century   = bcd2bin(timestamp[7]);
>   
> -		if (*(timestamp + 3) & 0x1)
> -			printk("%stimestamp is precise\n", pfx);
> -
> -		printk("%stime: %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
> +		printk("%s%ststamp: %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
> +			(timestamp[3] & 0x1 ? "precise " : ""),
>   			century, year, mon, day, hour, min, sec);
>   	}
>   }
This is basically what I already had in v14...you asked to move it into 
a different if-statement? https://lkml.org/lkml/2017/4/12/397

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web