Re: [PATCH 2/2] edac: add support for Amazon's Annapurna Labs EDAC

Hawa, Hanna
2019-06-11T19:56:02Z

Hi James,

>
> Allowing linux to access these implementation-defined registers has come up before:
> https://www.spinics.net/lists/kernel/msg2750349.html
>
> It looks like you've navigated most of the issues. Accessing implementation-defined
> registers is frowned on, but this stuff can't be done generically until v8.2.
Sure, no planning to do this generally for all ARM a57/a72. I'm doing
this specific for alpine SoCs.

>
> This can't be done on 'all A57/A72' because some platforms may not have been integrated to
> have error-checking at L1/L2, (1.3 'Features' of [0] says the ECC protection for L1 data
> cache etc is optional). Even if they did, this stuff needs turning on in L2CTLR_EL1.
> These implementation-defined registers may be trapped by the hypervisor, I assume for your
> platform this is linux booted at EL2, so no hypervisor.
In Alpine-v2/Alpine-v3 Bit[21]-"ECC and parity enable" in L2CTRL_EL1 is
enabled in the firmware.

>
>
>> +#define ARM_CA57_CPUMERRSR_INDEX_OFF (0)
>> +#define ARM_CA57_CPUMERRSR_INDEX_MASK (0x3FFFF)
>
> (GENMASK() would make it quicker and easier to compare this with the datasheet)
Will be used in next patchset.

>
>
>> +#define ARM_CA57_L2_INC_PLRU_RAM 0x18
>
> A57 describes this one as 'PF RAM'...
will be updated.

>
>
>
> Linux supports versions of binutils that choke on this syntax.
> See the sys_reg() definitions in arm64's asm/sysreg.h that define something you can feed
> to read_sysreg_s(). It would save having these wrapper functions.
>
> commit 72c583951526 ("arm64: gicv3: Allow GICv3 compilation with older binutils") for the
> story.
Great, I'll use sys_reg(), read_sysreg_s(), and remove the wrapper
functions.

>
>
>
> | #define ARM_CA57_CPUMERRSR_VALID BIT(31)
> | if (!(val & ARM_CA57_CPUMERRSR_VALID))
>
> would be easier to read, the same goes for 'fatal' as its a single bit.
Will be fixed, here and in al_a57_edac_l2merrsr.

>
>
>> + edac_device_handle_ce(edac_dev, 0, 0, "L2 Error");
>
> How do we know this was corrected?
>
> 6.4.8 "Error Correction Code" has "Double-bit ECC errors set the fatal bit." in a
> paragraph talking about the L1 memory system.
I'll check fatal field to check if it's uncorrected/corrected.

>
> "L2 Error" ? Copy and paste?
copy/paste mistake, will be fixed.

