On Fri, 08 Mar 2024 03:43:30 +0000,
Ruidong Tian <tianruidong@xxxxxxxxxxxxxxxxx> wrote:
在 2024/3/4 20:07, Marc Zyngier 写道:
On Mon, 04 Mar 2024 11:15:16 +0000,
Ruidong Tian<tianruidong@xxxxxxxxxxxxxxxxx> wrote:
diff --git a/arch/arm64/include/asm/ras.h b/arch/arm64/include/asm/ras.hAll these bits need to be defined in arch/arm64/tools/sysreg as
new file mode 100644
index 000000000000..2fb0d9741567
--- /dev/null
+++ b/arch/arm64/include/asm/ras.h
@@ -0,0 +1,38 @@
+/* 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_ER BIT(28)
+#define ERR_STATUS_OF BIT(27)
+#define ERR_STATUS_MV BIT(26)
+#define ERR_STATUS_CE (BIT(25) | BIT(24))
+#define ERR_STATUS_DE BIT(23)
+#define ERR_STATUS_PN BIT(22)
+#define ERR_STATUS_UET (BIT(21) | BIT(20))
+#define ERR_STATUS_CI BIT(19)
+#define ERR_STATUS_IERR GENMASK_ULL(15, 8)
+#define ERR_STATUS_SERR GENMASK_ULL(7, 0)
ERXSTATUS_EL1 fields.
This file only describes the system register, but RAS MMIO registers
use these bits too. Would it be appropriate to define them in
arch/arm64/tools/sysreg?
You are using them for system registers, they need to be defined
there. The fact that they are also used to MMIO is anecdotal.
[...]
+#define CASE_READ_CLEAR(x, clear) \Please don't use macros with side effects. This is horrible to debug.
+ case (x): { \
+ res = read_sysreg_s(SYS_##x##_EL1); \
+ if (clear) \
+ write_sysreg_s(0, SYS_##x##_EL1); \
+ break; \
+ }
Instead, *return* the value from the macro, or pass the variable you
want to affect as a parameter.
OK, I will pass **res** as a parameter like this:
#define CASE_READ_CLEAR(res, x, clear) \
case (x): { \
res = read_sysreg_s(SYS_##x##_EL1); \
if (clear) \
write_sysreg_s(0, SYS_##x##_EL1); \
break; \
}
Also, what ensures the synchronisation of this write? How is the W1TC
aspect enforced?
aest_proc is just call in irq context, one ras error is just routed to
one core, so it is thread safe. And this is a Write-After-Read (WAR)
Hazards with dependence,can i assume that pipeline would guarantee
the order of writing and reading?
You are missing the point. WAR hazarding doesn't mean that the write
has taken effect, and can be delayed for as long as the CPU decides
to, until the nest context synchronisation event.
The W1TC question still stands.
[...] >
+static u64 aest_iomem_read_clear(u64 base, u32 offset, bool clear)Do you need the explicit synchronisation? What ordering are you trying
+{
+ u64 res;
+
+ res = readq((void *)(base + offset));
+ if (clear)
+ writeq(0, (void *)(base + offset));
to guarantee?
This read and write use the same address, pipeline would guarantee
the order of writing and reading.
You are missing the point again. Non-relaxed accessors come with a DMB
that enforces ordering with younger reads and older writes. Why do you
need those?
[...]>
+static int __init aest_register_gsi(u32 gsi, int trigger, void *data,Enabling the PPI before requesting it? Looks... great. And how does
+ irq_handler_t aest_irq_func)
+{
+ 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);
this work on a system that supports EPPIs, which are in the
[1119:1056] range?
It is better to enable it after request it, i will fix it next version.
My machine do not use EPPI as RAS interrupt, i can not test it now. Can
we support EPPI in later patch?
No, because you shouldn't even have to care. Can you see a single
driver in the tree that do this?
Sorry,I do not really understand this comment, should I use
Also, if you get a trigger as a parameter, why the IRQ_TYPE_NONE?
(IRQ_LEVEL | IRQ_PER_CPU)?
You tell me. Either the trigger is relevant, or it isn't. But I assume
it is passed as a parameter to the function for a good reason.
+ for_each_possible_cpu(cpu) {Why SHARED? Who would share a RAS interrupt?????
+ memcpy(per_cpu_ptr(ppi_data[ppi_idx], cpu), data,
+ sizeof(struct aest_node));
+ }
+ 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)) {
Multi AEST nodes may use the same interrupt, for example, one DDRC with
a RAS interrupt has two sub channels, these two sub channel is described
as two AEST node in AEST table, so they share the same one. In another
case, SMMU has two RAS node, TCU and TBU, they may also share the same
interrupt.
I still find it odd, but hey, if that's the way people want to handle
RAS, they might as well OR all of them and wire it to the RESET pin.
+ pr_err("failed to register AEST IRQ %d\n", irq);Same question about extended SPIs.
+ return -EINVAL;
All in all, this whole logic is totally useless. It isn't the driver's
job to classify the GIC INTIDs...
AEST use both PPI and SPI, it seems that AEST driver must recognize
INTID in order to request irq number with different function, do you
have better solution here?
Again, you should have to look at the INTID, ever. That's none of your
business, and you don't even know what interrupt controller the system
is presenting you anyway. The way to identify a per-CPU interrupt is
to use the irq_is_percpu_devid() helper, and not to mess with
pointless heuristics.
Thanks,
M.