[PATCH v2] KVM: arm64: Fix hyp_trace clock disabling

[PATCH v2] KVM: arm64: Fix hyp_trace clock disabling

[Vincent Donnefort]

Fix the disable path in hyp_trace_clock_enable(), which fell through to
re-initialize and reschedule the clock after cancelling the work. Return
early instead.

While at it, cleanup hyp_trace_clock::lock which is unused and
hyp_trace_clock::running which is redundant: the trace_remote framework
already serializes calls to the callback enable_tracing.

Fixes: b22888917fa4 ("KVM: arm64: Sync boot clock with the nVHE/pKVM hyp")
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

--- 

v1 -> v2:
  * Rollback hyp_trace_clock_enable() on __tracing_enable error (Sashiko)

v1: https://lore.kernel.org/all/alczBmnItMwq8xj4@google.com/

diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
index 2411b4c32932..9bfa368dd841 100644
--- a/arch/arm64/kvm/hyp_trace.c
+++ b/arch/arm64/kvm/hyp_trace.c
@@ -37,8 +37,6 @@ static struct hyp_trace_clock {
 	u32			shift;
 	struct delayed_work	work;
 	struct completion	ready;
-	struct mutex		lock;
-	bool			running;
 } hyp_clock;
 
 static void __hyp_clock_work(struct work_struct *work)
@@ -110,12 +108,9 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
 {
 	struct system_time_snapshot snap;
 
-	if (hyp_clock->running == enable)
-		return;
-
 	if (!enable) {
 		cancel_delayed_work_sync(&hyp_clock->work);
-		hyp_clock->running = false;
+		return;
 	}
 
 	ktime_get_snapshot_id(CLOCK_BOOTTIME, &snap);
@@ -128,7 +123,6 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
 	INIT_DELAYED_WORK(&hyp_clock->work, __hyp_clock_work);
 	schedule_delayed_work(&hyp_clock->work, msecs_to_jiffies(CLOCK_INIT_MS));
 	wait_for_completion(&hyp_clock->ready);
-	hyp_clock->running = true;
 }
 
 /* Access to this struct within the trace_remote_callbacks are protected by the trace_remote lock */
@@ -304,9 +298,15 @@ static void hyp_trace_unload(struct trace_buffer_desc *desc, void *priv)
 
 static int hyp_trace_enable_tracing(bool enable, void *priv)
 {
+	int ret;
+
 	hyp_trace_clock_enable(&hyp_clock, enable);
 
-	return kvm_call_hyp_nvhe(__tracing_enable, enable);
+	ret = kvm_call_hyp_nvhe(__tracing_enable, enable);
+	if (ret)
+		hyp_trace_clock_enable(&hyp_clock, !enable);
+
+	return ret;
 }
 
 static int hyp_trace_swap_reader_page(unsigned int cpu, void *priv)

base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
-- 
2.55.0.141.g00534a21ce-goog

Re: [PATCH v2] KVM: arm64: Fix hyp_trace clock disabling

[Fuad Tabba]

Hi Vincent,

On Wed, 15 Jul 2026 at 11:51, Vincent Donnefort <vdonnefort@google.com> wrote:
>
> Fix the disable path in hyp_trace_clock_enable(), which fell through to
> re-initialize and reschedule the clock after cancelling the work. Return
> early instead.
>
> While at it, cleanup hyp_trace_clock::lock which is unused and
> hyp_trace_clock::running which is redundant: the trace_remote framework
> already serializes calls to the callback enable_tracing.

Although the cleanup and the fix are to the same commit, could this be
split into the fix and the lock/running cleanup as a separate patch?
Since b22888917fa41 is already in a released kernel, the fix will
likely go to stable, and it'd be nice to keep that backport to just
the behavioural change.

Cheers,
/fuad

