This series adds support for the ARM Error Source Table (AEST) based on
the latest version of ACPI for the Armv8 RAS Extensions [0].
The AEST driver supports both memory mapped and system register interfaces.
This series assumes system register interfaces are only registered with
private peripheral interrupts (PPIs); otherwise there is no guarantee the
core handling the error is the core which took the error and has the
syndrome info in it's system registers.
This is meant to be initial support for AEST to address the current gaps
with systems that support ARMv8 RAS extensions but don't have
firmware-first support. This series simply logs all the errors it finds
and triggers a kernel panic if there is an UE present.
I have tested this series on Ampere Altra using processor errors to
exercise PPI handling with system register interface and memory errors
to exercise SPI handling with MMIO interface. Both corrected and
uncorrected errors were tested to verify the non-fatal vs fatal
scenarios.
Future work:
- UER handling to avoid panic
- Looping through all external abort capable (ERR<n>FR.UE != 0) error
nodes in SEA/SEI handling
Changes from RFC patch series [1]:
- Updated for latest AEST spec
- Utilize ACPICA header defines of AEST structures
- Added support for ARMv8.4 RAS extension
- Dropped the SEA/SEI dumping of SR RAS registers
- Removed unused defines
- Unified RAS extension register printing to a single function
- Updated trace event with additional fields
- Addressed other feedback from RFC series
- Added myself to ARM64 ACPI MAINTAINERS as a reviewer
[0] https://developer.arm.com/documentation/den0085/latest
[1] https://lkml.org/lkml/2019/7/2/781
Tyler Baicar (2):
ACPI/AEST: Initial AEST driver
trace, ras: add ARM RAS extension trace event
MAINTAINERS | 1 +
arch/arm64/include/asm/ras.h | 52 ++++
arch/arm64/include/asm/sysreg.h | 2 +
arch/arm64/kernel/Makefile | 1 +
arch/arm64/kernel/ras.c | 129 +++++++++
arch/arm64/kvm/sys_regs.c | 2 +
drivers/acpi/arm64/Kconfig | 3 +
drivers/acpi/arm64/Makefile | 1 +
drivers/acpi/arm64/aest.c | 455 ++++++++++++++++++++++++++++++++
include/linux/acpi_aest.h | 50 ++++
include/linux/cpuhotplug.h | 1 +
include/ras/ras_event.h | 55 ++++
12 files changed, 752 insertions(+)
create mode 100644 arch/arm64/include/asm/ras.h
create mode 100644 arch/arm64/kernel/ras.c
create mode 100644 drivers/acpi/arm64/aest.c
create mode 100644 include/linux/acpi_aest.h
--
2.33.1
Add support for parsing the ARM Error Source Table and basic handling of
errors reported through both memory mapped and system register interfaces.
Assume system register interfaces are only registered with private
peripheral interrupts (PPIs); otherwise there is no guarantee the
core handling the error is the core which took the error and has the
syndrome info in its system registers.
Add logging for all detected errors and trigger a kernel panic if there is
any uncorrected error present.
Signed-off-by: Tyler Baicar <[email protected]>
---
MAINTAINERS | 1 +
arch/arm64/include/asm/ras.h | 52 ++++
arch/arm64/include/asm/sysreg.h | 2 +
arch/arm64/kernel/Makefile | 1 +
arch/arm64/kernel/ras.c | 125 +++++++++
arch/arm64/kvm/sys_regs.c | 2 +
drivers/acpi/arm64/Kconfig | 3 +
drivers/acpi/arm64/Makefile | 1 +
drivers/acpi/arm64/aest.c | 450 ++++++++++++++++++++++++++++++++
include/linux/acpi_aest.h | 50 ++++
include/linux/cpuhotplug.h | 1 +
11 files changed, 688 insertions(+)
create mode 100644 arch/arm64/include/asm/ras.h
create mode 100644 arch/arm64/kernel/ras.c
create mode 100644 drivers/acpi/arm64/aest.c
create mode 100644 include/linux/acpi_aest.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 5250298d2817..aa0483726606 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -382,6 +382,7 @@ ACPI FOR ARM64 (ACPI/arm64)
M: Lorenzo Pieralisi <[email protected]>
M: Hanjun Guo <[email protected]>
M: Sudeep Holla <[email protected]>
+R: Tyler Baicar <[email protected]>
L: [email protected]
L: [email protected] (moderated for non-subscribers)
S: Maintained
diff --git a/arch/arm64/include/asm/ras.h b/arch/arm64/include/asm/ras.h
new file mode 100644
index 000000000000..e88fa93e5f1c
--- /dev/null
+++ b/arch/arm64/include/asm/ras.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_RAS_H
+#define __ASM_RAS_H
+
+#include <linux/types.h>
+#include <linux/bits.h>
+
+#define ERR_STATUS_AV BIT(31)
+#define ERR_STATUS_V BIT(30)
+#define ERR_STATUS_UE BIT(29)
+#define ERR_STATUS_MV BIT(26)
+#define ERR_STATUS_CE_MASK (BIT(25) | BIT(24))
+#define ERR_STATUS_DE BIT(23)
+#define ERR_STATUS_UET_MASK (BIT(21) | BIT(20))
+#define ERR_STATUS_IERR_SHIFT 8
+#define ERR_STATUS_IERR_MASK 0xff
+#define ERR_STATUS_SERR_SHIFT 0
+#define ERR_STATUS_SERR_MASK 0xff
+#define ERR_STATUS_W1TC_MASK 0xfff80000
+
+#define ERRIDR_NUM_MASK 0xffff
+
+#define ERRGSR_OFFSET 0xe00
+#define ERRDEVARCH_OFFSET 0xfbc
+
+#define ERRDEVARCH_REV_SHIFT 0x16
+#define ERRDEVARCH_REV_MASK 0xf
+
+#define RAS_REV_v1_1 0x1
+
+struct ras_ext_regs {
+ u64 err_fr;
+ u64 err_ctlr;
+ u64 err_status;
+ u64 err_addr;
+ u64 err_misc0;
+ u64 err_misc1;
+ u64 err_misc2;
+ u64 err_misc3;
+};
+
+#ifdef CONFIG_ARM64_RAS_EXTN
+void arch_arm_ras_print_error(struct ras_ext_regs *regs, unsigned int i, bool misc23_present);
+u64 arch_arm_ras_get_status_clear_value(u64 err_status);
+void arch_arm_ras_report_error(u64 implemented, bool clear_misc);
+#else
+void arch_arm_ras_print_error(struct ras_ext_regs *regs, unsigned int i, bool misc23_present) { }
+u64 arch_arm_ras_get_status_clear_value(u64 err_status) { return 0; }
+void arch_arm_ras_report_error(u64 implemented, bool clear_misc) { }
+#endif
+
+#endif /* __ASM_RAS_H */
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 16b3f1a1d468..6bbed061d835 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -230,6 +230,8 @@
#define SYS_ERXADDR_EL1 sys_reg(3, 0, 5, 4, 3)
#define SYS_ERXMISC0_EL1 sys_reg(3, 0, 5, 5, 0)
#define SYS_ERXMISC1_EL1 sys_reg(3, 0, 5, 5, 1)
+#define SYS_ERXMISC2_EL1 sys_reg(3, 0, 5, 5, 2)
+#define SYS_ERXMISC3_EL1 sys_reg(3, 0, 5, 5, 3)
#define SYS_TFSR_EL1 sys_reg(3, 0, 5, 6, 0)
#define SYS_TFSRE0_EL1 sys_reg(3, 0, 5, 6, 1)
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 88b3e2a21408..fe73844494ba 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_ARM64_PTR_AUTH) += pointer_auth.o
obj-$(CONFIG_ARM64_MTE) += mte.o
obj-y += vdso-wrap.o
obj-$(CONFIG_COMPAT_VDSO) += vdso32-wrap.o
+obj-$(CONFIG_ARM64_RAS_EXTN) += ras.o
obj-y += probes/
head-y := head.o
diff --git a/arch/arm64/kernel/ras.c b/arch/arm64/kernel/ras.c
new file mode 100644
index 000000000000..31e2036a4c70
--- /dev/null
+++ b/arch/arm64/kernel/ras.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+#include <linux/cpu.h>
+
+#include <asm/cpufeature.h>
+#include <asm/ras.h>
+
+static bool ras_extn_v1p1(void)
+{
+ unsigned long fld, reg = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
+
+ fld = cpuid_feature_extract_unsigned_field(reg, ID_AA64PFR0_RAS_SHIFT);
+
+ return fld >= ID_AA64PFR0_RAS_V1P1;
+}
+
+u64 arch_arm_ras_get_status_clear_value(u64 err_status)
+{
+ /* Write-one-to-clear the bits we've seen */
+ err_status &= ERR_STATUS_W1TC_MASK;
+
+ /* If CE field is non-zero, all bits must be written to properly clear */
+ if (err_status & ERR_STATUS_CE_MASK)
+ err_status |= ERR_STATUS_CE_MASK;
+
+ /* If UET field is non-zero, all bits must be written to properly clear */
+ if (err_status & ERR_STATUS_UET_MASK)
+ err_status |= ERR_STATUS_UET_MASK;
+
+ return err_status;
+}
+
+void arch_arm_ras_print_error(struct ras_ext_regs *regs, unsigned int i, bool misc23_present)
+{
+ pr_err(" ERR%uSTATUS: 0x%llx\n", i, regs->err_status);
+ if (regs->err_status & ERR_STATUS_AV)
+ pr_err(" ERR%uADDR: 0x%llx\n", i, regs->err_addr);
+
+ if (regs->err_status & ERR_STATUS_MV) {
+ pr_err(" ERR%uMISC0: 0x%llx\n", i, regs->err_misc0);
+ pr_err(" ERR%uMISC1: 0x%llx\n", i, regs->err_misc1);
+
+ if (misc23_present) {
+ pr_err(" ERR%uMISC2: 0x%llx\n", i, regs->err_misc2);
+ pr_err(" ERR%uMISC3: 0x%llx\n", i, regs->err_misc3);
+ }
+ }
+}
+
+#undef pr_fmt
+#define pr_fmt(fmt) "ARM RAS: " fmt
+
+void arch_arm_ras_report_error(u64 implemented, bool clear_misc)
+{
+ struct ras_ext_regs regs = {0};
+ unsigned int i, cpu_num;
+ bool misc23_present;
+ bool fatal = false;
+ u64 num_records;
+
+ if (!this_cpu_has_cap(ARM64_HAS_RAS_EXTN))
+ return;
+
+ cpu_num = get_cpu();
+ num_records = read_sysreg_s(SYS_ERRIDR_EL1) & ERRIDR_NUM_MASK;
+
+ for (i = 0; i < num_records; i++) {
+ if (!(implemented & BIT(i)))
+ continue;
+
+ write_sysreg_s(i, SYS_ERRSELR_EL1);
+ isb();
+ regs.err_status = read_sysreg_s(SYS_ERXSTATUS_EL1);
+
+ if (!(regs.err_status & ERR_STATUS_V))
+ continue;
+
+ pr_err("error from processor 0x%x\n", cpu_num);
+
+ if (regs.err_status & ERR_STATUS_AV)
+ regs.err_addr = read_sysreg_s(SYS_ERXADDR_EL1);
+
+ misc23_present = ras_extn_v1p1();
+
+ if (regs.err_status & ERR_STATUS_MV) {
+ regs.err_misc0 = read_sysreg_s(SYS_ERXMISC0_EL1);
+ regs.err_misc1 = read_sysreg_s(SYS_ERXMISC1_EL1);
+
+ if (misc23_present) {
+ regs.err_misc2 = read_sysreg_s(SYS_ERXMISC2_EL1);
+ regs.err_misc3 = read_sysreg_s(SYS_ERXMISC3_EL1);
+ }
+ }
+
+ arch_arm_ras_print_error(®s, i, misc23_present);
+
+ /*
+ * In the future, we will treat UER conditions as potentially
+ * recoverable.
+ */
+ if (regs.err_status & ERR_STATUS_UE)
+ fatal = true;
+
+ regs.err_status = arch_arm_ras_get_status_clear_value(regs.err_status);
+ write_sysreg_s(regs.err_status, SYS_ERXSTATUS_EL1);
+
+ if (clear_misc) {
+ write_sysreg_s(0x0, SYS_ERXMISC0_EL1);
+ write_sysreg_s(0x0, SYS_ERXMISC1_EL1);
+
+ if (misc23_present) {
+ write_sysreg_s(0x0, SYS_ERXMISC2_EL1);
+ write_sysreg_s(0x0, SYS_ERXMISC3_EL1);
+ }
+ }
+
+ isb();
+ }
+
+ if (fatal)
+ panic("ARM RAS: uncorrectable error encountered");
+
+ put_cpu();
+}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e3ec1a44f94d..dc15e9896db4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1573,6 +1573,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
{ SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
{ SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
+ { SYS_DESC(SYS_ERXMISC2_EL1), trap_raz_wi },
+ { SYS_DESC(SYS_ERXMISC3_EL1), trap_raz_wi },
MTE_REG(TFSR_EL1),
MTE_REG(TFSRE0_EL1),
diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
index 6dba187f4f2e..8d5cf99976c8 100644
--- a/drivers/acpi/arm64/Kconfig
+++ b/drivers/acpi/arm64/Kconfig
@@ -8,3 +8,6 @@ config ACPI_IORT
config ACPI_GTDT
bool
+
+config ACPI_AEST
+ bool "ARM Error Source Table Support"
diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
index 66acbe77f46e..8f60a9fb6ab1 100644
--- a/drivers/acpi/arm64/Makefile
+++ b/drivers/acpi/arm64/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_ACPI_IORT) += iort.o
obj-$(CONFIG_ACPI_GTDT) += gtdt.o
obj-y += dma.o
+obj-$(CONFIG_ACPI_AEST) += aest.o
diff --git a/drivers/acpi/arm64/aest.c b/drivers/acpi/arm64/aest.c
new file mode 100644
index 000000000000..2df4f2377e51
--- /dev/null
+++ b/drivers/acpi/arm64/aest.c
@@ -0,0 +1,450 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM Error Source Table Support
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ */
+
+#include <linux/acpi.h>
+#include <linux/acpi_aest.h>
+#include <linux/cpuhotplug.h>
+#include <linux/kernel.h>
+
+#include <acpi/actbl.h>
+
+#include <asm/ras.h>
+
+#undef pr_fmt
+#define pr_fmt(fmt) "ACPI AEST: " fmt
+
+static struct acpi_table_header *aest_table;
+
+static struct aest_node_data __percpu **ppi_data;
+static int ppi_irqs[AEST_MAX_PPI];
+static u8 num_ppi;
+static u8 ppi_idx;
+
+static bool aest_mmio_ras_misc23_present(u64 base_addr)
+{
+ u32 val;
+
+ val = readl((void *) (base_addr + ERRDEVARCH_OFFSET));
+ val <<= ERRDEVARCH_REV_SHIFT;
+ val &= ERRDEVARCH_REV_MASK;
+
+ return val >= RAS_REV_v1_1;
+}
+
+static void aest_print(struct aest_node_data *data, struct ras_ext_regs regs,
+ int index, bool misc23_present)
+{
+ /* No more than 2 corrected messages every 5 seconds */
+ static DEFINE_RATELIMIT_STATE(ratelimit_corrected, 5*HZ, 2);
+
+ if (regs.err_status & ERR_STATUS_UE ||
+ regs.err_status & ERR_STATUS_DE ||
+ __ratelimit(&ratelimit_corrected)) {
+ switch (data->node_type) {
+ case ACPI_AEST_PROCESSOR_ERROR_NODE:
+ if (!(data->data.processor.flags & AEST_PROC_GLOBAL) &&
+ !(data->data.processor.flags & AEST_PROC_SHARED))
+ pr_err("error from processor 0x%x\n",
+ data->data.processor.processor_id);
+ break;
+ case ACPI_AEST_MEMORY_ERROR_NODE:
+ pr_err("error from memory at SRAT proximity domain 0x%x\n",
+ data->data.memory.srat_proximity_domain);
+ break;
+ case ACPI_AEST_SMMU_ERROR_NODE:
+ pr_err("error from SMMU IORT node 0x%x subcomponent 0x%x\n",
+ data->data.smmu.iort_node_reference,
+ data->data.smmu.subcomponent_reference);
+ break;
+ case ACPI_AEST_VENDOR_ERROR_NODE:
+ pr_err("error from vendor hid 0x%x uid 0x%x\n",
+ data->data.vendor.acpi_hid, data->data.vendor.acpi_uid);
+ break;
+ case ACPI_AEST_GIC_ERROR_NODE:
+ pr_err("error from GIC type 0x%x instance 0x%x\n",
+ data->data.gic.interface_type, data->data.gic.instance_id);
+ }
+
+ arch_arm_ras_print_error(®s, index, misc23_present);
+ }
+}
+
+static void aest_proc(struct aest_node_data *data)
+{
+ struct ras_ext_regs *regs_p, regs = {0};
+ bool misc23_present;
+ bool fatal = false;
+ u64 errgsr = 0;
+ int i;
+
+ /*
+ * Currently SR based handling is done through the architected
+ * discovery exposed through SRs. That may change in the future
+ * if there is supplemental information in the AEST that is
+ * needed.
+ */
+ if (data->interface.type == ACPI_AEST_NODE_SYSTEM_REGISTER) {
+ arch_arm_ras_report_error(data->interface.implemented,
+ data->interface.flags & AEST_INTERFACE_CLEAR_MISC);
+ return;
+ }
+
+ regs_p = data->interface.regs;
+ errgsr = readq((void *) (((u64) regs_p) + ERRGSR_OFFSET));
+
+ for (i = data->interface.start; i < data->interface.end; i++) {
+ if (!(data->interface.implemented & BIT(i)))
+ continue;
+
+ if (!(data->interface.status_reporting & BIT(i)) && !(errgsr & BIT(i)))
+ continue;
+
+ regs.err_status = readq(®s_p[i].err_status);
+ if (!(regs.err_status & ERR_STATUS_V))
+ continue;
+
+ if (regs.err_status & ERR_STATUS_AV)
+ regs.err_addr = readq(®s_p[i].err_addr);
+
+ regs.err_fr = readq(®s_p[i].err_fr);
+ regs.err_ctlr = readq(®s_p[i].err_ctlr);
+
+ if (regs.err_status & ERR_STATUS_MV) {
+ misc23_present = aest_mmio_ras_misc23_present((u64) regs_p);
+ regs.err_misc0 = readq(®s_p[i].err_misc0);
+ regs.err_misc1 = readq(®s_p[i].err_misc1);
+
+ if (misc23_present) {
+ regs.err_misc2 = readq(®s_p[i].err_misc2);
+ regs.err_misc3 = readq(®s_p[i].err_misc3);
+ }
+ }
+
+ aest_print(data, regs, i, misc23_present);
+
+ if (regs.err_status & ERR_STATUS_UE)
+ fatal = true;
+
+ regs.err_status = arch_arm_ras_get_status_clear_value(regs.err_status);
+ writeq(regs.err_status, ®s_p[i].err_status);
+
+ if (data->interface.flags & AEST_INTERFACE_CLEAR_MISC) {
+ writeq(0x0, ®s_p[i].err_misc0);
+ writeq(0x0, ®s_p[i].err_misc1);
+
+ if (misc23_present) {
+ writeq(0x0, ®s_p[i].err_misc2);
+ writeq(0x0, ®s_p[i].err_misc3);
+ }
+ }
+ }
+
+ if (fatal)
+ panic("AEST: uncorrectable error encountered");
+}
+
+static irqreturn_t aest_irq_func(int irq, void *input)
+{
+ struct aest_node_data *data = input;
+
+ aest_proc(data);
+
+ return IRQ_HANDLED;
+}
+
+static int __init aest_register_gsi(u32 gsi, int trigger, void *data)
+{
+ int cpu, irq;
+
+ irq = acpi_register_gsi(NULL, gsi, trigger, ACPI_ACTIVE_HIGH);
+
+ if (irq == -EINVAL) {
+ pr_err("failed to map AEST GSI %d\n", gsi);
+ return -EINVAL;
+ }
+
+ if (gsi < 16) {
+ pr_err("invalid GSI %d\n", gsi);
+ return -EINVAL;
+ } else if (gsi < 32) {
+ if (ppi_idx >= AEST_MAX_PPI) {
+ pr_err("Unable to register PPI %d\n", gsi);
+ return -EINVAL;
+ }
+ ppi_irqs[ppi_idx] = irq;
+ enable_percpu_irq(irq, IRQ_TYPE_NONE);
+ for_each_possible_cpu(cpu) {
+ memcpy(per_cpu_ptr(ppi_data[ppi_idx], cpu), data,
+ sizeof(struct aest_node_data));
+ }
+ if (request_percpu_irq(irq, aest_irq_func, "AEST",
+ ppi_data[ppi_idx++])) {
+ pr_err("failed to register AEST IRQ %d\n", irq);
+ return -EINVAL;
+ }
+ } else if (gsi < 1020) {
+ if (request_irq(irq, aest_irq_func, IRQF_SHARED, "AEST",
+ data)) {
+ pr_err("failed to register AEST IRQ %d\n", irq);
+ return -EINVAL;
+ }
+ } else {
+ pr_err("invalid GSI %d\n", gsi);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int __init aest_init_interrupts(struct acpi_aest_hdr *node,
+ struct aest_node_data *data)
+{
+ struct acpi_aest_node_interrupt *interrupt;
+ int i, trigger, ret = 0;
+
+ interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
+ node->node_interrupt_offset);
+
+ for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
+ trigger = (interrupt->flags & AEST_INTERRUPT_MODE) ?
+ ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
+ if (aest_register_gsi(interrupt->gsiv, trigger, data))
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static int __init aest_init_interface(struct acpi_aest_hdr *node,
+ struct aest_node_data *data)
+{
+ struct acpi_aest_node_interface *interface;
+ struct resource *res;
+ int size;
+
+ interface = ACPI_ADD_PTR(struct acpi_aest_node_interface, node,
+ node->node_interface_offset);
+
+ if (interface->type >= ACPI_AEST_XFACE_RESERVED) {
+ pr_err("invalid interface type: %d\n", interface->type);
+ return -EINVAL;
+ }
+
+ data->interface.type = interface->type;
+ data->interface.start = interface->error_record_index;
+ data->interface.end = interface->error_record_index + interface->error_record_count;
+ data->interface.flags = interface->flags;
+ data->interface.implemented = interface->error_record_implemented;
+ data->interface.status_reporting = interface->error_status_reporting;
+
+ /*
+ * Currently SR based handling is done through the architected
+ * discovery exposed through SRs. That may change in the future
+ * if there is supplemental information in the AEST that is
+ * needed.
+ */
+ if (interface->type == ACPI_AEST_NODE_SYSTEM_REGISTER)
+ return 0;
+
+ res = kzalloc(sizeof(struct resource), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ size = interface->error_record_count * sizeof(struct ras_ext_regs);
+ res->name = "AEST";
+ res->start = interface->address;
+ res->end = res->start + size;
+ res->flags = IORESOURCE_MEM;
+ if (request_resource_conflict(&iomem_resource, res)) {
+ pr_err("unable to request region starting at 0x%llx\n",
+ res->start);
+ kfree(res);
+ return -EEXIST;
+ }
+
+ data->interface.regs = ioremap(interface->address, size);
+ if (data->interface.regs == NULL) {
+ kfree(res);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int __init aest_init_node(struct acpi_aest_hdr *node)
+{
+ union acpi_aest_processor_data *proc_data;
+ union aest_node_spec *node_spec;
+ struct aest_node_data *data;
+ int ret;
+
+ data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->node_type = node->type;
+
+ node_spec = ACPI_ADD_PTR(union aest_node_spec, node, node->node_specific_offset);
+
+ switch (node->type) {
+ case ACPI_AEST_PROCESSOR_ERROR_NODE:
+ memcpy(&data->data, node_spec, sizeof(struct acpi_aest_processor));
+ break;
+ case ACPI_AEST_MEMORY_ERROR_NODE:
+ memcpy(&data->data, node_spec, sizeof(struct acpi_aest_memory));
+ break;
+ case ACPI_AEST_SMMU_ERROR_NODE:
+ memcpy(&data->data, node_spec, sizeof(struct acpi_aest_smmu));
+ break;
+ case ACPI_AEST_VENDOR_ERROR_NODE:
+ memcpy(&data->data, node_spec, sizeof(struct acpi_aest_vendor));
+ break;
+ case ACPI_AEST_GIC_ERROR_NODE:
+ memcpy(&data->data, node_spec, sizeof(struct acpi_aest_gic));
+ break;
+ default:
+ kfree(data);
+ return -EINVAL;
+ }
+
+ if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
+ proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data, node_spec,
+ sizeof(acpi_aest_processor));
+
+ switch (data->data.processor.resource_type) {
+ case ACPI_AEST_CACHE_RESOURCE:
+ memcpy(&data->proc_data, proc_data,
+ sizeof(struct acpi_aest_processor_cache));
+ break;
+ case ACPI_AEST_TLB_RESOURCE:
+ memcpy(&data->proc_data, proc_data,
+ sizeof(struct acpi_aest_processor_tlb));
+ break;
+ case ACPI_AEST_GENERIC_RESOURCE:
+ memcpy(&data->proc_data, proc_data,
+ sizeof(struct acpi_aest_processor_generic));
+ break;
+ }
+ }
+
+ ret = aest_init_interface(node, data);
+ if (ret) {
+ kfree(data);
+ return ret;
+ }
+
+ return aest_init_interrupts(node, data);
+}
+
+static void aest_count_ppi(struct acpi_aest_hdr *node)
+{
+ struct acpi_aest_node_interrupt *interrupt;
+ int i;
+
+ interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
+ node->node_interrupt_offset);
+
+ for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
+ if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
+ num_ppi++;
+ }
+}
+
+static int aest_starting_cpu(unsigned int cpu)
+{
+ int i;
+
+ for (i = 0; i < num_ppi; i++)
+ enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
+
+ return 0;
+}
+
+static int aest_dying_cpu(unsigned int cpu)
+{
+ return 0;
+}
+
+int __init acpi_aest_init(void)
+{
+ struct acpi_aest_hdr *aest_node, *aest_end;
+ struct acpi_table_aest *aest;
+ int i, ret = 0;
+
+ if (acpi_disabled)
+ return 0;
+
+ if (!IS_ENABLED(CONFIG_ARM64_RAS_EXTN))
+ return 0;
+
+ if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_AEST, 0, &aest_table)))
+ return -EINVAL;
+
+ aest = (struct acpi_table_aest *)aest_table;
+
+ /* Get the first AEST node */
+ aest_node = ACPI_ADD_PTR(struct acpi_aest_hdr, aest,
+ sizeof(struct acpi_table_header));
+ /* Pointer to the end of the AEST table */
+ aest_end = ACPI_ADD_PTR(struct acpi_aest_hdr, aest,
+ aest_table->length);
+
+ while (aest_node < aest_end) {
+ if (((u64)aest_node + aest_node->length) > (u64)aest_end) {
+ pr_err("AEST node pointer overflow, bad table\n");
+ return -EINVAL;
+ }
+
+ aest_count_ppi(aest_node);
+
+ aest_node = ACPI_ADD_PTR(struct acpi_aest_hdr, aest_node,
+ aest_node->length);
+ }
+
+ if (num_ppi > AEST_MAX_PPI) {
+ pr_err("Limiting PPI support to %d PPIs\n", AEST_MAX_PPI);
+ num_ppi = AEST_MAX_PPI;
+ }
+
+ ppi_data = kcalloc(num_ppi, sizeof(struct aest_node_data *),
+ GFP_KERNEL);
+
+ for (i = 0; i < num_ppi; i++) {
+ ppi_data[i] = alloc_percpu(struct aest_node_data);
+ if (!ppi_data[i]) {
+ pr_err("Failed percpu allocation\n");
+ ret = -ENOMEM;
+ goto fail;
+ }
+ }
+
+ aest_node = ACPI_ADD_PTR(struct acpi_aest_hdr, aest,
+ sizeof(struct acpi_table_header));
+
+ while (aest_node < aest_end) {
+ ret = aest_init_node(aest_node);
+ if (ret)
+ pr_err("failed to init node: %d", ret);
+
+ aest_node = ACPI_ADD_PTR(struct acpi_aest_hdr, aest_node,
+ aest_node->length);
+ }
+
+ cpuhp_setup_state(CPUHP_AP_ARM_AEST_STARTING,
+ "drivers/acpi/arm64/aest:starting",
+ aest_starting_cpu, aest_dying_cpu);
+
+ return 0;
+
+fail:
+ for (i = 0; i < num_ppi; i++)
+ free_percpu(ppi_data[i]);
+ kfree(ppi_data);
+ return ret;
+}
+
+early_initcall(acpi_aest_init);
diff --git a/include/linux/acpi_aest.h b/include/linux/acpi_aest.h
new file mode 100644
index 000000000000..492503f54ebc
--- /dev/null
+++ b/include/linux/acpi_aest.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef AEST_H
+#define AEST_H
+
+#include <acpi/actbl.h>
+
+#define ACPI_SIG_AEST "AEST" /* ARM Error Source Table */
+
+#define AEST_INTERRUPT_MODE BIT(0)
+
+#define AEST_MAX_PPI 4
+
+#define AEST_PROC_GLOBAL BIT(0)
+#define AEST_PROC_SHARED BIT(1)
+
+#define AEST_INTERFACE_SHARED BIT(0)
+#define AEST_INTERFACE_CLEAR_MISC BIT(1)
+
+struct aest_interface_data {
+ u8 type;
+ u16 start;
+ u16 end;
+ u32 flags;
+ u64 implemented;
+ u64 status_reporting;
+ struct ras_ext_regs *regs;
+};
+
+union acpi_aest_processor_data {
+ struct acpi_aest_processor_cache cache_data;
+ struct acpi_aest_processor_tlb tlb_data;
+ struct acpi_aest_processor_generic generic_data;
+};
+
+union aest_node_spec {
+ struct acpi_aest_processor processor;
+ struct acpi_aest_memory memory;
+ struct acpi_aest_smmu smmu;
+ struct acpi_aest_vendor vendor;
+ struct acpi_aest_gic gic;
+};
+
+struct aest_node_data {
+ u8 node_type;
+ struct aest_interface_data interface;
+ union aest_node_spec data;
+ union acpi_aest_processor_data proc_data;
+};
+
+#endif /* AEST_H */
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 773c83730906..9d30e4c00a52 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -186,6 +186,7 @@ enum cpuhp_state {
CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
CPUHP_AP_KVM_ARM_VGIC_STARTING,
CPUHP_AP_KVM_ARM_TIMER_STARTING,
+ CPUHP_AP_ARM_AEST_STARTING,
/* Must be the last timer callback */
CPUHP_AP_DUMMY_TIMER_STARTING,
CPUHP_AP_ARM_XEN_STARTING,
--
2.33.1
Add a trace event for hardware errors reported by the ARMv8
RAS extension registers.
Signed-off-by: Tyler Baicar <[email protected]>
---
arch/arm64/kernel/ras.c | 4 +++
drivers/acpi/arm64/aest.c | 5 ++++
include/ras/ras_event.h | 55 +++++++++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+)
diff --git a/arch/arm64/kernel/ras.c b/arch/arm64/kernel/ras.c
index 31e2036a4c70..18071790b2a3 100644
--- a/arch/arm64/kernel/ras.c
+++ b/arch/arm64/kernel/ras.c
@@ -6,6 +6,8 @@
#include <asm/cpufeature.h>
#include <asm/ras.h>
+#include <ras/ras_event.h>
+
static bool ras_extn_v1p1(void)
{
unsigned long fld, reg = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
@@ -95,6 +97,8 @@ void arch_arm_ras_report_error(u64 implemented, bool clear_misc)
arch_arm_ras_print_error(®s, i, misc23_present);
+ trace_arm_ras_ext_event(0, cpu_num, 0, i, ®s);
+
/*
* In the future, we will treat UER conditions as potentially
* recoverable.
diff --git a/drivers/acpi/arm64/aest.c b/drivers/acpi/arm64/aest.c
index 2df4f2377e51..7ef1750f91b3 100644
--- a/drivers/acpi/arm64/aest.c
+++ b/drivers/acpi/arm64/aest.c
@@ -14,6 +14,8 @@
#include <asm/ras.h>
+#include <ras/ras_event.h>
+
#undef pr_fmt
#define pr_fmt(fmt) "ACPI AEST: " fmt
@@ -126,6 +128,9 @@ static void aest_proc(struct aest_node_data *data)
aest_print(data, regs, i, misc23_present);
+ trace_arm_ras_ext_event(data->node_type, data->data.vendor.acpi_hid,
+ data->data.vendor.acpi_uid, i, ®s);
+
if (regs.err_status & ERR_STATUS_UE)
fatal = true;
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 0bdbc0d17d2f..27b2be9f950d 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -338,6 +338,61 @@ TRACE_EVENT(aer_event,
"Not available")
);
+/*
+ * ARM RAS Extension Events Report
+ *
+ * This event is generated when an error reported by the ARM RAS extension
+ * hardware is detected.
+ */
+
+#ifdef CONFIG_ARM64_RAS_EXTN
+#include <asm/ras.h>
+TRACE_EVENT(arm_ras_ext_event,
+
+ TP_PROTO(u8 type, u32 id0, u32 id1, u32 index, struct ras_ext_regs *regs),
+
+ TP_ARGS(type, id0, id1, index, regs),
+
+ TP_STRUCT__entry(
+ __field(u8, type)
+ __field(u32, id0)
+ __field(u32, id1)
+ __field(u32, index)
+ __field(u64, err_fr)
+ __field(u64, err_ctlr)
+ __field(u64, err_status)
+ __field(u64, err_addr)
+ __field(u64, err_misc0)
+ __field(u64, err_misc1)
+ __field(u64, err_misc2)
+ __field(u64, err_misc3)
+ ),
+
+ TP_fast_assign(
+ __entry->type = type;
+ __entry->id0 = id0;
+ __entry->id1 = id1;
+ __entry->index = index;
+ __entry->err_fr = regs->err_fr;
+ __entry->err_ctlr = regs->err_ctlr;
+ __entry->err_status = regs->err_status;
+ __entry->err_addr = regs->err_addr;
+ __entry->err_misc0 = regs->err_misc0;
+ __entry->err_misc1 = regs->err_misc1;
+ __entry->err_misc2 = regs->err_misc2;
+ __entry->err_misc3 = regs->err_misc3;
+ ),
+
+ TP_printk("type: %d; id0: %d; id1: %d; index: %d; ERR_FR: %llx; ERR_CTLR: %llx; "
+ "ERR_STATUS: %llx; ERR_ADDR: %llx; ERR_MISC0: %llx; ERR_MISC1: %llx; "
+ "ERR_MISC2: %llx; ERR_MISC3: %llx",
+ __entry->type, __entry->id0, __entry->id1, __entry->index, __entry->err_fr,
+ __entry->err_ctlr, __entry->err_status, __entry->err_addr,
+ __entry->err_misc0, __entry->err_misc1, __entry->err_misc2,
+ __entry->err_misc3)
+);
+#endif
+
/*
* memory-failure recovery action result event
*
--
2.33.1
On Wed, 24 Nov 2021 17:07:07 +0000,
Tyler Baicar <[email protected]> wrote:
>
> Add support for parsing the ARM Error Source Table and basic handling of
> errors reported through both memory mapped and system register interfaces.
>
> Assume system register interfaces are only registered with private
> peripheral interrupts (PPIs); otherwise there is no guarantee the
> core handling the error is the core which took the error and has the
> syndrome info in its system registers.
>
> Add logging for all detected errors and trigger a kernel panic if there is
> any uncorrected error present.
>
> Signed-off-by: Tyler Baicar <[email protected]>
> ---
> MAINTAINERS | 1 +
> arch/arm64/include/asm/ras.h | 52 ++++
> arch/arm64/include/asm/sysreg.h | 2 +
> arch/arm64/kernel/Makefile | 1 +
> arch/arm64/kernel/ras.c | 125 +++++++++
> arch/arm64/kvm/sys_regs.c | 2 +
> drivers/acpi/arm64/Kconfig | 3 +
> drivers/acpi/arm64/Makefile | 1 +
> drivers/acpi/arm64/aest.c | 450 ++++++++++++++++++++++++++++++++
> include/linux/acpi_aest.h | 50 ++++
> include/linux/cpuhotplug.h | 1 +
> 11 files changed, 688 insertions(+)
> create mode 100644 arch/arm64/include/asm/ras.h
> create mode 100644 arch/arm64/kernel/ras.c
> create mode 100644 drivers/acpi/arm64/aest.c
> create mode 100644 include/linux/acpi_aest.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5250298d2817..aa0483726606 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -382,6 +382,7 @@ ACPI FOR ARM64 (ACPI/arm64)
> M: Lorenzo Pieralisi <[email protected]>
> M: Hanjun Guo <[email protected]>
> M: Sudeep Holla <[email protected]>
> +R: Tyler Baicar <[email protected]>
> L: [email protected]
> L: [email protected] (moderated for non-subscribers)
> S: Maintained
Isn't this a bit premature? This isn't even mentioned in the commit
message, only in passing in the cover letter.
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 16b3f1a1d468..6bbed061d835 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -230,6 +230,8 @@
> #define SYS_ERXADDR_EL1 sys_reg(3, 0, 5, 4, 3)
> #define SYS_ERXMISC0_EL1 sys_reg(3, 0, 5, 5, 0)
> #define SYS_ERXMISC1_EL1 sys_reg(3, 0, 5, 5, 1)
> +#define SYS_ERXMISC2_EL1 sys_reg(3, 0, 5, 5, 2)
> +#define SYS_ERXMISC3_EL1 sys_reg(3, 0, 5, 5, 3)
> #define SYS_TFSR_EL1 sys_reg(3, 0, 5, 6, 0)
> #define SYS_TFSRE0_EL1 sys_reg(3, 0, 5, 6, 1)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e3ec1a44f94d..dc15e9896db4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1573,6 +1573,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
> { SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
> { SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
> + { SYS_DESC(SYS_ERXMISC2_EL1), trap_raz_wi },
> + { SYS_DESC(SYS_ERXMISC3_EL1), trap_raz_wi },
>
This looks like a fix that would deserve its own patch.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Hi,
I haven't looked at this in great detail, but I spotted a few issues
from an initial scan.
On Wed, Nov 24, 2021 at 12:07:07PM -0500, Tyler Baicar wrote:
> Add support for parsing the ARM Error Source Table and basic handling of
> errors reported through both memory mapped and system register interfaces.
>
> Assume system register interfaces are only registered with private
> peripheral interrupts (PPIs); otherwise there is no guarantee the
> core handling the error is the core which took the error and has the
> syndrome info in its system registers.
Can we actually assume that? What does the specification mandate?
> Add logging for all detected errors and trigger a kernel panic if there is
> any uncorrected error present.
Has this been tested on any hardware or software platform?
[...]
> +#define ERRDEVARCH_REV_SHIFT 0x16
IIUC This should be 16, not 0x16 (i.e. 22).
> +#define ERRDEVARCH_REV_MASK 0xf
> +
> +#define RAS_REV_v1_1 0x1
> +
> +struct ras_ext_regs {
> + u64 err_fr;
> + u64 err_ctlr;
> + u64 err_status;
> + u64 err_addr;
> + u64 err_misc0;
> + u64 err_misc1;
> + u64 err_misc2;
> + u64 err_misc3;
> +};
These last four might be better an an array.
[...]
> +static bool ras_extn_v1p1(void)
> +{
> + unsigned long fld, reg = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
> +
> + fld = cpuid_feature_extract_unsigned_field(reg, ID_AA64PFR0_RAS_SHIFT);
> +
> + return fld >= ID_AA64PFR0_RAS_V1P1;
> +}
I suspect it'd be better to pass this value around directly as
`version`, rather than dropping this into a `misc23_present` temporary
variable, as that would be a little clearer, and future-proof if/when
more registers get added.
[...]
> +void arch_arm_ras_report_error(u64 implemented, bool clear_misc)
> +{
> + struct ras_ext_regs regs = {0};
> + unsigned int i, cpu_num;
> + bool misc23_present;
> + bool fatal = false;
> + u64 num_records;
> +
> + if (!this_cpu_has_cap(ARM64_HAS_RAS_EXTN))
> + return;
> +
> + cpu_num = get_cpu();
Why get_cpu() here? Do you just need smp_processor_id()?
The commit message explained that this would be PE-local (e.g. in a PPI
handler), and we've already checked this_cpu_has_cap() which assumes
we're not preemptible.
So I don't see why we should use get_cpu() here -- any time it would
have a difference implies something has already gone wrong.
> + num_records = read_sysreg_s(SYS_ERRIDR_EL1) & ERRIDR_NUM_MASK;
> +
> + for (i = 0; i < num_records; i++) {
> + if (!(implemented & BIT(i)))
> + continue;
> +
> + write_sysreg_s(i, SYS_ERRSELR_EL1);
> + isb();
> + regs.err_status = read_sysreg_s(SYS_ERXSTATUS_EL1);
> +
> + if (!(regs.err_status & ERR_STATUS_V))
> + continue;
> +
> + pr_err("error from processor 0x%x\n", cpu_num);
Why in hex? We normally print 'cpu%d' or 'CPU%d', since this is a
logical ID anyway.
> +
> + if (regs.err_status & ERR_STATUS_AV)
> + regs.err_addr = read_sysreg_s(SYS_ERXADDR_EL1);
> +
> + misc23_present = ras_extn_v1p1();
As above, I reckon it's better to have this as 'version' or
'ras_version', and have the checks below be:
if (version >= ID_AA64PFR0_RAS_V1P1) {
// poke SYS_ERXMISC2_EL1
// poke SYS_ERXMISC3_EL1
}
> +
> + if (regs.err_status & ERR_STATUS_MV) {
> + regs.err_misc0 = read_sysreg_s(SYS_ERXMISC0_EL1);
> + regs.err_misc1 = read_sysreg_s(SYS_ERXMISC1_EL1);
> +
> + if (misc23_present) {
> + regs.err_misc2 = read_sysreg_s(SYS_ERXMISC2_EL1);
> + regs.err_misc3 = read_sysreg_s(SYS_ERXMISC3_EL1);
> + }
> + }
> +
> + arch_arm_ras_print_error(®s, i, misc23_present);
> +
> + /*
> + * In the future, we will treat UER conditions as potentially
> + * recoverable.
> + */
> + if (regs.err_status & ERR_STATUS_UE)
> + fatal = true;
> +
> + regs.err_status = arch_arm_ras_get_status_clear_value(regs.err_status);
> + write_sysreg_s(regs.err_status, SYS_ERXSTATUS_EL1);
> +
> + if (clear_misc) {
> + write_sysreg_s(0x0, SYS_ERXMISC0_EL1);
> + write_sysreg_s(0x0, SYS_ERXMISC1_EL1);
> +
> + if (misc23_present) {
> + write_sysreg_s(0x0, SYS_ERXMISC2_EL1);
> + write_sysreg_s(0x0, SYS_ERXMISC3_EL1);
> + }
> + }
Any reason not to clear when we read, above? e.g.
#define READ_CLEAR_MISC(nr, clear) \
({ \
unsigned long __val = read_sysreg_s(SYS_ERXMISC##nr##_EL1); \
if (clear); \
write_sysreg_s(0, SYS_ERXMISC##nr##_EL1); \
__val; \
})
if (regs.err_status & ERR_STATUS_MV) {
regs.err_misc0 = READ_CLEAR_MISC(0, clear_misc);
regs.err_misc1 = READ_CLEAR_MISC(1, clear_misc);
if (version >= ID_AA64PFR0_RAS_V1P1) {
regs.err_misc2 = READ_CLEAR_MISC(2, clear_misc);
regs.err_misc3 = READ_CLEAR_MISC(3, clear_misc);
}
}
... why does the clearing need to be conditional?
> +
> + isb();
> + }
> +
> + if (fatal)
> + panic("ARM RAS: uncorrectable error encountered");
> +
> + put_cpu();
> +}
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e3ec1a44f94d..dc15e9896db4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1573,6 +1573,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
> { SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
> { SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
> + { SYS_DESC(SYS_ERXMISC2_EL1), trap_raz_wi },
> + { SYS_DESC(SYS_ERXMISC3_EL1), trap_raz_wi },
This should be a preparatory patch; this is preumably a latent bug in
KVM.
[...]
> +static struct aest_node_data __percpu **ppi_data;
> +static int ppi_irqs[AEST_MAX_PPI];
> +static u8 num_ppi;
> +static u8 ppi_idx;
As above, do we have any guarantee these are actually PPIs?
> +static bool aest_mmio_ras_misc23_present(u64 base_addr)
> +{
> + u32 val;
> +
> + val = readl((void *) (base_addr + ERRDEVARCH_OFFSET));
> + val <<= ERRDEVARCH_REV_SHIFT;
> + val &= ERRDEVARCH_REV_MASK;
> +
> + return val >= RAS_REV_v1_1;
> +}
Is the shift the wrong way around?
Above we have:
#define ERRDEVARCH_REV_SHIFT 0x16
#define ERRDEVARCH_REV_MASK 0xf
#define RAS_REV_v1_1 0x1
.. so this is:
val <<= 0x16;
val &= 0xf; // val[0x15:0] == 0, so this is 0
return val >= 0x1; // false
It'd be nicer to use FIELD_GET() here.
As above, I also think it would be better to retrun the value of the
field, and check that explciitly, for future proofing.
[...]
> +static void aest_proc(struct aest_node_data *data)
> +{
> + struct ras_ext_regs *regs_p, regs = {0};
> + bool misc23_present;
> + bool fatal = false;
> + u64 errgsr = 0;
> + int i;
> +
> + /*
> + * Currently SR based handling is done through the architected
> + * discovery exposed through SRs. That may change in the future
> + * if there is supplemental information in the AEST that is
> + * needed.
> + */
> + if (data->interface.type == ACPI_AEST_NODE_SYSTEM_REGISTER) {
> + arch_arm_ras_report_error(data->interface.implemented,
> + data->interface.flags & AEST_INTERFACE_CLEAR_MISC);
> + return;
> + }
> +
> + regs_p = data->interface.regs;
> + errgsr = readq((void *) (((u64) regs_p) + ERRGSR_OFFSET));
> +
> + for (i = data->interface.start; i < data->interface.end; i++) {
> + if (!(data->interface.implemented & BIT(i)))
> + continue;
> +
> + if (!(data->interface.status_reporting & BIT(i)) && !(errgsr & BIT(i)))
> + continue;
> +
> + regs.err_status = readq(®s_p[i].err_status);
> + if (!(regs.err_status & ERR_STATUS_V))
> + continue;
> +
> + if (regs.err_status & ERR_STATUS_AV)
> + regs.err_addr = readq(®s_p[i].err_addr);
> +
> + regs.err_fr = readq(®s_p[i].err_fr);
> + regs.err_ctlr = readq(®s_p[i].err_ctlr);
> +
> + if (regs.err_status & ERR_STATUS_MV) {
> + misc23_present = aest_mmio_ras_misc23_present((u64) regs_p);
> + regs.err_misc0 = readq(®s_p[i].err_misc0);
> + regs.err_misc1 = readq(®s_p[i].err_misc1);
> +
> + if (misc23_present) {
> + regs.err_misc2 = readq(®s_p[i].err_misc2);
> + regs.err_misc3 = readq(®s_p[i].err_misc3);
> + }
> + }
> +
> + aest_print(data, regs, i, misc23_present);
> +
> + if (regs.err_status & ERR_STATUS_UE)
> + fatal = true;
> +
> + regs.err_status = arch_arm_ras_get_status_clear_value(regs.err_status);
> + writeq(regs.err_status, ®s_p[i].err_status);
> +
> + if (data->interface.flags & AEST_INTERFACE_CLEAR_MISC) {
> + writeq(0x0, ®s_p[i].err_misc0);
> + writeq(0x0, ®s_p[i].err_misc1);
> +
> + if (misc23_present) {
> + writeq(0x0, ®s_p[i].err_misc2);
> + writeq(0x0, ®s_p[i].err_misc3);
> + }
> + }
> + }
> +
> + if (fatal)
> + panic("AEST: uncorrectable error encountered");
Why don't we call panic() as soon as we realise an error is fatal?
Thanks,
Mark.
On Wed, Nov 24, 2021 at 06:09:14PM +0000, Marc Zyngier wrote:
> On Wed, 24 Nov 2021 17:07:07 +0000,
> Tyler Baicar <[email protected]> wrote:
> >
> > Add support for parsing the ARM Error Source Table and basic handling of
> > errors reported through both memory mapped and system register interfaces.
> >
> > Assume system register interfaces are only registered with private
> > peripheral interrupts (PPIs); otherwise there is no guarantee the
> > core handling the error is the core which took the error and has the
> > syndrome info in its system registers.
> >
> > Add logging for all detected errors and trigger a kernel panic if there is
> > any uncorrected error present.
> >
> > Signed-off-by: Tyler Baicar <[email protected]>
> > ---
> > MAINTAINERS | 1 +
> > arch/arm64/include/asm/ras.h | 52 ++++
> > arch/arm64/include/asm/sysreg.h | 2 +
> > arch/arm64/kernel/Makefile | 1 +
> > arch/arm64/kernel/ras.c | 125 +++++++++
> > arch/arm64/kvm/sys_regs.c | 2 +
> > drivers/acpi/arm64/Kconfig | 3 +
> > drivers/acpi/arm64/Makefile | 1 +
> > drivers/acpi/arm64/aest.c | 450 ++++++++++++++++++++++++++++++++
> > include/linux/acpi_aest.h | 50 ++++
> > include/linux/cpuhotplug.h | 1 +
> > 11 files changed, 688 insertions(+)
> > create mode 100644 arch/arm64/include/asm/ras.h
> > create mode 100644 arch/arm64/kernel/ras.c
> > create mode 100644 drivers/acpi/arm64/aest.c
> > create mode 100644 include/linux/acpi_aest.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5250298d2817..aa0483726606 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -382,6 +382,7 @@ ACPI FOR ARM64 (ACPI/arm64)
> > M: Lorenzo Pieralisi <[email protected]>
> > M: Hanjun Guo <[email protected]>
> > M: Sudeep Holla <[email protected]>
> > +R: Tyler Baicar <[email protected]>
> > L: [email protected]
> > L: [email protected] (moderated for non-subscribers)
> > S: Maintained
>
> Isn't this a bit premature? This isn't even mentioned in the commit
> message, only in passing in the cover letter.
>
Hi Marc,
This was something I encouraged Tyler to add during internal review,
both in response to the checkpatch.pl warning about adding new drivers
as well as our interest in reviewing any future changes to the aest
driver. Since refactoring is common, this level made sense to me - but
would it be preferable to add a new entry for just the new driver Tyler
added?
Thanks,
--
Darren Hart
Ampere Computing / OS and Kernel
Hi Darren,
On Mon, 29 Nov 2021 20:39:23 +0000,
Darren Hart <[email protected]> wrote:
>
> On Wed, Nov 24, 2021 at 06:09:14PM +0000, Marc Zyngier wrote:
> > On Wed, 24 Nov 2021 17:07:07 +0000,
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 5250298d2817..aa0483726606 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -382,6 +382,7 @@ ACPI FOR ARM64 (ACPI/arm64)
> > > M: Lorenzo Pieralisi <[email protected]>
> > > M: Hanjun Guo <[email protected]>
> > > M: Sudeep Holla <[email protected]>
> > > +R: Tyler Baicar <[email protected]>
> > > L: [email protected]
> > > L: [email protected] (moderated for non-subscribers)
> > > S: Maintained
> >
> > Isn't this a bit premature? This isn't even mentioned in the commit
> > message, only in passing in the cover letter.
> >
>
> Hi Marc,
>
> This was something I encouraged Tyler to add during internal review,
> both in response to the checkpatch.pl warning about adding new drivers
> as well as our interest in reviewing any future changes to the aest
> driver. Since refactoring is common, this level made sense to me - but
> would it be preferable to add a new entry for just the new driver Tyler
> added?
Adding someone as the co-maintainer/co-reviewer of a whole subsystem
(ACPI/arm64 in this case) comes, IMO, with a number of pre-requisites:
has the proposed co-{maintainer,reviewer} contributed and/or reviewed
a significant number of patches to that subsystem and/or actively
participated in the public discussions on the design and the
maintenance of the subsystem, so that their reviewing is authoritative
enough? I won't be judge of this, but it is definitely something to
consider.
I don't think preemptively adding someone to the MAINTAINERS entry to
indicate an interest in a whole subsystem is the right way to do it.
One could argue that this is what a mailing list is for! ;-) On the
other hand, an active participation to the review process is the
perfect way to engage with fellow developers and to grow a profile. It
is at this stage that adding oneself as an upstream reviewer makes a
lot of sense.
Alternatively, adding a MAINTAINERS entry for a specific driver is
definitely helpful and will certainly result in the listed maintainer
to be Cc'd on changes affecting it. But I would really like this
maintainer to actively engage with upstream, rather than simply be on
the receiving end of a stream of changes.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Tue, Nov 30, 2021 at 09:45:46AM +0000, Marc Zyngier wrote:
> Hi Darren,
>
> On Mon, 29 Nov 2021 20:39:23 +0000,
> Darren Hart <[email protected]> wrote:
> >
> > On Wed, Nov 24, 2021 at 06:09:14PM +0000, Marc Zyngier wrote:
> > > On Wed, 24 Nov 2021 17:07:07 +0000,
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 5250298d2817..aa0483726606 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -382,6 +382,7 @@ ACPI FOR ARM64 (ACPI/arm64)
> > > > M: Lorenzo Pieralisi <[email protected]>
> > > > M: Hanjun Guo <[email protected]>
> > > > M: Sudeep Holla <[email protected]>
> > > > +R: Tyler Baicar <[email protected]>
> > > > L: [email protected]
> > > > L: [email protected] (moderated for non-subscribers)
> > > > S: Maintained
> > >
> > > Isn't this a bit premature? This isn't even mentioned in the commit
> > > message, only in passing in the cover letter.
> > >
> >
> > Hi Marc,
> >
> > This was something I encouraged Tyler to add during internal review,
> > both in response to the checkpatch.pl warning about adding new drivers
> > as well as our interest in reviewing any future changes to the aest
> > driver. Since refactoring is common, this level made sense to me - but
> > would it be preferable to add a new entry for just the new driver Tyler
> > added?
>
> Adding someone as the co-maintainer/co-reviewer of a whole subsystem
> (ACPI/arm64 in this case) comes, IMO, with a number of pre-requisites:
> has the proposed co-{maintainer,reviewer} contributed and/or reviewed
> a significant number of patches to that subsystem and/or actively
> participated in the public discussions on the design and the
> maintenance of the subsystem, so that their reviewing is authoritative
> enough? I won't be judge of this, but it is definitely something to
> consider.
Hi Marc,
Agreed. I applied similar criteria when considering sub maintainers for
the platform/x86 subsystem while I maintained it.
> I don't think preemptively adding someone to the MAINTAINERS entry to
> indicate an interest in a whole subsystem is the right way to do it.
> One could argue that this is what a mailing list is for! ;-) On the
> other hand, an active participation to the review process is the
> perfect way to engage with fellow developers and to grow a profile. It
> is at this stage that adding oneself as an upstream reviewer makes a
> lot of sense.
Also generally agree. In this specific case, our interest was in the
driver itself, and we had to decide between the whole subsystem or
adding another F: entry in MAINTAINERS for the specific driver. Since
drivers/acpi/arm64 only has 3 .c files in it, adding another entry
seemed premature and overly granular. Certainly a subjective thing and
we have no objection to adding the extra line if that's preferred. This
should have been noted in the commit message.
> Alternatively, adding a MAINTAINERS entry for a specific driver is
> definitely helpful and will certainly result in the listed maintainer
> to be Cc'd on changes affecting it. But I would really like this
> maintainer to actively engage with upstream, rather than simply be on
> the receiving end of a stream of changes.
Agree for subsystems. For individual drivers, I think having the author
as a reviewer is appropriate and should result in more patch reviews,
which moves us in the direction of more community participation which we
all want to see.
Thanks,
--
Darren Hart
Ampere Computing / OS and Kernel
Hi, Tyler.
We would like to make a few comments.
> -----Original Message-----
> From: Tyler Baicar <[email protected]>
> Sent: Thursday, November 25, 2021 2:07 AM
> To: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Ishii, Shuuichirou/$B@P0f(B
> $B<~0lO:(B <[email protected]>; [email protected]
> Cc: Tyler Baicar <[email protected]>
> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>
> Add support for parsing the ARM Error Source Table and basic handling of
> errors reported through both memory mapped and system register interfaces.
>
> Assume system register interfaces are only registered with private
> peripheral interrupts (PPIs); otherwise there is no guarantee the
> core handling the error is the core which took the error and has the
> syndrome info in its system registers.
>
> Add logging for all detected errors and trigger a kernel panic if there is
> any uncorrected error present.
>
> Signed-off-by: Tyler Baicar <[email protected]>
> ---
[...]
> +static int __init aest_init_node(struct acpi_aest_hdr *node)
> +{
> + union acpi_aest_processor_data *proc_data;
> + union aest_node_spec *node_spec;
> + struct aest_node_data *data;
> + int ret;
> +
> + data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->node_type = node->type;
> +
> + node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
> node->node_specific_offset);
> +
> + switch (node->type) {
> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
> + memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_processor));
> + break;
> + case ACPI_AEST_MEMORY_ERROR_NODE:
> + memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_memory));
> + break;
> + case ACPI_AEST_SMMU_ERROR_NODE:
> + memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_smmu));
> + break;
> + case ACPI_AEST_VENDOR_ERROR_NODE:
> + memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_vendor));
> + break;
> + case ACPI_AEST_GIC_ERROR_NODE:
> + memcpy(&data->data, node_spec, sizeof(struct
> acpi_aest_gic));
> + break;
> + default:
> + kfree(data);
> + return -EINVAL;
> + }
> +
> + if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
> + proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data,
> node_spec,
> + sizeof(acpi_aest_processor));
> +
> + switch (data->data.processor.resource_type) {
> + case ACPI_AEST_CACHE_RESOURCE:
> + memcpy(&data->proc_data, proc_data,
> + sizeof(struct acpi_aest_processor_cache));
> + break;
> + case ACPI_AEST_TLB_RESOURCE:
> + memcpy(&data->proc_data, proc_data,
> + sizeof(struct acpi_aest_processor_tlb));
> + break;
> + case ACPI_AEST_GENERIC_RESOURCE:
> + memcpy(&data->proc_data, proc_data,
> + sizeof(struct acpi_aest_processor_generic));
> + break;
> + }
> + }
> +
> + ret = aest_init_interface(node, data);
> + if (ret) {
> + kfree(data);
> + return ret;
> + }
> +
> + return aest_init_interrupts(node, data);
If aest_init_interrupts() failed, is it necessary to release
the data pointer acquired by kzalloc?
> +}
> +
> +static void aest_count_ppi(struct acpi_aest_hdr *node)
> +{
> + struct acpi_aest_node_interrupt *interrupt;
> + int i;
> +
> + interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
> + node->node_interrupt_offset);
> +
> + for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
> + if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
> + num_ppi++;
> + }
> +}
> +
> +static int aest_starting_cpu(unsigned int cpu)
> +{
> + int i;
> +
> + for (i = 0; i < num_ppi; i++)
> + enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
> +
> + return 0;
> +}
> +
> +static int aest_dying_cpu(unsigned int cpu)
> +{
Wouldn't it be better to execute disable_percpu_irq(), which is paired
with enable_percpu_irq(), in aest_dying_cpu()?
> + return 0;
> +}
> +
[...]
Best regards,
Shuuichirou.
-Moved ACPI for ARM64 maintainers to "to:"
Hi Marc, Darren,
On 11/30/2021 11:41 AM, Darren Hart wrote:
> On Tue, Nov 30, 2021 at 09:45:46AM +0000, Marc Zyngier wrote:
>> Hi Darren,
>>
>> On Mon, 29 Nov 2021 20:39:23 +0000,
>> Darren Hart <[email protected]> wrote:
>>> On Wed, Nov 24, 2021 at 06:09:14PM +0000, Marc Zyngier wrote:
>>>> On Wed, 24 Nov 2021 17:07:07 +0000,
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 5250298d2817..aa0483726606 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -382,6 +382,7 @@ ACPI FOR ARM64 (ACPI/arm64)
>>>>> M: Lorenzo Pieralisi <[email protected]>
>>>>> M: Hanjun Guo <[email protected]>
>>>>> M: Sudeep Holla <[email protected]>
>>>>> +R: Tyler Baicar <[email protected]>
>>>>> L: [email protected]
>>>>> L: [email protected] (moderated for non-subscribers)
>>>>> S: Maintained
>>>> Isn't this a bit premature? This isn't even mentioned in the commit
>>>> message, only in passing in the cover letter.
>>>>
>>> Hi Marc,
>>>
>>> This was something I encouraged Tyler to add during internal review,
>>> both in response to the checkpatch.pl warning about adding new drivers
>>> as well as our interest in reviewing any future changes to the aest
>>> driver. Since refactoring is common, this level made sense to me - but
>>> would it be preferable to add a new entry for just the new driver Tyler
>>> added?
>> Adding someone as the co-maintainer/co-reviewer of a whole subsystem
>> (ACPI/arm64 in this case) comes, IMO, with a number of pre-requisites:
>> has the proposed co-{maintainer,reviewer} contributed and/or reviewed
>> a significant number of patches to that subsystem and/or actively
>> participated in the public discussions on the design and the
>> maintenance of the subsystem, so that their reviewing is authoritative
>> enough? I won't be judge of this, but it is definitely something to
>> consider.
> Hi Marc,
>
> Agreed. I applied similar criteria when considering sub maintainers for
> the platform/x86 subsystem while I maintained it.
>
>> I don't think preemptively adding someone to the MAINTAINERS entry to
>> indicate an interest in a whole subsystem is the right way to do it.
>> One could argue that this is what a mailing list is for! ;-) On the
>> other hand, an active participation to the review process is the
>> perfect way to engage with fellow developers and to grow a profile. It
>> is at this stage that adding oneself as an upstream reviewer makes a
>> lot of sense.
> Also generally agree. In this specific case, our interest was in the
> driver itself, and we had to decide between the whole subsystem or
> adding another F: entry in MAINTAINERS for the specific driver. Since
> drivers/acpi/arm64 only has 3 .c files in it, adding another entry
> seemed premature and overly granular. Certainly a subjective thing and
> we have no objection to adding the extra line if that's preferred. This
> should have been noted in the commit message.
Thank you for the feedback here, I will make sure to add this to the
commit message and cover letter in the next version.
Hi Lorenzo, Hanjun, Sudeep,
As for adding myself as a reviewer under ACPI for ARM64 or adding
another F: entry, do you have a preference or guidance on what I should
do here?
Thanks,
Tyler
>> Alternatively, adding a MAINTAINERS entry for a specific driver is
>> definitely helpful and will certainly result in the listed maintainer
>> to be Cc'd on changes affecting it. But I would really like this
>> maintainer to actively engage with upstream, rather than simply be on
>> the receiving end of a stream of changes.
> Agree for subsystems. For individual drivers, I think having the author
> as a reviewer is appropriate and should result in more patch reviews,
> which moves us in the direction of more community participation which we
> all want to see.
>
> Thanks,
Hi Mark,
Thank you for the initial feedback!
On 11/24/2021 1:51 PM, Mark Rutland wrote:
> Hi,
>
> I haven't looked at this in great detail, but I spotted a few issues
> from an initial scan.
>
> On Wed, Nov 24, 2021 at 12:07:07PM -0500, Tyler Baicar wrote:
>> Add support for parsing the ARM Error Source Table and basic handling of
>> errors reported through both memory mapped and system register interfaces.
>>
>> Assume system register interfaces are only registered with private
>> peripheral interrupts (PPIs); otherwise there is no guarantee the
>> core handling the error is the core which took the error and has the
>> syndrome info in its system registers.
> Can we actually assume that? What does the specification mandate?
The ARM Architecture Reference Manual Supplement RAS document
(https://developer.arm.com/documentation/ddi0587/latest) states in
section 3.9 the following:
"For an Arm Generic Interrupt Controller (GIC), if the error records of
the node that generates the interrupts are
only accessible via the System registers of one or more PEs, Arm
strongly recommends that the interrupt is a
Private Peripheral Interrupt (PPI) targeting that PE or one of those PEs."
>> Add logging for all detected errors and trigger a kernel panic if there is
>> any uncorrected error present.
> Has this been tested on any hardware or software platform?
Yes, I have tested this on Ampere Altra hardware. I've tested both the
PPI and SPI interrupt handling as well as system register and memory
mapped interface options.
>
> [...]
>
>> +#define ERRDEVARCH_REV_SHIFT 0x16
> IIUC This should be 16, not 0x16 (i.e. 22).
Yes, this should be 16. I'll fix that in the next version.
>> +#define ERRDEVARCH_REV_MASK 0xf
>> +
>> +#define RAS_REV_v1_1 0x1
>> +
>> +struct ras_ext_regs {
>> + u64 err_fr;
>> + u64 err_ctlr;
>> + u64 err_status;
>> + u64 err_addr;
>> + u64 err_misc0;
>> + u64 err_misc1;
>> + u64 err_misc2;
>> + u64 err_misc3;
>> +};
> These last four might be better an an array.
Will do.
> [...]
>
>> +static bool ras_extn_v1p1(void)
>> +{
>> + unsigned long fld, reg = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
>> +
>> + fld = cpuid_feature_extract_unsigned_field(reg, ID_AA64PFR0_RAS_SHIFT);
>> +
>> + return fld >= ID_AA64PFR0_RAS_V1P1;
>> +}
> I suspect it'd be better to pass this value around directly as
> `version`, rather than dropping this into a `misc23_present` temporary
> variable, as that would be a little clearer, and future-proof if/when
> more registers get added.
That's a good point. I'll update this in the next version.
> [...]
>
>> +void arch_arm_ras_report_error(u64 implemented, bool clear_misc)
>> +{
>> + struct ras_ext_regs regs = {0};
>> + unsigned int i, cpu_num;
>> + bool misc23_present;
>> + bool fatal = false;
>> + u64 num_records;
>> +
>> + if (!this_cpu_has_cap(ARM64_HAS_RAS_EXTN))
>> + return;
>> +
>> + cpu_num = get_cpu();
> Why get_cpu() here? Do you just need smp_processor_id()?
>
> The commit message explained that this would be PE-local (e.g. in a PPI
> handler), and we've already checked this_cpu_has_cap() which assumes
> we're not preemptible.
>
> So I don't see why we should use get_cpu() here -- any time it would
> have a difference implies something has already gone wrong.
Yes, will update.
>> + num_records = read_sysreg_s(SYS_ERRIDR_EL1) & ERRIDR_NUM_MASK;
>> +
>> + for (i = 0; i < num_records; i++) {
>> + if (!(implemented & BIT(i)))
>> + continue;
>> +
>> + write_sysreg_s(i, SYS_ERRSELR_EL1);
>> + isb();
>> + regs.err_status = read_sysreg_s(SYS_ERXSTATUS_EL1);
>> +
>> + if (!(regs.err_status & ERR_STATUS_V))
>> + continue;
>> +
>> + pr_err("error from processor 0x%x\n", cpu_num);
> Why in hex? We normally print 'cpu%d' or 'CPU%d', since this is a
> logical ID anyway.
I will update to use decimal print.
>> +
>> + if (regs.err_status & ERR_STATUS_AV)
>> + regs.err_addr = read_sysreg_s(SYS_ERXADDR_EL1);
>> +
>> + misc23_present = ras_extn_v1p1();
> As above, I reckon it's better to have this as 'version' or
> 'ras_version', and have the checks below be:
>
> if (version >= ID_AA64PFR0_RAS_V1P1) {
> // poke SYS_ERXMISC2_EL1
> // poke SYS_ERXMISC3_EL1
> }
Will do.
>> +
>> + if (regs.err_status & ERR_STATUS_MV) {
>> + regs.err_misc0 = read_sysreg_s(SYS_ERXMISC0_EL1);
>> + regs.err_misc1 = read_sysreg_s(SYS_ERXMISC1_EL1);
>> +
>> + if (misc23_present) {
>> + regs.err_misc2 = read_sysreg_s(SYS_ERXMISC2_EL1);
>> + regs.err_misc3 = read_sysreg_s(SYS_ERXMISC3_EL1);
>> + }
>> + }
>> +
>> + arch_arm_ras_print_error(®s, i, misc23_present);
>> +
>> + /*
>> + * In the future, we will treat UER conditions as potentially
>> + * recoverable.
>> + */
>> + if (regs.err_status & ERR_STATUS_UE)
>> + fatal = true;
>> +
>> + regs.err_status = arch_arm_ras_get_status_clear_value(regs.err_status);
>> + write_sysreg_s(regs.err_status, SYS_ERXSTATUS_EL1);
>> +
>> + if (clear_misc) {
>> + write_sysreg_s(0x0, SYS_ERXMISC0_EL1);
>> + write_sysreg_s(0x0, SYS_ERXMISC1_EL1);
>> +
>> + if (misc23_present) {
>> + write_sysreg_s(0x0, SYS_ERXMISC2_EL1);
>> + write_sysreg_s(0x0, SYS_ERXMISC3_EL1);
>> + }
>> + }
> Any reason not to clear when we read, above? e.g.
>
> #define READ_CLEAR_MISC(nr, clear) \
> ({ \
> unsigned long __val = read_sysreg_s(SYS_ERXMISC##nr##_EL1); \
> if (clear); \
> write_sysreg_s(0, SYS_ERXMISC##nr##_EL1); \
> __val; \
> })
>
> if (regs.err_status & ERR_STATUS_MV) {
> regs.err_misc0 = READ_CLEAR_MISC(0, clear_misc);
> regs.err_misc1 = READ_CLEAR_MISC(1, clear_misc);
>
> if (version >= ID_AA64PFR0_RAS_V1P1) {
> regs.err_misc2 = READ_CLEAR_MISC(2, clear_misc);
> regs.err_misc3 = READ_CLEAR_MISC(3, clear_misc);
> }
>
> }
>
> ... why does the clearing need to be conditional?
I like this proposal and will adopt it in the next version. The clearing
is conditional based on an option in the ACPI table. The misc registers
report mostly IMPDEF information which may or may not need to be cleared
after reporting depending on the hardware IP.
>> +
>> + isb();
>> + }
>> +
>> + if (fatal)
>> + panic("ARM RAS: uncorrectable error encountered");
>> +
>> + put_cpu();
>> +}
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index e3ec1a44f94d..dc15e9896db4 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1573,6 +1573,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>> { SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
>> { SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
>> { SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
>> + { SYS_DESC(SYS_ERXMISC2_EL1), trap_raz_wi },
>> + { SYS_DESC(SYS_ERXMISC3_EL1), trap_raz_wi },
> This should be a preparatory patch; this is preumably a latent bug in
> KVM.
I'll separate this into it's own patch. It's not just a bug in KVM,
these system registers are not defined at all today in the arm64
sysreg.h file (adding those is part of this patch as well).
> [...]
>
>> +static struct aest_node_data __percpu **ppi_data;
>> +static int ppi_irqs[AEST_MAX_PPI];
>> +static u8 num_ppi;
>> +static u8 ppi_idx;
> As above, do we have any guarantee these are actually PPIs?
Yes, aest_register_gsi() sets these up so that only PPIs are added to
the PPI list.
>> +static bool aest_mmio_ras_misc23_present(u64 base_addr)
>> +{
>> + u32 val;
>> +
>> + val = readl((void *) (base_addr + ERRDEVARCH_OFFSET));
>> + val <<= ERRDEVARCH_REV_SHIFT;
>> + val &= ERRDEVARCH_REV_MASK;
>> +
>> + return val >= RAS_REV_v1_1;
>> +}
> Is the shift the wrong way around?
>
> Above we have:
>
> #define ERRDEVARCH_REV_SHIFT 0x16
> #define ERRDEVARCH_REV_MASK 0xf
>
> #define RAS_REV_v1_1 0x1
>
> .. so this is:
>
> val <<= 0x16;
> val &= 0xf; // val[0x15:0] == 0, so this is 0
>
> return val >= 0x1; // false
>
> It'd be nicer to use FIELD_GET() here.
>
> As above, I also think it would be better to retrun the value of the
> field, and check that explciitly, for future proofing.
Yes, I can do that. When I tested this the IP I used did not have a
DEVARCH register which followed the spec, otherwise I would have caught
this.
>
> [...]
>
>> +static void aest_proc(struct aest_node_data *data)
>> +{
>> + struct ras_ext_regs *regs_p, regs = {0};
>> + bool misc23_present;
>> + bool fatal = false;
>> + u64 errgsr = 0;
>> + int i;
>> +
>> + /*
>> + * Currently SR based handling is done through the architected
>> + * discovery exposed through SRs. That may change in the future
>> + * if there is supplemental information in the AEST that is
>> + * needed.
>> + */
>> + if (data->interface.type == ACPI_AEST_NODE_SYSTEM_REGISTER) {
>> + arch_arm_ras_report_error(data->interface.implemented,
>> + data->interface.flags & AEST_INTERFACE_CLEAR_MISC);
>> + return;
>> + }
>> +
>> + regs_p = data->interface.regs;
>> + errgsr = readq((void *) (((u64) regs_p) + ERRGSR_OFFSET));
>> +
>> + for (i = data->interface.start; i < data->interface.end; i++) {
>> + if (!(data->interface.implemented & BIT(i)))
>> + continue;
>> +
>> + if (!(data->interface.status_reporting & BIT(i)) && !(errgsr & BIT(i)))
>> + continue;
>> +
>> + regs.err_status = readq(®s_p[i].err_status);
>> + if (!(regs.err_status & ERR_STATUS_V))
>> + continue;
>> +
>> + if (regs.err_status & ERR_STATUS_AV)
>> + regs.err_addr = readq(®s_p[i].err_addr);
>> +
>> + regs.err_fr = readq(®s_p[i].err_fr);
>> + regs.err_ctlr = readq(®s_p[i].err_ctlr);
>> +
>> + if (regs.err_status & ERR_STATUS_MV) {
>> + misc23_present = aest_mmio_ras_misc23_present((u64) regs_p);
>> + regs.err_misc0 = readq(®s_p[i].err_misc0);
>> + regs.err_misc1 = readq(®s_p[i].err_misc1);
>> +
>> + if (misc23_present) {
>> + regs.err_misc2 = readq(®s_p[i].err_misc2);
>> + regs.err_misc3 = readq(®s_p[i].err_misc3);
>> + }
>> + }
>> +
>> + aest_print(data, regs, i, misc23_present);
>> +
>> + if (regs.err_status & ERR_STATUS_UE)
>> + fatal = true;
>> +
>> + regs.err_status = arch_arm_ras_get_status_clear_value(regs.err_status);
>> + writeq(regs.err_status, ®s_p[i].err_status);
>> +
>> + if (data->interface.flags & AEST_INTERFACE_CLEAR_MISC) {
>> + writeq(0x0, ®s_p[i].err_misc0);
>> + writeq(0x0, ®s_p[i].err_misc1);
>> +
>> + if (misc23_present) {
>> + writeq(0x0, ®s_p[i].err_misc2);
>> + writeq(0x0, ®s_p[i].err_misc3);
>> + }
>> + }
>> + }
>> +
>> + if (fatal)
>> + panic("AEST: uncorrectable error encountered");
> Why don't we call panic() as soon as we realise an error is fatal?
Good point. We should panic at least before clearing the error. I think
the panic should happen immediately after the aest_print() call, do you
agree? We should still print the error before panicking (APEI/GHES
prints the full error prior to panicking as well).
Thanks,
Tyler
Hi Shuuichirou,
Thank you for your feedback!
On 12/9/2021 3:10 AM, [email protected] wrote:
> Hi, Tyler.
>
> We would like to make a few comments.
>
>> -----Original Message-----
>> From: Tyler Baicar <[email protected]>
>> Sent: Thursday, November 25, 2021 2:07 AM
>> To: [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; Ishii, Shuuichirou/石井
>> 周一郎 <[email protected]>; [email protected]
>> Cc: Tyler Baicar <[email protected]>
>> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>
>> Add support for parsing the ARM Error Source Table and basic handling of
>> errors reported through both memory mapped and system register interfaces.
>>
>> Assume system register interfaces are only registered with private
>> peripheral interrupts (PPIs); otherwise there is no guarantee the
>> core handling the error is the core which took the error and has the
>> syndrome info in its system registers.
>>
>> Add logging for all detected errors and trigger a kernel panic if there is
>> any uncorrected error present.
>>
>> Signed-off-by: Tyler Baicar <[email protected]>
>> ---
> [...]
>
>> +static int __init aest_init_node(struct acpi_aest_hdr *node)
>> +{
>> + union acpi_aest_processor_data *proc_data;
>> + union aest_node_spec *node_spec;
>> + struct aest_node_data *data;
>> + int ret;
>> +
>> + data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->node_type = node->type;
>> +
>> + node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
>> node->node_specific_offset);
>> +
>> + switch (node->type) {
>> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
>> + memcpy(&data->data, node_spec, sizeof(struct
>> acpi_aest_processor));
>> + break;
>> + case ACPI_AEST_MEMORY_ERROR_NODE:
>> + memcpy(&data->data, node_spec, sizeof(struct
>> acpi_aest_memory));
>> + break;
>> + case ACPI_AEST_SMMU_ERROR_NODE:
>> + memcpy(&data->data, node_spec, sizeof(struct
>> acpi_aest_smmu));
>> + break;
>> + case ACPI_AEST_VENDOR_ERROR_NODE:
>> + memcpy(&data->data, node_spec, sizeof(struct
>> acpi_aest_vendor));
>> + break;
>> + case ACPI_AEST_GIC_ERROR_NODE:
>> + memcpy(&data->data, node_spec, sizeof(struct
>> acpi_aest_gic));
>> + break;
>> + default:
>> + kfree(data);
>> + return -EINVAL;
>> + }
>> +
>> + if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
>> + proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data,
>> node_spec,
>> + sizeof(acpi_aest_processor));
>> +
>> + switch (data->data.processor.resource_type) {
>> + case ACPI_AEST_CACHE_RESOURCE:
>> + memcpy(&data->proc_data, proc_data,
>> + sizeof(struct acpi_aest_processor_cache));
>> + break;
>> + case ACPI_AEST_TLB_RESOURCE:
>> + memcpy(&data->proc_data, proc_data,
>> + sizeof(struct acpi_aest_processor_tlb));
>> + break;
>> + case ACPI_AEST_GENERIC_RESOURCE:
>> + memcpy(&data->proc_data, proc_data,
>> + sizeof(struct acpi_aest_processor_generic));
>> + break;
>> + }
>> + }
>> +
>> + ret = aest_init_interface(node, data);
>> + if (ret) {
>> + kfree(data);
>> + return ret;
>> + }
>> +
>> + return aest_init_interrupts(node, data);
> If aest_init_interrupts() failed, is it necessary to release
> the data pointer acquired by kzalloc?
aest_init_interrupts() returns an error if any of the interrupts in the
interrupt list fails, but it's possible that some interrupts in the list
registered successfully. So we attempt to keep chugging along in that
scenario because some interrupts may be enabled and registered with the
interface successfully. If any interrupt registration fails, there will
be a print notifying that there was a failure when initializing that node.
>> +}
>> +
>> +static void aest_count_ppi(struct acpi_aest_hdr *node)
>> +{
>> + struct acpi_aest_node_interrupt *interrupt;
>> + int i;
>> +
>> + interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
>> + node->node_interrupt_offset);
>> +
>> + for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
>> + if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
>> + num_ppi++;
>> + }
>> +}
>> +
>> +static int aest_starting_cpu(unsigned int cpu)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < num_ppi; i++)
>> + enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
>> +
>> + return 0;
>> +}
>> +
>> +static int aest_dying_cpu(unsigned int cpu)
>> +{
> Wouldn't it be better to execute disable_percpu_irq(), which is paired
> with enable_percpu_irq(), in aest_dying_cpu()?
Good point. I will add that in the next version.
Thanks,
Tyler
On Thu, Dec 16, 2021 at 05:05:15PM -0500, Tyler Baicar wrote:
> -Moved ACPI for ARM64 maintainers to "to:"
>
> Hi Marc, Darren,
>
> On 11/30/2021 11:41 AM, Darren Hart wrote:
> > On Tue, Nov 30, 2021 at 09:45:46AM +0000, Marc Zyngier wrote:
> > > Hi Darren,
> > >
> > > On Mon, 29 Nov 2021 20:39:23 +0000,
> > > Darren Hart <[email protected]> wrote:
> > > > On Wed, Nov 24, 2021 at 06:09:14PM +0000, Marc Zyngier wrote:
> > > > > On Wed, 24 Nov 2021 17:07:07 +0000,
> > > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > > index 5250298d2817..aa0483726606 100644
> > > > > > --- a/MAINTAINERS
> > > > > > +++ b/MAINTAINERS
> > > > > > @@ -382,6 +382,7 @@ ACPI FOR ARM64 (ACPI/arm64)
> > > > > > M: Lorenzo Pieralisi <[email protected]>
> > > > > > M: Hanjun Guo <[email protected]>
> > > > > > M: Sudeep Holla <[email protected]>
> > > > > > +R: Tyler Baicar <[email protected]>
> > > > > > L: [email protected]
> > > > > > L: [email protected] (moderated for non-subscribers)
> > > > > > S: Maintained
> > > > > Isn't this a bit premature? This isn't even mentioned in the commit
> > > > > message, only in passing in the cover letter.
> > > > >
> > > > Hi Marc,
> > > >
> > > > This was something I encouraged Tyler to add during internal review,
> > > > both in response to the checkpatch.pl warning about adding new drivers
> > > > as well as our interest in reviewing any future changes to the aest
> > > > driver. Since refactoring is common, this level made sense to me - but
> > > > would it be preferable to add a new entry for just the new driver Tyler
> > > > added?
> > > Adding someone as the co-maintainer/co-reviewer of a whole subsystem
> > > (ACPI/arm64 in this case) comes, IMO, with a number of pre-requisites:
> > > has the proposed co-{maintainer,reviewer} contributed and/or reviewed
> > > a significant number of patches to that subsystem and/or actively
> > > participated in the public discussions on the design and the
> > > maintenance of the subsystem, so that their reviewing is authoritative
> > > enough? I won't be judge of this, but it is definitely something to
> > > consider.
> > Hi Marc,
> >
> > Agreed. I applied similar criteria when considering sub maintainers for
> > the platform/x86 subsystem while I maintained it.
> >
> > > I don't think preemptively adding someone to the MAINTAINERS entry to
> > > indicate an interest in a whole subsystem is the right way to do it.
> > > One could argue that this is what a mailing list is for! ;-) On the
> > > other hand, an active participation to the review process is the
> > > perfect way to engage with fellow developers and to grow a profile. It
> > > is at this stage that adding oneself as an upstream reviewer makes a
> > > lot of sense.
> > Also generally agree. In this specific case, our interest was in the
> > driver itself, and we had to decide between the whole subsystem or
> > adding another F: entry in MAINTAINERS for the specific driver. Since
> > drivers/acpi/arm64 only has 3 .c files in it, adding another entry
> > seemed premature and overly granular. Certainly a subjective thing and
> > we have no objection to adding the extra line if that's preferred. This
> > should have been noted in the commit message.
>
> Thank you for the feedback here, I will make sure to add this to the commit
> message and cover letter in the next version.
Hi Marc,
Thanks for responding and providing all the necessary details.
>
> Hi Lorenzo, Hanjun, Sudeep,
>
> As for adding myself as a reviewer under ACPI for ARM64 or adding another F:
> entry, do you have a preference or guidance on what I should do here?
>
I prefer to start with an entry specific to the $subject driver for all
the reasons Marc has already stated. It may also add confusion and provide
misleading reference to others who want to maintain specific drivers like
this in the future. Further it will result in this list to grow even though
not all in that will be interested in reviewing or maintaining ARM64
ACPI subsystem if we take the approach in this patch and more confusion
to the developers.
Ofcourse if you are interested and get engaged in the review of ARM64
ACPI in the future we can always revisit and update accordingly.
Hope this helps and provides clarification you are looking for.
--
Regards,
Sudeep
Hi, Tyler.
When do you plan to post the v2 patch series?
Please let me know if you don't mind.
Best regards.
> -----Original Message-----
> From: Tyler Baicar <[email protected]>
> Sent: Friday, December 17, 2021 8:33 AM
> To: Ishii, Shuuichirou/石井 周一郎 <[email protected]>; 'Tyler Baicar'
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>
> Hi Shuuichirou,
>
> Thank you for your feedback!
>
> On 12/9/2021 3:10 AM, [email protected] wrote:
> > Hi, Tyler.
> >
> > We would like to make a few comments.
> >
> >> -----Original Message-----
> >> From: Tyler Baicar <[email protected]>
> >> Sent: Thursday, November 25, 2021 2:07 AM
> >> To: [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; Ishii, Shuuichirou/石井
> >> 周一郎 <[email protected]>; [email protected]
> >> Cc: Tyler Baicar <[email protected]>
> >> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
> >>
> >> Add support for parsing the ARM Error Source Table and basic handling
> >> of errors reported through both memory mapped and system register
> interfaces.
> >>
> >> Assume system register interfaces are only registered with private
> >> peripheral interrupts (PPIs); otherwise there is no guarantee the
> >> core handling the error is the core which took the error and has the
> >> syndrome info in its system registers.
> >>
> >> Add logging for all detected errors and trigger a kernel panic if
> >> there is any uncorrected error present.
> >>
> >> Signed-off-by: Tyler Baicar <[email protected]>
> >> ---
> > [...]
> >
> >> +static int __init aest_init_node(struct acpi_aest_hdr *node) {
> >> + union acpi_aest_processor_data *proc_data;
> >> + union aest_node_spec *node_spec;
> >> + struct aest_node_data *data;
> >> + int ret;
> >> +
> >> + data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
> >> + if (!data)
> >> + return -ENOMEM;
> >> +
> >> + data->node_type = node->type;
> >> +
> >> + node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
> >> node->node_specific_offset);
> >> +
> >> + switch (node->type) {
> >> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
> >> + memcpy(&data->data, node_spec, sizeof(struct
> >> acpi_aest_processor));
> >> + break;
> >> + case ACPI_AEST_MEMORY_ERROR_NODE:
> >> + memcpy(&data->data, node_spec, sizeof(struct
> >> acpi_aest_memory));
> >> + break;
> >> + case ACPI_AEST_SMMU_ERROR_NODE:
> >> + memcpy(&data->data, node_spec, sizeof(struct
> >> acpi_aest_smmu));
> >> + break;
> >> + case ACPI_AEST_VENDOR_ERROR_NODE:
> >> + memcpy(&data->data, node_spec, sizeof(struct
> >> acpi_aest_vendor));
> >> + break;
> >> + case ACPI_AEST_GIC_ERROR_NODE:
> >> + memcpy(&data->data, node_spec, sizeof(struct
> >> acpi_aest_gic));
> >> + break;
> >> + default:
> >> + kfree(data);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
> >> + proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data,
> >> node_spec,
> >> + sizeof(acpi_aest_processor));
> >> +
> >> + switch (data->data.processor.resource_type) {
> >> + case ACPI_AEST_CACHE_RESOURCE:
> >> + memcpy(&data->proc_data, proc_data,
> >> + sizeof(struct acpi_aest_processor_cache));
> >> + break;
> >> + case ACPI_AEST_TLB_RESOURCE:
> >> + memcpy(&data->proc_data, proc_data,
> >> + sizeof(struct acpi_aest_processor_tlb));
> >> + break;
> >> + case ACPI_AEST_GENERIC_RESOURCE:
> >> + memcpy(&data->proc_data, proc_data,
> >> + sizeof(struct acpi_aest_processor_generic));
> >> + break;
> >> + }
> >> + }
> >> +
> >> + ret = aest_init_interface(node, data);
> >> + if (ret) {
> >> + kfree(data);
> >> + return ret;
> >> + }
> >> +
> >> + return aest_init_interrupts(node, data);
> > If aest_init_interrupts() failed, is it necessary to release the data
> > pointer acquired by kzalloc?
> aest_init_interrupts() returns an error if any of the interrupts in the interrupt list
> fails, but it's possible that some interrupts in the list registered successfully. So
> we attempt to keep chugging along in that scenario because some interrupts may
> be enabled and registered with the interface successfully. If any interrupt
> registration fails, there will be a print notifying that there was a failure when
> initializing that node.
> >> +}
> >> +
> >> +static void aest_count_ppi(struct acpi_aest_hdr *node)
> >> +{
> >> + struct acpi_aest_node_interrupt *interrupt;
> >> + int i;
> >> +
> >> + interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
> >> + node->node_interrupt_offset);
> >> +
> >> + for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
> >> + if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
> >> + num_ppi++;
> >> + }
> >> +}
> >> +
> >> +static int aest_starting_cpu(unsigned int cpu)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < num_ppi; i++)
> >> + enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int aest_dying_cpu(unsigned int cpu)
> >> +{
> > Wouldn't it be better to execute disable_percpu_irq(), which is paired
> > with enable_percpu_irq(), in aest_dying_cpu()?
>
> Good point. I will add that in the next version.
>
> Thanks,
>
> Tyler
Hi Shuuichirou,
I should be able to get a v2 patch series out by the end of the month.
Thanks,
Tyler
On 4/20/2022 3:54 AM, [email protected] wrote:
> Hi, Tyler.
>
> When do you plan to post the v2 patch series?
> Please let me know if you don't mind.
>
> Best regards.
>
>> -----Original Message-----
>> From: Tyler Baicar <[email protected]>
>> Sent: Friday, December 17, 2021 8:33 AM
>> To: Ishii, Shuuichirou/石井 周一郎 <[email protected]>; 'Tyler Baicar'
>> <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>
>> Hi Shuuichirou,
>>
>> Thank you for your feedback!
>>
>> On 12/9/2021 3:10 AM, [email protected] wrote:
>>> Hi, Tyler.
>>>
>>> We would like to make a few comments.
>>>
>>>> -----Original Message-----
>>>> From: Tyler Baicar <[email protected]>
>>>> Sent: Thursday, November 25, 2021 2:07 AM
>>>> To: [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected]; [email protected];
>>>> [email protected]; [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected]; [email protected];
>>>> [email protected]; [email protected]; [email protected];
>>>> [email protected]; [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; Ishii, Shuuichirou/石井
>>>> 周一郎 <[email protected]>; [email protected]
>>>> Cc: Tyler Baicar <[email protected]>
>>>> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>>>
>>>> Add support for parsing the ARM Error Source Table and basic handling
>>>> of errors reported through both memory mapped and system register
>> interfaces.
>>>>
>>>> Assume system register interfaces are only registered with private
>>>> peripheral interrupts (PPIs); otherwise there is no guarantee the
>>>> core handling the error is the core which took the error and has the
>>>> syndrome info in its system registers.
>>>>
>>>> Add logging for all detected errors and trigger a kernel panic if
>>>> there is any uncorrected error present.
>>>>
>>>> Signed-off-by: Tyler Baicar <[email protected]>
>>>> ---
>>> [...]
>>>
>>>> +static int __init aest_init_node(struct acpi_aest_hdr *node) {
>>>> + union acpi_aest_processor_data *proc_data;
>>>> + union aest_node_spec *node_spec;
>>>> + struct aest_node_data *data;
>>>> + int ret;
>>>> +
>>>> + data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
>>>> + if (!data)
>>>> + return -ENOMEM;
>>>> +
>>>> + data->node_type = node->type;
>>>> +
>>>> + node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
>>>> node->node_specific_offset);
>>>> +
>>>> + switch (node->type) {
>>>> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_processor));
>>>> + break;
>>>> + case ACPI_AEST_MEMORY_ERROR_NODE:
>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_memory));
>>>> + break;
>>>> + case ACPI_AEST_SMMU_ERROR_NODE:
>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_smmu));
>>>> + break;
>>>> + case ACPI_AEST_VENDOR_ERROR_NODE:
>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_vendor));
>>>> + break;
>>>> + case ACPI_AEST_GIC_ERROR_NODE:
>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_gic));
>>>> + break;
>>>> + default:
>>>> + kfree(data);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
>>>> + proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data,
>>>> node_spec,
>>>> + sizeof(acpi_aest_processor));
>>>> +
>>>> + switch (data->data.processor.resource_type) {
>>>> + case ACPI_AEST_CACHE_RESOURCE:
>>>> + memcpy(&data->proc_data, proc_data,
>>>> + sizeof(struct acpi_aest_processor_cache));
>>>> + break;
>>>> + case ACPI_AEST_TLB_RESOURCE:
>>>> + memcpy(&data->proc_data, proc_data,
>>>> + sizeof(struct acpi_aest_processor_tlb));
>>>> + break;
>>>> + case ACPI_AEST_GENERIC_RESOURCE:
>>>> + memcpy(&data->proc_data, proc_data,
>>>> + sizeof(struct acpi_aest_processor_generic));
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + ret = aest_init_interface(node, data);
>>>> + if (ret) {
>>>> + kfree(data);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return aest_init_interrupts(node, data);
>>> If aest_init_interrupts() failed, is it necessary to release the data
>>> pointer acquired by kzalloc?
>> aest_init_interrupts() returns an error if any of the interrupts in the interrupt list
>> fails, but it's possible that some interrupts in the list registered successfully. So
>> we attempt to keep chugging along in that scenario because some interrupts may
>> be enabled and registered with the interface successfully. If any interrupt
>> registration fails, there will be a print notifying that there was a failure when
>> initializing that node.
>>>> +}
>>>> +
>>>> +static void aest_count_ppi(struct acpi_aest_hdr *node)
>>>> +{
>>>> + struct acpi_aest_node_interrupt *interrupt;
>>>> + int i;
>>>> +
>>>> + interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
>>>> + node->node_interrupt_offset);
>>>> +
>>>> + for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
>>>> + if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
>>>> + num_ppi++;
>>>> + }
>>>> +}
>>>> +
>>>> +static int aest_starting_cpu(unsigned int cpu)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < num_ppi; i++)
>>>> + enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int aest_dying_cpu(unsigned int cpu)
>>>> +{
>>> Wouldn't it be better to execute disable_percpu_irq(), which is paired
>>> with enable_percpu_irq(), in aest_dying_cpu()?
>>
>> Good point. I will add that in the next version.
>>
>> Thanks,
>>
>> Tyler
>
Hi, Tyler
Thank you for your reply.
After the v2 patch series is posted,
we would like to review the source locations we noted.
Best regards,
Shuuichirou.
> -----Original Message-----
> From: Tyler Baicar <[email protected]>
> Sent: Monday, May 9, 2022 10:38 PM
> To: Ishii, Shuuichirou/石井 周一郎 <[email protected]>; 'Tyler Baicar'
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>
> Hi Shuuichirou,
>
> I should be able to get a v2 patch series out by the end of the month.
>
> Thanks,
> Tyler
>
> On 4/20/2022 3:54 AM, [email protected] wrote:
> > Hi, Tyler.
> >
> > When do you plan to post the v2 patch series?
> > Please let me know if you don't mind.
> >
> > Best regards.
> >
> >> -----Original Message-----
> >> From: Tyler Baicar <[email protected]>
> >> Sent: Friday, December 17, 2021 8:33 AM
> >> To: Ishii, Shuuichirou/石井 周一郎 <[email protected]>; 'Tyler
> Baicar'
> >> <[email protected]>; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]
> >> Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
> >>
> >> Hi Shuuichirou,
> >>
> >> Thank you for your feedback!
> >>
> >> On 12/9/2021 3:10 AM, [email protected] wrote:
> >>> Hi, Tyler.
> >>>
> >>> We would like to make a few comments.
> >>>
> >>>> -----Original Message-----
> >>>> From: Tyler Baicar <[email protected]>
> >>>> Sent: Thursday, November 25, 2021 2:07 AM
> >>>> To: [email protected];
> [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected]; [email protected];
> >>>> [email protected]; [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected]; [email protected];
> >>>> [email protected]; [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; Ishii, Shuuichirou/石井
> >>>> 周一郎 <[email protected]>; [email protected]
> >>>> Cc: Tyler Baicar <[email protected]>
> >>>> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
> >>>>
> >>>> Add support for parsing the ARM Error Source Table and basic
> >>>> handling of errors reported through both memory mapped and system
> >>>> register
> >> interfaces.
> >>>>
> >>>> Assume system register interfaces are only registered with private
> >>>> peripheral interrupts (PPIs); otherwise there is no guarantee the
> >>>> core handling the error is the core which took the error and has
> >>>> the syndrome info in its system registers.
> >>>>
> >>>> Add logging for all detected errors and trigger a kernel panic if
> >>>> there is any uncorrected error present.
> >>>>
> >>>> Signed-off-by: Tyler Baicar <[email protected]>
> >>>> ---
> >>> [...]
> >>>
> >>>> +static int __init aest_init_node(struct acpi_aest_hdr *node) {
> >>>> + union acpi_aest_processor_data *proc_data;
> >>>> + union aest_node_spec *node_spec;
> >>>> + struct aest_node_data *data;
> >>>> + int ret;
> >>>> +
> >>>> + data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
> >>>> + if (!data)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + data->node_type = node->type;
> >>>> +
> >>>> + node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
> >>>> node->node_specific_offset);
> >>>> +
> >>>> + switch (node->type) {
> >>>> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
> >>>> + memcpy(&data->data, node_spec, sizeof(struct
> >>>> acpi_aest_processor));
> >>>> + break;
> >>>> + case ACPI_AEST_MEMORY_ERROR_NODE:
> >>>> + memcpy(&data->data, node_spec, sizeof(struct
> >>>> acpi_aest_memory));
> >>>> + break;
> >>>> + case ACPI_AEST_SMMU_ERROR_NODE:
> >>>> + memcpy(&data->data, node_spec, sizeof(struct
> >>>> acpi_aest_smmu));
> >>>> + break;
> >>>> + case ACPI_AEST_VENDOR_ERROR_NODE:
> >>>> + memcpy(&data->data, node_spec, sizeof(struct
> >>>> acpi_aest_vendor));
> >>>> + break;
> >>>> + case ACPI_AEST_GIC_ERROR_NODE:
> >>>> + memcpy(&data->data, node_spec, sizeof(struct
> >>>> acpi_aest_gic));
> >>>> + break;
> >>>> + default:
> >>>> + kfree(data);
> >>>> + return -EINVAL;
> >>>> + }
> >>>> +
> >>>> + if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
> >>>> + proc_data = ACPI_ADD_PTR(union
> acpi_aest_processor_data,
> >>>> node_spec,
> >>>> +
> sizeof(acpi_aest_processor));
> >>>> +
> >>>> + switch (data->data.processor.resource_type) {
> >>>> + case ACPI_AEST_CACHE_RESOURCE:
> >>>> + memcpy(&data->proc_data, proc_data,
> >>>> + sizeof(struct
> acpi_aest_processor_cache));
> >>>> + break;
> >>>> + case ACPI_AEST_TLB_RESOURCE:
> >>>> + memcpy(&data->proc_data, proc_data,
> >>>> + sizeof(struct
> acpi_aest_processor_tlb));
> >>>> + break;
> >>>> + case ACPI_AEST_GENERIC_RESOURCE:
> >>>> + memcpy(&data->proc_data, proc_data,
> >>>> + sizeof(struct
> acpi_aest_processor_generic));
> >>>> + break;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + ret = aest_init_interface(node, data);
> >>>> + if (ret) {
> >>>> + kfree(data);
> >>>> + return ret;
> >>>> + }
> >>>> +
> >>>> + return aest_init_interrupts(node, data);
> >>> If aest_init_interrupts() failed, is it necessary to release the
> >>> data pointer acquired by kzalloc?
> >> aest_init_interrupts() returns an error if any of the interrupts in
> >> the interrupt list fails, but it's possible that some interrupts in
> >> the list registered successfully. So we attempt to keep chugging
> >> along in that scenario because some interrupts may be enabled and
> >> registered with the interface successfully. If any interrupt
> >> registration fails, there will be a print notifying that there was a failure when
> initializing that node.
> >>>> +}
> >>>> +
> >>>> +static void aest_count_ppi(struct acpi_aest_hdr *node) {
> >>>> + struct acpi_aest_node_interrupt *interrupt;
> >>>> + int i;
> >>>> +
> >>>> + interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt,
> node,
> >>>> + node->node_interrupt_offset);
> >>>> +
> >>>> + for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
> >>>> + if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
> >>>> + num_ppi++;
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +static int aest_starting_cpu(unsigned int cpu) {
> >>>> + int i;
> >>>> +
> >>>> + for (i = 0; i < num_ppi; i++)
> >>>> + enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int aest_dying_cpu(unsigned int cpu) {
> >>> Wouldn't it be better to execute disable_percpu_irq(), which is
> >>> paired with enable_percpu_irq(), in aest_dying_cpu()?
> >>
> >> Good point. I will add that in the next version.
> >>
> >> Thanks,
> >>
> >> Tyler
> >
Hi, Tyler.
I am very interested in your work about AEST.
When do you plan to update the v2 patch series?
Best regards.
在 2022/5/9 21:37, Tyler Baicar 写道:
> Hi Shuuichirou,
>
> I should be able to get a v2 patch series out by the end of the month.
>
> Thanks,
> Tyler
>
> On 4/20/2022 3:54 AM, [email protected] wrote:
>> Hi, Tyler.
>>
>> When do you plan to post the v2 patch series?
>> Please let me know if you don't mind.
>>
>> Best regards.
>>
>>> -----Original Message-----
>>> From: Tyler Baicar <[email protected]>
>>> Sent: Friday, December 17, 2021 8:33 AM
>>> To: Ishii, Shuuichirou/石井 周一郎 <[email protected]>; 'Tyler
>>> Baicar'
>>> <[email protected]>; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]
>>> Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>>
>>> Hi Shuuichirou,
>>>
>>> Thank you for your feedback!
>>>
>>> On 12/9/2021 3:10 AM, [email protected] wrote:
>>>> Hi, Tyler.
>>>>
>>>> We would like to make a few comments.
>>>>
>>>>> -----Original Message-----
>>>>> From: Tyler Baicar <[email protected]>
>>>>> Sent: Thursday, November 25, 2021 2:07 AM
>>>>> To: [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected]; [email protected];
>>>>> [email protected]; [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected]; [email protected];
>>>>> [email protected]; [email protected]; [email protected];
>>>>> [email protected]; [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; Ishii, Shuuichirou/石井
>>>>> 周一郎 <[email protected]>; [email protected]
>>>>> Cc: Tyler Baicar <[email protected]>
>>>>> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>>>>
>>>>> Add support for parsing the ARM Error Source Table and basic handling
>>>>> of errors reported through both memory mapped and system register
>>> interfaces.
>>>>>
>>>>> Assume system register interfaces are only registered with private
>>>>> peripheral interrupts (PPIs); otherwise there is no guarantee the
>>>>> core handling the error is the core which took the error and has the
>>>>> syndrome info in its system registers.
>>>>>
>>>>> Add logging for all detected errors and trigger a kernel panic if
>>>>> there is any uncorrected error present.
>>>>>
>>>>> Signed-off-by: Tyler Baicar <[email protected]>
>>>>> ---
>>>> [...]
>>>>
>>>>> +static int __init aest_init_node(struct acpi_aest_hdr *node) {
>>>>> + union acpi_aest_processor_data *proc_data;
>>>>> + union aest_node_spec *node_spec;
>>>>> + struct aest_node_data *data;
>>>>> + int ret;
>>>>> +
>>>>> + data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
>>>>> + if (!data)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + data->node_type = node->type;
>>>>> +
>>>>> + node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
>>>>> node->node_specific_offset);
>>>>> +
>>>>> + switch (node->type) {
>>>>> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
>>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_processor));
>>>>> + break;
>>>>> + case ACPI_AEST_MEMORY_ERROR_NODE:
>>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_memory));
>>>>> + break;
>>>>> + case ACPI_AEST_SMMU_ERROR_NODE:
>>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_smmu));
>>>>> + break;
>>>>> + case ACPI_AEST_VENDOR_ERROR_NODE:
>>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_vendor));
>>>>> + break;
>>>>> + case ACPI_AEST_GIC_ERROR_NODE:
>>>>> + memcpy(&data->data, node_spec, sizeof(struct
>>>>> acpi_aest_gic));
>>>>> + break;
>>>>> + default:
>>>>> + kfree(data);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
>>>>> + proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data,
>>>>> node_spec,
>>>>> + sizeof(acpi_aest_processor));
>>>>> +
>>>>> + switch (data->data.processor.resource_type) {
>>>>> + case ACPI_AEST_CACHE_RESOURCE:
>>>>> + memcpy(&data->proc_data, proc_data,
>>>>> + sizeof(struct acpi_aest_processor_cache));
>>>>> + break;
>>>>> + case ACPI_AEST_TLB_RESOURCE:
>>>>> + memcpy(&data->proc_data, proc_data,
>>>>> + sizeof(struct acpi_aest_processor_tlb));
>>>>> + break;
>>>>> + case ACPI_AEST_GENERIC_RESOURCE:
>>>>> + memcpy(&data->proc_data, proc_data,
>>>>> + sizeof(struct acpi_aest_processor_generic));
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + ret = aest_init_interface(node, data);
>>>>> + if (ret) {
>>>>> + kfree(data);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + return aest_init_interrupts(node, data);
>>>> If aest_init_interrupts() failed, is it necessary to release the data
>>>> pointer acquired by kzalloc?
>>> aest_init_interrupts() returns an error if any of the interrupts in
>>> the interrupt list
>>> fails, but it's possible that some interrupts in the list registered
>>> successfully. So
>>> we attempt to keep chugging along in that scenario because some
>>> interrupts may
>>> be enabled and registered with the interface successfully. If any
>>> interrupt
>>> registration fails, there will be a print notifying that there was a
>>> failure when
>>> initializing that node.
>>>>> +}
>>>>> +
>>>>> +static void aest_count_ppi(struct acpi_aest_hdr *node)
>>>>> +{
>>>>> + struct acpi_aest_node_interrupt *interrupt;
>>>>> + int i;
>>>>> +
>>>>> + interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
>>>>> + node->node_interrupt_offset);
>>>>> +
>>>>> + for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
>>>>> + if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
>>>>> + num_ppi++;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static int aest_starting_cpu(unsigned int cpu)
>>>>> +{
>>>>> + int i;
>>>>> +
>>>>> + for (i = 0; i < num_ppi; i++)
>>>>> + enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int aest_dying_cpu(unsigned int cpu)
>>>>> +{
>>>> Wouldn't it be better to execute disable_percpu_irq(), which is paired
>>>> with enable_percpu_irq(), in aest_dying_cpu()?
>>>
>>> Good point. I will add that in the next version.
>>>
>>> Thanks,
>>>
>>> Tyler
>>
> _______________________________________________
> kvmarm mailing list
> [email protected]
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm