Re: [PATCH 2/3] arm64: arch_timer: Allow an workaround descriptor to provide vdso_clock_mode

[Marc Zyngier]

On 2020-07-02 11:28, Mark Rutland wrote:

On Wed, Jul 01, 2020 at 05:18:23PM +0100, Marc Zyngier wrote:

As we are about to disable the vdso for compat tasks in some circumstances, let's allow a workaround descriptor to provide the vdso_clock_mode that

matches the platform.

Signed-off-by: Marc Zyngier <m...@kernel.org>
---
 arch/arm64/include/asm/arch_timer.h  | 3 +++
 drivers/clocksource/arm_arch_timer.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h

index 7ae54d7d333a..fb8dfcbf9c01 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -18,6 +18,8 @@
 #include <linux/smp.h>
 #include <linux/types.h>

+#include <vdso/clocksource.h>
+
 #include <clocksource/arm_arch_timer.h>

 #if IS_ENABLED(CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND)
@@ -58,6 +60,7 @@ struct arch_timer_erratum_workaround {
        u64 (*read_cntvct_el0)(void);

int (*set_next_event_phys)(unsigned long, struct clock_event_device *); int (*set_next_event_virt)(unsigned long, struct clock_event_device *);

+       enum vdso_clock_mode vdso_clock_mode;
 };

 DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c

index ecf7b7db2d05..f828835c568f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c

@@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa

        if (wa->read_cntvct_el0) {
                clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
                vdso_default = VDSO_CLOCKMODE_NONE;
+       } else {
+               clocksource_counter.vdso_clock_mode = wa->vdso_clock_mode;
+               vdso_default = wa->vdso_clock_mode;
        }

I fear that we're liable to forget to set vdso_clock_mode on new errata
that don't need a read_cntvct_el0 hook, and if so we'll happen to set
these to 0 (i.e. VDSO_CLOCKMODE_NONE).

For now could we instead have a boolan disable_compat_vdso, with this
being:

| if (wa->read_cntvct_el0) {
|       clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
|       vdso_default = VDSO_CLOCKMODE_NONE;
| } else if (wa->disable_compat_vdso) {

| clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT; | vdso_default = wa->vdso_clock_mode = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;

| }

Sure, that'd work too.

Do we need to handle the comination of a workaround seeting NONE and one

setting ARCHTIMER_NOCOMPAT?

Not yet. Probably coming any time now. Which is why we may need to
look into per-CPU vdso data pages (Broonie has a series that could
be of some use, apparently).

Thanks,

        M.
--
Jazz is not dead. It just smells funny...