bhyve: atkbdc: do not lose keyboard bytes when the buffer is full

[wanpengqian_gmail.com]

Summary

With scancode translation enabled, atkbdc_kbd_read() took a byte from
the PS/2 keyboard's queue also when the controller's own buffer was
full, and atkbdc_kbd_queue_data() then dropped it ("atkbd data buffer
full"). The guest lost a make or a break code: a missing character, or
a key that stays down.

Leave the byte in the keyboard's queue until there is room, as the path
without translation does.

Signed-off-by: Wanpeng Qian <[e-mail]>
Sponsored by: keelos.dev

Test Plan

main (f958aa7e7), FreeBSD 16.0-CURRENT guest with one vCPU, started with the UEFI firmware (edk2-bhyve, which enables the keyboard and the scancode translation) and -s 29,fbuf,tcp=127.0.0.1:5900.

In the guest stall 6 (below) keeps the vCPU busy with interrupts disabled for 6 seconds, so the guest does not read the keyboard. Meanwhile a VNC client types the 12 keys a to l: 24 scancodes after translation, the controller's buffer holds 15.

The bytes the guest then reads from port 0x60 (dtrace on bhyve's atkbdc_data_handler()):

before, 15 of 24; the break code of h and the keys i to l are lost

1e 9e 30 b0 2e ae 20 a0 12 92 21 a1 22 a2 23

after, all 24

1e 9e 30 b0 2e ae 20 a0 12 92 21 a1 22 a2 23 a3 17 97 24 a4 25 a5 26 a6

What the guest makes of it (evdev events of the AT keyboard, 9 keys a to i typed): before, 7 keys pressed and released, then h pressed and repeating, never released, and no i; after, 9 keys pressed and released.

How it was found (FreeBSD 14.5): text pasted into a Windows 11 guest by a remote display client that types it as key events. bhyve printed "atkbd data buffer full" 60 times during one paste; characters were missing, and a Shift whose break code was lost stayed down.

stall.c

/*
 * stall SECONDS: keep the CPU busy with interrupts disabled (IOPL 3 from /dev/io), so that a
 * guest with one vCPU does not serve any device for that long.
 */
#include <sys/types.h>
#include <sys/sysctl.h>
#include <machine/cpufunc.h>
#include <err.h>
#include <fcntl.h>
#include <stdlib.h>

int
main(int argc, char **argv)
{
	uint64_t freq, end;
	size_t len = sizeof(freq);
	int secs = argc > 1 ? atoi(argv[1]) : 5;

	if (open("/dev/io", O_RDWR) < 0)
		err(1, "/dev/io");
	if (sysctlbyname("machdep.tsc_freq", &freq, &len, NULL, 0) != 0)
		err(1, "machdep.tsc_freq");
	disable_intr();
	end = rdtsc() + freq * secs;	/* no system call in here: it would let interrupts in */
	while (rdtsc() < end)
		;
	enable_intr();
	return (0);
}

[bnovkov]

usr.sbin/bhyve/amd64/atkbdc.c

227–228

This check is used in multiple places now, so I suggest splitting it out into its own function (e.g., atkbdc_kdb_queue_full or similar) to make the check a bit clearer.

[wanpengqian_gmail.com]

Add atkbdc_kbd_queue_full() and use it for all four buffer-full checks (bnovkov).

[wanpengqian_gmail.com]

Thanks, done: added atkbdc_kbd_queue_full() and used it for all four checks in the file. Retested: all 24 scancodes arrive, no "atkbd data buffer full".