[bug report] KVM: x86: Unify TSC logic (sleeping in atomic?)

[bug report] KVM: x86: Unify TSC logic (sleeping in atomic?)

[Dan Carpenter]

I don't know why I'm seeing this static checker warning only now.  All
this code looks to be about 15 years old...

Commit e48672fa25e8 ("KVM: x86: Unify TSC logic") from Aug 19, 2010
(linux-next), leads to the following Smatch static checker warning:

	arch/x86/kernel/tsc.c:1214 mark_tsc_unstable()
	warn: sleeping in atomic context

The code path is:

vcpu_load() <- disables preempt
-> kvm_arch_vcpu_load()
   -> mark_tsc_unstable() <- sleeps

virt/kvm/kvm_main.c
   166  void vcpu_load(struct kvm_vcpu *vcpu)
   167  {
   168          int cpu = get_cpu();
                          ^^^^^^^^^^
This get_cpu() disables preemption.

   169  
   170          __this_cpu_write(kvm_running_vcpu, vcpu);
   171          preempt_notifier_register(&vcpu->preempt_notifier);
   172          kvm_arch_vcpu_load(vcpu, cpu);
   173          put_cpu();
   174  }

arch/x86/kvm/x86.c
  4979          if (unlikely(vcpu->cpu != cpu) || kvm_check_tsc_unstable()) {
  4980                  s64 tsc_delta = !vcpu->arch.last_host_tsc ? 0 :
  4981                                  rdtsc() - vcpu->arch.last_host_tsc;
  4982                  if (tsc_delta < 0)
  4983                          mark_tsc_unstable("KVM discovered backwards TSC");
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
It seems pretty unlikely that we'll get a backwards tsc.  I remember
dealing with some of these seeing this 20 years ago.

  4984  

arch/x86/kernel/tsc.c
    1206 void mark_tsc_unstable(char *reason)
    1207 {
    1208         if (tsc_unstable)
    1209                 return;
    1210 
    1211         tsc_unstable = 1;
    1212         if (using_native_sched_clock())
    1213                 clear_sched_clock_stable();
--> 1214         disable_sched_clock_irqtime();
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I'm on an allmodconfig on x86_64 so this expands to the
function below...

    1215         pr_info("Marking TSC unstable due to %s\n", reason);
    1216 
    1217         clocksource_mark_unstable(&clocksource_tsc_early);
    1218         clocksource_mark_unstable(&clocksource_tsc);
    1219 }

kernel/jump_label.c
   245  void static_key_disable(struct static_key *key)
   246  {
   247          cpus_read_lock();
                ^^^^^^^^^^^^^^^^
This lock has a might_sleep() in it which triggers the static checker
warning.

   248          static_key_disable_cpuslocked(key);
   249          cpus_read_unlock();
   250  }

regards,
dan carpenter

Re: [bug report] KVM: x86: Unify TSC logic (sleeping in atomic?)

[Sean Christopherson]

+sched folks

On Fri, Jan 17, 2025, Dan Carpenter wrote:
> I don't know why I'm seeing this static checker warning only now.  All
> this code looks to be about 15 years old...
> 
> Commit e48672fa25e8 ("KVM: x86: Unify TSC logic") from Aug 19, 2010
> (linux-next), leads to the following Smatch static checker warning:

That's not the problematic commit.  This popped because commit 8722903cbb8f
("sched: Define sched_clock_irqtime as static key") in the tip tree turned
sched_clock_irqtime into a static key (it was a simple "int").

https://lore.kernel.org/all/20250103022409.2544-2-laoar.shao@gmail.com

> 	arch/x86/kernel/tsc.c:1214 mark_tsc_unstable()
> 	warn: sleeping in atomic context
> 
> The code path is:
> 
> vcpu_load() <- disables preempt
> -> kvm_arch_vcpu_load()
>    -> mark_tsc_unstable() <- sleeps
> 
> virt/kvm/kvm_main.c
>    166  void vcpu_load(struct kvm_vcpu *vcpu)
>    167  {
>    168          int cpu = get_cpu();
>                           ^^^^^^^^^^
> This get_cpu() disables preemption.
> 
>    169  
>    170          __this_cpu_write(kvm_running_vcpu, vcpu);
>    171          preempt_notifier_register(&vcpu->preempt_notifier);
>    172          kvm_arch_vcpu_load(vcpu, cpu);
>    173          put_cpu();
>    174  }
> 
> arch/x86/kvm/x86.c
>   4979          if (unlikely(vcpu->cpu != cpu) || kvm_check_tsc_unstable()) {
>   4980                  s64 tsc_delta = !vcpu->arch.last_host_tsc ? 0 :
>   4981                                  rdtsc() - vcpu->arch.last_host_tsc;
>   4982                  if (tsc_delta < 0)
>   4983                          mark_tsc_unstable("KVM discovered backwards TSC");
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> It seems pretty unlikely that we'll get a backwards tsc.

TSC will go "backwards" if the host is suspended, in which case host TSC gets
reset to 0.

>     1215         pr_info("Marking TSC unstable due to %s\n", reason);
>     1216 
>     1217         clocksource_mark_unstable(&clocksource_tsc_early);
>     1218         clocksource_mark_unstable(&clocksource_tsc);
>     1219 }
> 
> kernel/jump_label.c
>    245  void static_key_disable(struct static_key *key)
>    246  {
>    247          cpus_read_lock();
>                 ^^^^^^^^^^^^^^^^
> This lock has a might_sleep() in it which triggers the static checker
> warning.
> 
>    248          static_key_disable_cpuslocked(key);
>    249          cpus_read_unlock();
>    250  }
> 
> regards,
> dan carpenter

Re: [bug report] KVM: x86: Unify TSC logic (sleeping in atomic?)

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Fri, Jan 17, 2025 at 09:51:41AM -0800, Sean Christopherson <seanjc@google.com> wrote:
> That's not the problematic commit.  This popped because commit 8722903cbb8f
> ("sched: Define sched_clock_irqtime as static key") in the tip tree turned
> sched_clock_irqtime into a static key (it was a simple "int").
> 
> https://lore.kernel.org/all/20250103022409.2544-2-laoar.shao@gmail.com

Thanks for the analysis, it's spot on. What a bad luck.

Is there a precedent for static key switching in non-preemptible
contexts?

More generally, why does KVM do this tsc check in vcpu_load? Shouldn't
possible unstability for that cpu be already checked and decided at boot
(regardless of KVM)? (Unless unstability itself is not stable property.
Which means any previously measured IRQ times are skewed.)

(Or a third option to revert the static-keyness if Yafang doesn't have
some backing data that it improves generic performance. The robot [1]
didn't like the fourth patch but that one actually adds back the
complexity guarded previously.)

Michal

[1] https://lore.kernel.org/all/202501101033.b66070d2-lkp@intel.com/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [bug report] KVM: x86: Unify TSC logic (sleeping in atomic?)

[Sean Christopherson]

On Fri, Jan 24, 2025, Michal Koutný wrote:
> On Fri, Jan 17, 2025 at 09:51:41AM -0800, Sean Christopherson <seanjc@google.com> wrote:
> > That's not the problematic commit.  This popped because commit 8722903cbb8f
> > ("sched: Define sched_clock_irqtime as static key") in the tip tree turned
> > sched_clock_irqtime into a static key (it was a simple "int").
> > 
> > https://lore.kernel.org/all/20250103022409.2544-2-laoar.shao@gmail.com
> 
> Thanks for the analysis, it's spot on. What a bad luck.
> 
> Is there a precedent for static key switching in non-preemptible
> contexts?

Abuse static_key_deferred to push the patching to a workqueue?

> More generally, why does KVM do this tsc check in vcpu_load? 

The logic triggers when a vCPU migrates to a different pCPU.  The code detects
that the case where TSC is inconsistent between pCPUs, and would cause time to go
backwards from the guest's perspective.  E.g. TSC = X on CPU0, migrate to CPU1
where TSC = X - Y.

> Shouldn't possible unstability for that cpu be already checked and decided at
> boot (regardless of KVM)? (Unless unstability itself is not stable property.
> Which means any previously measured IRQ times are skewed.)

This isn't a problem that's unique to KVM.  The clocksource watchdog also marks
TSC unstable from non-preemptible context (presumably from IRQ context?)

  clocksource_watchdog()
  |
  -> spin_lock(&watchdog_lock);
     |
     -> __clocksource_unstable()
        |
        -> clocksource.mark_unstable() == tsc_cs_mark_unstable()
           |
           -> disable_sched_clock_irqtime()

Uh, and sched_clock_register() toggles the static key on with IRQs disabled...

	/* Cannot register a sched_clock with interrupts on */
	local_irq_save(flags);

	...

	/* Enable IRQ time accounting if we have a fast enough sched_clock() */
	if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
		enable_sched_clock_irqtime();

	local_irq_restore(flags);

> (Or a third option to revert the static-keyness if Yafang doesn't have

Given there are issues all over the place, either a revert or a generic fix.

Re: [bug report] KVM: x86: Unify TSC logic (sleeping in atomic?)

[Yafang Shao]

On Fri, Jan 24, 2025 at 11:36 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Jan 24, 2025, Michal Koutný wrote:
> > On Fri, Jan 17, 2025 at 09:51:41AM -0800, Sean Christopherson <seanjc@google.com> wrote:
> > > That's not the problematic commit.  This popped because commit 8722903cbb8f
> > > ("sched: Define sched_clock_irqtime as static key") in the tip tree turned
> > > sched_clock_irqtime into a static key (it was a simple "int").
> > >
> > > https://lore.kernel.org/all/20250103022409.2544-2-laoar.shao@gmail.com
> >
> > Thanks for the analysis, it's spot on. What a bad luck.
> >
> > Is there a precedent for static key switching in non-preemptible
> > contexts?
>
> Abuse static_key_deferred to push the patching to a workqueue?
>
> > More generally, why does KVM do this tsc check in vcpu_load?
>
> The logic triggers when a vCPU migrates to a different pCPU.  The code detects
> that the case where TSC is inconsistent between pCPUs, and would cause time to go
> backwards from the guest's perspective.  E.g. TSC = X on CPU0, migrate to CPU1
> where TSC = X - Y.
>
> > Shouldn't possible unstability for that cpu be already checked and decided at
> > boot (regardless of KVM)? (Unless unstability itself is not stable property.
> > Which means any previously measured IRQ times are skewed.)
>
> This isn't a problem that's unique to KVM.  The clocksource watchdog also marks
> TSC unstable from non-preemptible context (presumably from IRQ context?)
>
>   clocksource_watchdog()
>   |
>   -> spin_lock(&watchdog_lock);
>      |
>      -> __clocksource_unstable()
>         |
>         -> clocksource.mark_unstable() == tsc_cs_mark_unstable()
>            |
>            -> disable_sched_clock_irqtime()
>
> Uh, and sched_clock_register() toggles the static key on with IRQs disabled...
>
>         /* Cannot register a sched_clock with interrupts on */
>         local_irq_save(flags);
>
>         ...
>
>         /* Enable IRQ time accounting if we have a fast enough sched_clock() */
>         if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
>                 enable_sched_clock_irqtime();
>
>         local_irq_restore(flags);
>
> > (Or a third option to revert the static-keyness if Yafang doesn't have
>
> Given there are issues all over the place, either a revert or a generic fix.

It seems that introducing a generic fix for this static key change
might not be worth it. I’ll proceed with sending a revert.


--
Regards
Yafang