>
>
>> + edac_printk(KERN_CRIT, DRV_NAME, "CPU%d L1 %serror detected\n",
>> + cpu, (fatal) ? "Fatal " : "");
>> + edac_printk(KERN_CRIT, DRV_NAME, "RAMID=");
>> +
>> + switch (ramid) {
>> + case ARM_CA57_L1_I_TAG_RAM:
>> + pr_cont("'L1-I Tag RAM' index=%d way=%d", index, way);
>> + break;
>> + case ARM_CA57_L1_I_DATA_RAM:
>> + pr_cont("'L1-I Data RAM' index=%d bank= %d", index, way);
>> + break;
>
> Is index/way information really useful? I can't replace way-3 on the system, nor can I
> stop it being used. If its useless, I'd rather we don't bother parsing and printing it out.
I'll remove the index/way information, and keep CPUMERRSR_EL1 value
print (who want this information can parse the value and get the
index/bank status)

>
>
>> + pr_cont(", repeat=%d, other=%d (CPUMERRSR_EL1=0x%llx)\n", repeat, other,
>> + val);
>
> 'other' here is another error, but we don't know the ramid.
> 'repeat' is another error for the same ramid.
>
> Could we still feed this stuff into edac? This would make the counters accurate if the
> polling frequency isn't quite fast enough.
There is no API in EDAC to increase the counters, I can increase by
accessing the ce_count/ue_count from
edac_device_ctl_info/edac_device_instance structures if it's okay..

>
>
>> +static void al_a57_edac_l2merrsr(void *arg)
>> +{
>
>> + edac_device_handle_ce(edac_dev, 0, 0, "L2 Error");
>
> How do we know this is corrected?
>
> If looks like L2CTLR_EL1[20] might force fatal 1/0 to map to uncorrected/corrected. Is
> this what you are depending on here?
No - not on this. Reporting all the errors as corrected seems to be bad.

Can i be depends on fatal field?

if (fatal)
edac_device_handle_ue(edac_dev, 0, 0, "L2 Error");
else
edac_device_handle_ce(edac_dev, 0, 0, "L2 Error");

How can L2CTLR_EL1[20] force fatal?

>
> (it would be good to have a list of integration-time and firmware dependencies this driver
> has, for the next person who tries to enable it on their system and complains it doesn't
> work for them)
Where can i add such information?

>
>
>> + case ARM_CA57_L2_INC_PLRU_RAM:
>> + pr_cont("'L2 Inclusion PLRU RAM'");
>
> The A57 TRM describes this as "Inclusion PF RAM", and notes its only in r1p0 or later,
> (but doesn't say what it is). The A72 TRM describes the same encoding as "Inclusion PLRU
> RAM", which is something to do with its replacement policy. It has control bits that A57's
> version doesn't, so these are not the same thing.
>
> Disambiguating A57/A72 here is a load of faff, 'L2 internal metadata' probably covers both
> cases, but unless these RAMs are replaceable or can be disabled, there isn't much point
> working out which one it was.
Will be fixed to 'L2 internal metadata'

>
>> + pr_cont(", cpuid/way=%d, repeat=%d, other=%d (L2MERRSR_EL1=0x%llx)\n",
>> + way, repeat, other, val);
>
> cpuid could be useful if you can map it back to the cpu number linux has.
> If you can spot that cpu-7 is experiencing more errors than it should, you can leave it
> offline.
>
> To do this you'd need to map each L2MERRSR_EL1's '0-3' range back to the CPUs they
> actually are. The gic's 'ppi-partitions' does this with phandles, e.g.
> Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml. You could add a
> similar shaped thing to the l2-cacheX node in the DT, (or in your edac node, but it is a
> property of the cache integration).
As in L1 prints, I'll remove non-relevant prints.

>
>
>> + /*
>> + * Use get_online_cpus/put_online_cpus to prevent the online CPU map
>> + * changing while reads the L1/L2 error status
>
> For walking the list of offline cpus, this makes sense. But you schedule work without
> waiting, it may get run after you drop the cpus_read_lock()...,
Will update the smp_call_function_single() call function to wait.

>
>
>> + get_online_cpus();
>
> The comment above these helpers is:
> | /* Wrappers which go away once all code is converted */
>
> cpus_read_lock()?
Will be updated.

>
>
>> + for_each_online_cpu(cpu) {
>> + /* Check L1 errors */
>> + smp_call_function_single(cpu, al_a57_edac_cpumerrsr, edac_dev,
>> + 0);
>
> As you aren't testing for big/little, wouldn't on_each_cpu() here be simpler?
Could be simpler for L1, how can be implemented for L2?

>
> As you don't wait, what stops al_a57_edac_cpumerrsr() feeding two errors into
> edac_device_handle_ce() at the same time? Do you need a spinlock in al_a57_edac_cpumerrsr()?
Will call smp_call_function_single() with wait.

>
>
>> + cluster = topology_physical_package_id(cpu);
>
> Hmm, I'm not sure cluster==package is guaranteed to be true forever.
>
> If you describe the L2MERRSR_EL1 cpu mapping in your DT you could use that. Otherwise
> pulling out the DT using something like the arch code's parse_cluster().
I rely on that it's alpine SoC specific driver.

>
>
>> + if (cluster != last_cluster) {
>> + smp_call_function_single(cpu, al_a57_edac_l2merrsr,
>> + edac_dev, 0);
>> + last_cluster = cluster;
>> + }
>
> Here you depend on the CPUs being listed in cluster-order in the DT. I'm fairly sure the
> numbering is arbitrary: On my Juno 0,3,4,5 are the A53 cluster, and 1,2 are the A57 cluster.
>
> If 1,3,5 were cluster-a and 2,4,6 were cluster-b, you would end up calling
> al_a57_edac_l2merrsr() for each cpu. As you don't wait, they could race.
>
> If you can get a cpu-mask for each cluster, smp_call_function_any() would to the
> pick-one-online-cpu work for you.
Again, I rely on that it's alpine SoC specific driver.
How can I get cpu-mask for each cluster? from DT?

>
>
>> +static int al_a57_edac_remove(struct platform_device *pdev)
>> +{
>> + struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev);
>> +
>> + edac_device_del_device(edac_dev->dev);
>> + edac_device_free_ctl_info(edac_dev);
>
> Your poll function schedule work on other CPUs and didn't wait, is it possible
> al_a57_edac_l2merrsr() is still using this memory when you free it?
This will be okay, after using wait in smp_call_function_single().
>
>
>> +MODULE_LICENSE("GPL");
>
> | MODULE_LICENSE("GPL v2");
>
> To match the SPDX header?
Will be fixed.

Thanks for your detailed review.

Thanks,
Hanna
>
>
>
> Thanks,
>
> James
>
>
> [0]
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0488c/DDI0488C_cortex_a57_mpcore_r1p0_trm.pdf
>