>
> Fixes: b22888917fa4 ("KVM: arm64: Sync boot clock with the nVHE/pKVM hyp")
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
>
> ---
>
> v1 -> v2:
>   * Rollback hyp_trace_clock_enable() on __tracing_enable error (Sashiko)
>
> v1: https://lore.kernel.org/all/alczBmnItMwq8xj4@google.com/
>
> diff --git a/arch/arm64/kvm/hyp_trace.c b/arch/arm64/kvm/hyp_trace.c
> index 2411b4c32932..9bfa368dd841 100644
> --- a/arch/arm64/kvm/hyp_trace.c
> +++ b/arch/arm64/kvm/hyp_trace.c
> @@ -37,8 +37,6 @@ static struct hyp_trace_clock {
>         u32                     shift;
>         struct delayed_work     work;
>         struct completion       ready;
> -       struct mutex            lock;
> -       bool                    running;
>  } hyp_clock;
>
>  static void __hyp_clock_work(struct work_struct *work)
> @@ -110,12 +108,9 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
>  {
>         struct system_time_snapshot snap;
>
> -       if (hyp_clock->running == enable)
> -               return;
> -
>         if (!enable) {
>                 cancel_delayed_work_sync(&hyp_clock->work);
> -               hyp_clock->running = false;
> +               return;
>         }
>
>         ktime_get_snapshot_id(CLOCK_BOOTTIME, &snap);
> @@ -128,7 +123,6 @@ static void hyp_trace_clock_enable(struct hyp_trace_clock *hyp_clock, bool enabl
>         INIT_DELAYED_WORK(&hyp_clock->work, __hyp_clock_work);
>         schedule_delayed_work(&hyp_clock->work, msecs_to_jiffies(CLOCK_INIT_MS));
>         wait_for_completion(&hyp_clock->ready);
> -       hyp_clock->running = true;
>  }
>
>  /* Access to this struct within the trace_remote_callbacks are protected by the trace_remote lock */
> @@ -304,9 +298,15 @@ static void hyp_trace_unload(struct trace_buffer_desc *desc, void *priv)
>
>  static int hyp_trace_enable_tracing(bool enable, void *priv)
>  {
> +       int ret;
> +
>         hyp_trace_clock_enable(&hyp_clock, enable);
>
> -       return kvm_call_hyp_nvhe(__tracing_enable, enable);
> +       ret = kvm_call_hyp_nvhe(__tracing_enable, enable);
> +       if (ret)
> +               hyp_trace_clock_enable(&hyp_clock, !enable);
> +
> +       return ret;
>  }
>
>  static int hyp_trace_swap_reader_page(unsigned int cpu, void *priv)
>
> base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
> --
> 2.55.0.141.g00534a21ce-goog
>

Re: [PATCH v2] KVM: arm64: Fix hyp_trace clock disabling

[Marc Zyngier]

On Mon, 20 Jul 2026 20:45:00 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> Hi Vincent,
> 
> On Wed, 15 Jul 2026 at 11:51, Vincent Donnefort <vdonnefort@google.com> wrote:
> >
> > Fix the disable path in hyp_trace_clock_enable(), which fell through to
> > re-initialize and reschedule the clock after cancelling the work. Return
> > early instead.
> >
> > While at it, cleanup hyp_trace_clock::lock which is unused and
> > hyp_trace_clock::running which is redundant: the trace_remote framework
> > already serializes calls to the callback enable_tracing.
> 
> Although the cleanup and the fix are to the same commit, could this be
> split into the fix and the lock/running cleanup as a separate patch?
> Since b22888917fa41 is already in a released kernel, the fix will
> likely go to stable, and it'd be nice to keep that backport to just
> the behavioural change.

Not sure I get what you mean here. From what I can see, this patch
fully applies to b22888917fa41. Given that this is the third version
for something that trivial (two from Vincent, one from you), can we
please agree on *something* quickly?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2] KVM: arm64: Fix hyp_trace clock disabling

[Fuad Tabba]

On Tue, 21 Jul 2026 at 07:52, 'Marc Zyngier' via kernel-team
<kernel-team@android.com> wrote:
>
> On Mon, 20 Jul 2026 20:45:00 +0100,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> >
> > Hi Vincent,
> >
> > On Wed, 15 Jul 2026 at 11:51, Vincent Donnefort <vdonnefort@google.com> wrote:
> > >
> > > Fix the disable path in hyp_trace_clock_enable(), which fell through to
> > > re-initialize and reschedule the clock after cancelling the work. Return
> > > early instead.
> > >
> > > While at it, cleanup hyp_trace_clock::lock which is unused and
> > > hyp_trace_clock::running which is redundant: the trace_remote framework
> > > already serializes calls to the callback enable_tracing.
> >
> > Although the cleanup and the fix are to the same commit, could this be
> > split into the fix and the lock/running cleanup as a separate patch?
> > Since b22888917fa41 is already in a released kernel, the fix will
> > likely go to stable, and it'd be nice to keep that backport to just
> > the behavioural change.
>
> Not sure I get what you mean here. From what I can see, this patch
> fully applies to b22888917fa41. Given that this is the third version
> for something that trivial (two from Vincent, one from you), can we
> please agree on *something* quickly?

No strong preference from my side. The fix and the cleanup are
correct, I just thought combining them in one commit might be less
than ideal. If that's not an issue:

Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>

Cheers,
/fuad


>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
>
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>