Message-ID: <8734szr8y9.wl-maz@kernel.org>
Date: Sat, 09 Mar 2024 10:33:18 +0000
From: Marc Zyngier <maz@...nel.org>
To: Ruidong Tian <tianruidong@...ux.alibaba.com>
Cc: catalin.marinas@....com,
	will@...nel.org,
	lpieralisi@...nel.org,
	guohanjun@...wei.com,
	sudeep.holla@....com,
	xueshuai@...ux.alibaba.com,
	baolin.wang@...ux.alibaba.com,
	linux-kernel@...r.kernel.org,
	linux-acpi@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Tyler Baicar <baicar@...amperecomputing.com>
Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver

On Fri, 08 Mar 2024 03:43:30 +0000,
Ruidong Tian <tianruidong@...ux.alibaba.com> wrote:
> 
> 在 2024/3/4 20:07, Marc Zyngier 写道:
> > On Mon, 04 Mar 2024 11:15:16 +0000,
> > Ruidong Tian<tianruidong@...ux.alibaba.com>  wrote:
> >> diff --git a/arch/arm64/include/asm/ras.h b/arch/arm64/include/asm/rash
> >> 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.

[...]

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

[...]

> >> +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?

[...]

> >> +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?

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

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

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists