Re: patch review: avoid com delay() in VMs

[Christoph Badura <bad%bsd.de@localhost>]

On Fri, Feb 07, 2025 at 06:35:04AM +0100, Emile `iMil' Heitor wrote:
> On Wed, 5 Feb 2025, Emile `iMil' Heitor wrote:
> Here are the results of discussions about finally getting rid of delay()
> and instead checking LSR_TXRDY (aka LSR_THRE) or LSR_TSRE (aka LSR_TEMT)
> 
> @@ -588,9 +589,13 @@ com_attach_subr(struct com_softc *sc)
>                         sc->sc_lcr = cflag2lcr(comcons_info.cflag);
>                         break;
>                 }
> +               /* wait for output to finish */
> +               timo = 10000;
> +               while (!ISSET(CSR_READ_1(regsp, COM_REG_LSR),
> +                   LSR_TXRDY | LSR_TSRE) && --timo)
> +                       continue;
> 
>                 /* Make sure the console is always "hardwired". */
> -               delay(10000);                   /* wait for output to finish */

The check for LSR_TSRE should be removed, it isn't relevant.

That is the same as what FreeBSD and OpenBSD  uses:
https://github.com/freebsd/freebsd-src/blob/e17e33f997d63107e3a6859cfe3c19eba041b424/sys/dev/uart/uart_dev_ns8250.c#L100

should be call to delay() in the loop.  Othewise, in a VM you really are
just causing (page) faults for the read to the emulated device register as
fast as the hypervisor and the emulation backend can handle them.

OpenBSD always uses delay(1) when looping on LSR_TXRDY (aka LSR_THRE):
https://github.com/openbsd/src/blob/30772397147598ca58ad6448458c89b20bcd5c6e/sys/dev/ic/com.c#L1259

FreeBSD, apart from the previously cited place, uses either DELAY(4) or a
dynamic delay calculated to allow transmission of a single character at
the current baud rate.

https://github.com/freebsd/freebsd-src/blob/e17e33f997d63107e3a6859cfe3c19eba041b424/sys/dev/uart/uart_dev_ns8250.c#L202

Search for other references to LSR_THRE, LSR_TXRDY, LSR_TEMT.

So:


> +               /* wait up to 10ms for output to finish */
> +               timo = 10000;
> +               while (!ISSET(CSR_READ_1(regsp, COM_REG_LSR),
> +                   LSR_TXRDY) && --timo)
> +                       delay(1);

--chris