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

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

quoted

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.

quoted

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 :)

I don't think we care about the counter value, as that's *calculated*
on demand when the guest tries to read from it. Or, more to the point,
*if* the guest tries to read from it.

As opposed to the interrupt, which is a timer in the VMM which takes a
CPU out of guest mode and incurs steal time, just to waggle a pin on
the emulated PICs for no good reason.

quoted

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.

Right, we don't *have* to. I vacillated about that and almost ripped it
out before sending the patch, but came down on the side of "hardware is
a steaming pile of crap and if I don't *have* to change its behaviour,
let's not touch it".

I justify my cowardice on the basis that it doesn't *matter* if a
hardware implementation is still toggling the IRQ pin; in that case
it's only a few irrelevant transistors which are busy, and it doesn't
translate to steal time.