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

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

On Tue, Feb 07 2023 at 09:14, lirongqing@xxxxxxxxx wrote:
> @@ -117,11 +110,6 @@ static int pit_shutdown(struct clock_event_device *evt)
>  
>  	outb_p(0x30, PIT_MODE);
>  
> -	if (i8253_clear_counter_on_shutdown) {
> -		outb_p(0, PIT_CH0);
> -		outb_p(0, PIT_CH0);
> -	}
> -

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

The above does not make sure it actually has not rolled over and it
obviously is initiating the new count by writing the MSB too. So it does
not work on real hardware either.

The proper sequence is:

         // Switch to mode 0
         outb_p(0x30, PIT_MODE);
         // Load the maximum value to ensure there is no rollover
         outb_p(0xff, PIT_CH0);
         // Writing MSB starts the counter from 0xFFFF and clears rollover
         outb_p(0xff, PIT_CH0);
         // Stop the counter by writing only LSB
         outb_p(0xff, PIT_CH0);

That works on real hardware and fails on KVM... So much for the claim
that KVM follows the spec :)

So yes, the code is buggy, but instead of deleting it, we rather fix it,
no?

User space test program below.

Thanks,

        tglx
---
#include <stdio.h>
#include <unistd.h>
#include <sys/io.h>

typedef unsigned char	u8;
typedef unsigned short	u16;

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

BUILDIO(b, b, u8)

#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[])
{
	if (argc > 1)
		is8254 = 1;

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

	dump_pit();

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

	dump_pit();
	usleep(1000);
	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 stop\n");
	outb(0x30, PIT_MODE);
	outb(0xFF, PIT_CH0);
	outb(0xFF, PIT_CH0);
	outb(0xFF, PIT_CH0);

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

	return 0;
}