On Thu, Aug 01 2024 at 18:49, David Woodhouse wrote:

On Thu, 2024-08-01 at 16:21 +0200, Thomas Gleixner wrote:

quoted

The stop sequence is wrong:

    When there is a count in progress, writing a new LSB before the
    counter has counted down to 0 and rolled over to FFFFh, WILL stop
    the counter.  However, if the LSB is loaded AFTER the counter has
    rolled over to FFFFh, so that an MSB now exists in the counter, then
    the counter WILL NOT stop.

The original i8253 datasheet says:

    1) Write 1st byte stops the current counting
    2) Write 2nd byte starts the new count

It says that for mode zero ("Interrupt on Terminal Count"), yes. But in
that mode, shouldn't the IRQ only fire *one* more time anyway, rather
than repeatedly? That should be OK, shouldn't it?

"When terminal count is reached, the output will go high and remain
high until the selected count register is reloaded wityh the mode or a
new count is loaded".

I just confirmed that this is the case on KVM.

It's OK for it to keep *counting* as long as it stops firing
interrupts, isn't it?

Yes. So the sequence should stop KVM from trying to inject
interrupts. Maybe someone fixes it to actually stop fiddling with the
counter too :)

Either way, this is somewhat orthogonal to the patch I posted in 
https://lore.kernel.org/kvm/6cd62b5058e11a6262cb2e798cc85cc5daead3b1.camel@infradead.org/T/#u (local)
for the fact that we don't shut down the PIT at *all* if we aren't ever
going to use it.

I'm glad I decided to export a function from the clocksource driver and
just *call* it from pit_timer_init() though. Means we can bikeshed the
shutdown sequence in *one* place and it isn't duplicated.

Right. Though we don't have to make this conditional on hypervisor I
think.

Thanks,

        tglx