From: Mark Rutland <mark.rutland@arm.com>
To: Tyler Baicar <baicar@os.amperecomputing.com>
Cc: patches@amperecomputing.com, abdulhamid@os.amperecomputing.com,
darren@os.amperecomputing.com, catalin.marinas@arm.com,
will@kernel.org, maz@kernel.org, james.morse@arm.com,
alexandru.elisei@arm.com, suzuki.poulose@arm.com,
lorenzo.pieralisi@arm.com, guohanjun@huawei.com,
sudeep.holla@arm.com, rafael@kernel.org, lenb@kernel.org,
tony.luck@intel.com, bp@alien8.de, anshuman.khandual@arm.com,
vincenzo.frascino@arm.com, tabba@google.com, marcan@marcan.st,
keescook@chromium.org, jthierry@redhat.com, masahiroy@kernel.org,
samitolvanen@google.com, john.garry@huawei.com,
daniel.lezcano@linaro.org, gor@linux.ibm.com,
zhangshaokun@hisilicon.com, tmricht@linux.ibm.com,
dchinner@redhat.com, tglx@linutronix.de,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
kvmarm@lists.cs.columbia.edu, linux-acpi@vger.kernel.org,
linux-edac@vger.kernel.org, ishii.shuuichir@fujitsu.com,
Vineeth.Pillai@microsoft.com
Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
Date: Wed, 24 Nov 2021 18:51:23 +0000 [thread overview]
Message-ID: <YZ6Jq39Z9dIUqcfO@lakrids> (raw)
In-Reply-To: <20211124170708.3874-2-baicar@os.amperecomputing.com>
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.
next prev parent reply other threads:[~2021-11-24 18:51 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-24 17:07 [PATCH 0/2] ARM Error Source Table Support Tyler Baicar
2021-11-24 17:07 ` [PATCH 1/2] ACPI/AEST: Initial AEST driver Tyler Baicar
2021-11-24 18:09 ` Marc Zyngier
2021-11-29 20:39 ` Darren Hart
2021-11-30 9:45 ` Marc Zyngier
2021-11-30 16:41 ` Darren Hart
2021-12-16 22:05 ` Tyler Baicar
2021-12-16 23:42 ` Sudeep Holla
2021-11-24 18:51 ` Mark Rutland [this message]
2021-12-16 23:22 ` Tyler Baicar
2021-12-09 8:10 ` ishii.shuuichir
2021-12-16 23:33 ` Tyler Baicar
2022-04-20 7:54 ` ishii.shuuichir
2022-05-09 13:37 ` Tyler Baicar
2022-05-09 23:23 ` ishii.shuuichir
2022-12-07 5:44 ` Ruidong Tian
2021-11-24 17:07 ` [PATCH 2/2] trace, ras: add ARM RAS extension trace event Tyler Baicar
-- strict thread matches above, loose matches on Subject: below --
2024-03-04 11:15 [PATCH 0/2] ARM Error Source Table V1 Support Ruidong Tian
2024-03-04 11:15 ` [PATCH 1/2] ACPI/AEST: Initial AEST driver Ruidong Tian
2024-03-04 12:07 ` Marc Zyngier
2024-03-08 4:49 ` Ruidong Tian
[not found] ` <aaad88c3-333d-4714-a9ca-3b66c8a5d9c8@linux.alibaba.com>
2024-03-09 10:33 ` Marc Zyngier
2024-03-12 9:53 ` Ruidong Tian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YZ6Jq39Z9dIUqcfO@lakrids \
--to=mark.rutland@arm.com \
--cc=Vineeth.Pillai@microsoft.com \
--cc=abdulhamid@os.amperecomputing.com \
--cc=alexandru.elisei@arm.com \
--cc=anshuman.khandual@arm.com \
--cc=baicar@os.amperecomputing.com \
--cc=bp@alien8.de \
--cc=catalin.marinas@arm.com \
--cc=daniel.lezcano@linaro.org \
--cc=darren@os.amperecomputing.com \
--cc=dchinner@redhat.com \
--cc=gor@linux.ibm.com \
--cc=guohanjun@huawei.com \
--cc=ishii.shuuichir@fujitsu.com \
--cc=james.morse@arm.com \
--cc=john.garry@huawei.com \
--cc=jthierry@redhat.com \
--cc=keescook@chromium.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=marcan@marcan.st \
--cc=masahiroy@kernel.org \
--cc=maz@kernel.org \
--cc=patches@amperecomputing.com \
--cc=rafael@kernel.org \
--cc=samitolvanen@google.com \
--cc=sudeep.holla@arm.com \
--cc=suzuki.poulose@arm.com \
--cc=tabba@google.com \
--cc=tglx@linutronix.de \
--cc=tmricht@linux.ibm.com \
--cc=tony.luck@intel.com \
--cc=vincenzo.frascino@arm.com \
--cc=will@kernel.org \
--cc=zhangshaokun@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).