On 2016/10/27 20:23, Marc Zyngier wrote: > On 27/10/16 13:17, Ding Tianhong wrote: >> >> >> On 2016/10/27 18:58, Marc Zyngier wrote: >>> On 27/10/16 08:34, Ding Tianhong wrote: >>>> Erratum Hisilicon-161601 says that the ARM generic timer counter "has the >>>> potential to contain an erroneous value when the timer value changes". >>>> Accesses to TVAL (both read and write) are also affected due to the implicit counter >>>> read. Accesses to CVAL are not affected. >>>> >>>> The workaround is to reread the system count registers until the value of the second >>>> read is larger than the first one by less than 32, the system counter can be guaranteed >>>> not to return wrong value twice by back-to-back read and the error value is always larger >>>> than the correct one by 32. Writes to TVAL are replaced with an equivalent write to CVAL. >>>> >>>> The workaround is enabled if the hisilicon,erratum-161601 property is found in >>>> the timer node in the device tree. This can be overridden with the >>>> clocksource.arm_arch_timer.hisilicon-161601 boot parameter, which allows KVM >>>> users to enable the workaround until a mechanism is implemented to >>>> automatically communicate this information. >>>> >>>> Fix some description for fsl erratum a008585. >>>> >>>> v2: Significant rework based on feedback, including seperate the fsl erratum a008585 >>>> to another patch, update the erratum name and remove unwanted code. >>>> >>>> Signed-off-by: Ding Tianhong <dingtianhong@xxxxxxxxxx> >>>> --- >>>> Documentation/arm64/silicon-errata.txt | 1 + >>>> Documentation/kernel-parameters.txt | 9 ++++ >>>> arch/arm64/include/asm/arch_timer.h | 28 ++++++++++- >>>> drivers/clocksource/Kconfig | 14 +++++- >>>> drivers/clocksource/arm_arch_timer.c | 88 ++++++++++++++++++++++++++-------- >>>> 5 files changed, 118 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt >>>> index 405da11..70c5d5e 100644 >>>> --- a/Documentation/arm64/silicon-errata.txt >>>> +++ b/Documentation/arm64/silicon-errata.txt >>>> @@ -63,3 +63,4 @@ stable kernels. >>>> | Cavium | ThunderX SMMUv2 | #27704 | N/A | >>>> | | | | | >>>> | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | >>>> +| Hisilicon | Hip05/Hip06/Hip07 | #161601 | HISILICON_ERRATUM_161601| >>> >>> I've already commented on the alignment. Please read my initial review. >>> >> >> It sees misunderstood, fix it this time. >> >>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt >>>> index 6fa1d8a..735b4b6 100644 >>>> --- a/Documentation/kernel-parameters.txt >>>> +++ b/Documentation/kernel-parameters.txt >>>> @@ -707,6 +707,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted. >>>> erratum. If unspecified, the workaround is >>>> enabled based on the device tree. >>>> >>>> + clocksource.arm_arch_timer.hisilicon-161601= >>>> + [ARM64] >>>> + Format: <bool> >>>> + Enable/disable the workaround of Hisilicon >>>> + erratum 161601. This can be useful for KVM >>>> + guests, if the guest device tree doesn't show the >>>> + erratum. If unspecified, the workaround is >>>> + enabled based on the device tree. >>>> + >>>> clearcpuid=BITNUM [X86] >>>> Disable CPUID feature X for the kernel. See >>>> arch/x86/include/asm/cpufeatures.h for the valid bit >>>> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h >>>> index 118719d8..49b3041 100644 >>>> --- a/arch/arm64/include/asm/arch_timer.h >>>> +++ b/arch/arm64/include/asm/arch_timer.h >>>> @@ -29,7 +29,7 @@ >>>> >>>> #include <clocksource/arm_arch_timer.h> >>>> >>>> -#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) >>>> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601) >>>> extern struct static_key_false arch_timer_read_ool_enabled; >>>> #define needs_timer_erratum_workaround() \ >>>> static_branch_unlikely(&arch_timer_read_ool_enabled) >>>> @@ -65,11 +65,35 @@ extern struct arch_timer_erratum_workaround *erratum_workaround; >>>> _new; \ >>>> }) >>>> >>>> + >>>> + >>>> +/* >>>> + * The number of retries is an arbitrary value well beyond the highest number >>>> + * of iterations the loop has been observed to take. >>>> + * Verify whether the value of the second read is larger than the first by >>>> + * less than 32 is the only way to confirm the value is correct, the system >>>> + * counter can be guaranteed not to return wrong value twice by back-to-back read >>>> + * and the error value is always larger than the correct one by 32. >>>> + */ >>>> +#define __hisi_161601_read_reg(reg) ({ \ >>>> + u64 _old, _new; \ >>>> + int _retries = 200; \ >>> >>> Please document how this value was found (either in the code or in the >>> commit message). >> >> It really difficult to give the accurate standard, theoretically the error should not happened >> twice together, so maybe 2 is enough here, I just give a arbitrary value. >> >>> >>>> + \ >>>> + do { \ >>>> + _old = read_sysreg(reg); \ >>>> + _new = read_sysreg(reg); \ >>>> + _retries--; \ >>>> + } while (unlikely((_new - _old) >> 5) && _retries); \ >>>> + \ >>>> + WARN_ON_ONCE(!_retries); \ >>>> + _new; \ >>>> +}) >>> >>> Same remark as in the previous patch. >>> >> >> I think the sentence *Verify whether the value of the second read is larger than the first by >> less than 32 is the only way to confirm the value is correct* could explain why should *(_new - _old) >> 5*. >> it is the same as (_new - _old)/32, also mean the _new should never bigger than _old more than 32. > > This is not about the explanation of the erratum, but about the location > of the #define, which can be made private to the .c file instead of > being globally visible. > Ok, update it . Thanks Ding > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html