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.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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
