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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, Jan 07, 2021 at 11:15:20AM +0000, Mark Rutland wrote:
> 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).

Rob objected to adding dedicated nodes to attach the driver to. That
leaves adding a property like "edac-enabled" or similar to the CPU nodes
when edac is desired. Would that be ok?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |