Re: [PATCH] clockevents/drivers/i8253: Do not zero timer counter in shutdown

[David Woodhouse]

On Thu, 2024-08-01 at 22:31 +0100, David Woodhouse wrote:
> On 1 August 2024 22:22:56 BST, Thomas Gleixner <t...@linutronix.de> wrote:
> > On Thu, Aug 01 2024 at 21:49, David Woodhouse wrote:
> > > On Thu, 2024-08-01 at 22:00 +0200, Thomas Gleixner wrote:
> > > > > 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.
> > > > 
> > > > On real hardware it translates to power...
> > > 
> > > Perhaps, although I'd guess it's a negligible amount. Still, happy to
> > > be brave and make it unconditional. Want a new version of the patch?
> > 
> > Let'ss fix the shutdown sequence first (See Michaels latest mail) and
> > then do the clockevents_i8253_init() change on top.
> 
> Makes sense.

On second thoughts, let's add clockevent_i8253_disable() first and call
it when the PIT isn't being used, then we can bikeshed the precise
behaviour of that function to our hearts' content.

I think it should look like this. Revised version of your test program
attached.

void clockevent_i8253_disable(void)
{
        raw_spin_lock(&i8253_lock);

        /*
         * Writing the MODE register should stop the counter, according to
         * the datasheet. This appears to work on real hardware (well, on
         * modern Intel and AMD boxes; I didn't dig the Pegasos out of the
         * shed).
         *
         * However, some virtual implementations differ, and the MODE change
         * doesn't have any effect until either the counter is written (KVM
         * in-kernel PIT) or the next interrupt (QEMU). And in those cases,
         * it may not stop the *count*, only the interrupts. Although in
         * the virt case, that probably doesn't matter, as the value of the
         * counter will only be calculated on demand if the guest reads it;
         * it's the interrupts which cause steal time.
         *
         * Hyper-V apparently has a bug where even in mode 0, the IRQ keeps
         * firing repeatedly if the counter is running. But it *does* do the
         * right thing when the MODE register is written.
         *
         * So: write the MODE and then load the counter, which ensures that
         * the IRQ is stopped on those buggy virt implementations. And then
         * write the MODE again, which is the right way to stop it.
         */
        outb_p(0x30, PIT_MODE);
        outb_p(0, PIT_CH0);
        outb_p(0, PIT_CH0);

        outb_p(0x30, PIT_MODE);

        raw_spin_unlock(&i8253_lock);
}


#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdint.h>
#include <sys/io.h>

typedef unsigned char	uint8_t;
typedef unsigned short	uint16_t;

#define BUILDIO(bwl, bw, type)						\
static __always_inline void __out##bwl(type value, uint16_t port)	\
{									\
	asm volatile("out" #bwl " %" #bw "0, %w1"			\
		     : : "a"(value), "Nd"(port));			\
}									\
									\
static __always_inline type __in##bwl(uint16_t port)			\
{									\
	type value;							\
	asm volatile("in" #bwl " %w1, %" #bw "0"			\
		     : "=a"(value) : "Nd"(port));			\
	return value;							\
}

BUILDIO(b, b, uint8_t)

#define inb __inb
#define outb __outb

#define PIT_MODE	0x43
#define PIT_CH0		0x40
#define PIT_CH2		0x42

static int is8254;

static void dump_pit(void)
{
	if (is8254) {
		// Latch and output counter and status
		outb(0xC2, PIT_MODE);
		printf("%02x %02x %02x\n", inb(PIT_CH0), inb(PIT_CH0), inb(PIT_CH0));
	} else {
		// Latch and output counter
		outb(0x0, PIT_MODE);
		printf("%02x %02x\n", inb(PIT_CH0), inb(PIT_CH0));
	}
}

int main(int argc, char* argv[])
{
	int nr_counts = 2;

	if (argc > 1)
		nr_counts = atoi(argv[1]);

	if (argc > 2)
		is8254 = 1;

	if (ioperm(0x40, 4, 1) != 0)
		return 1;

	dump_pit();

	printf("Set oneshot\n");
	outb(0x38, PIT_MODE);
	outb(0x00, PIT_CH0);
	outb(0x0F, PIT_CH0);

	dump_pit();
	usleep(1000);
	dump_pit();

	printf("Set periodic\n");
	outb(0x34, PIT_MODE);
	outb(0x00, PIT_CH0);
	outb(0x0F, PIT_CH0);

	dump_pit();
	usleep(1000);
	dump_pit();
	dump_pit();
	usleep(100000);
	dump_pit();
	usleep(100000);
	dump_pit();

	printf("Set stop (%d counter writes)\n", nr_counts);
	outb(0x30, PIT_MODE);
	while (nr_counts--)
		outb(0xFF, PIT_CH0);

	dump_pit();
	usleep(100000);
	dump_pit();
	usleep(100000);
	dump_pit();

	printf("Set MODE 0\n");
	outb(0x30, PIT_MODE);

	dump_pit();
	usleep(100000);
	dump_pit();
	usleep(100000);
	dump_pit();

	return 0;
}

smime.p7s
Description: S/MIME cryptographic signature