Re: [PATCH 1/1] drivers/edac: Add L1 and L2 error detection for A53 and A57

[Mark Rutland <mark.rutland@arm.com>]

Hi Sascha,

On Thu, Jan 07, 2021 at 11:38:19AM +0100, Sascha Hauer wrote:
> The Cortex A53 and A57 cores have error detection capabilities for the
> L1/L2 Caches, this patch adds a driver for them.
> 
> Unfortunately there is no robust way to inject errors into the caches,
> so this driver doesn't contain any code to actually test it. It has
> been tested though with code taken from an older version of this driver
> found here: https://lkml.org/lkml/2018/3/14/1203. For reasons stated
> in this thread the error injection code is not suitable for mainline,
> so it is removed from the driver.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/edac/Kconfig              |   6 +
>  drivers/edac/Makefile             |   1 +
>  drivers/edac/cortex_arm64_l1_l2.c | 218 ++++++++++++++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100644 drivers/edac/cortex_arm64_l1_l2.c

[...]

> +#define SYS_CPUMERRSR_EL1			sys_reg(3, 1, 15, 2, 2)
> +#define SYS_L2MERRSR_EL1			sys_reg(3, 1, 15, 2, 3)

Just to note -- these registers are IMPLEMENTATION DEFINED, and so may
be trapped by a hypervisor or messed around with arbitrarily by FW.

> +static void read_errors(void *data)
> +{
> +	struct merrsr *merrsr = data;
> +
> +	merrsr->cpumerr = read_sysreg_s(SYS_CPUMERRSR_EL1);
> +	write_sysreg_s(0, SYS_CPUMERRSR_EL1);
> +	merrsr->l2merr = read_sysreg_s(SYS_L2MERRSR_EL1);
> +	write_sysreg_s(0, SYS_L2MERRSR_EL1);
> +}

... and so here the MRS instructions may behave as UNDEFINED in some
cases. Foer example, KVM would trap these via HCR_EL2.TIDCP, and emulate
these as UNDEFINED.

> +static const struct of_device_id cortex_arm64_edac_of_match[] = {
> +	{ .compatible = "arm,cortex-a53" },
> +	{ .compatible = "arm,cortex-a57" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cortex_arm64_edac_of_match);

... and so it's not safe to probe this based on the CPU compatible
string alone. If we really need to support this, it will need its own
binding (to describe that the reigsters are safe to access and behave in
a useful way).

Thanks,
Mark.