在 2024/3/9 18:33, Marc Zyngier 写道:
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.h
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)
All these bits need to be defined in arch/arm64/tools/sysreg as
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.
There might have been some misunderstanding. AEST interface can be
SR(processor) or MMIO(GIC, SMMU, Memory), All these two types of nodes
have ERR<n>STATUS register, and AEST driver need to operate these filed
both in SR and MMIO register.
[...]
+#define CASE_READ_CLEAR(x, clear) \
+ case (x): { \
+ res = read_sysreg_s(SYS_##x##_EL1); \
+ if (clear) \
+ write_sysreg_s(0, SYS_##x##_EL1); \
+ break; \
+ }
Please don't use macros with side effects. This is horrible to debug.
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.
OK, i will add an ISB at the end of aest_proc to ensure all writes has
taken effect.
[...] >
+static u64 aest_iomem_read_clear(u64 base, u32 offset, bool clear)
+{
+ u64 res;
+
+ res = readq((void *)(base + offset));
+ if (clear)
+ writeq(0, (void *)(base + offset));
Do you need the explicit synchronisation? What ordering are you trying
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?
I get it. Relaxed accesses are still guaranteed to be ordered when
operating MMIO address. I will use readq/writeq_relaxed in next version.
[...]
>
+static int __init aest_register_gsi(u32 gsi, int trigger, void *data,
+ 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);
Enabling the PPI before requesting it? Looks... great. And how does
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?
OK，I will fix it next version.
Also, if you get a trigger as a parameter, why the IRQ_TYPE_NONE?
Sorry，I do not really understand this comment, should I use
(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) {
+ 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)) {
Why SHARED? Who would share a RAS interrupt?????
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.
It depends on HW designer especially for DDRC, and i think **SHARED** is
more compatible. But in fact, I'm not sure if driver should assume that
all RAS interruptions are no shared. If AEST driver set IRQ no shared,
does it need to loop through all AEST node first to ensure no node
sharing interrupt?
+ pr_err("failed to register AEST IRQ %d\n", irq);
+ return -EINVAL;
Same question about extended SPIs.
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.
OK, it will be fixed in next version.
Thanks,
M.
