Hi,
Thanks for writing this. I've tested it with kvmtool, which emulates a 8250
UART:
Tested-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
This fixes a longstanding bug with kvmtool, where migrate_once() would read
the last character that was sent, and then think that migration was
completed even though it was never performed.
While we are on the subject of migration:
SKIP: gicv3: its-migration: Test requires at least 4 vcpus
Now migrate the VM, then press a key to continue...
INFO: gicv3: its-migration: Migration complete
SUMMARY: 1 tests, 1 skipped
That's extremely confusing. Why is migrate_once() executed after the
test_its_pending() function call without checking if the test was skipped?
Nitpicks below.
On Fri, Feb 16, 2024 at 03:02:10PM +0100, Thomas Huth wrote:
> getchar() can currently only be called once on arm since the implementation
> is a little bit too naïve: After the first character has arrived, the
> data register never gets set to zero again. To properly check whether a
> byte is available, we need to check the "RX fifo empty" on the pl011 UART
> or the "RX data ready" bit on the ns16550a UART instead.
>
> With this proper check in place, we can finally also get rid of the
> ugly assert(count < 16) statement here.
>
> Signed-off-by: Thomas Huth <thuth@xxxxxxxxxx>
> ---
> lib/arm/io.c | 34 ++++++++++++++--------------------
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index c15e57c4..836fa854 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -28,6 +28,7 @@ static struct spinlock uart_lock;
> */
> #define UART_EARLY_BASE (u8 *)(unsigned long)CONFIG_UART_EARLY_BASE
> static volatile u8 *uart0_base = UART_EARLY_BASE;
> +bool is_pl011_uart;
>
> static void uart0_init_fdt(void)
> {
> @@ -59,7 +60,10 @@ static void uart0_init_fdt(void)
> abort();
> }
>
> + is_pl011_uart = (i == 0);
> } else {
> + is_pl011_uart = !fdt_node_check_compatible(dt_fdt(), ret,
> + "arm,pl011");
> ret = dt_pbus_translate_node(ret, 0, &base);
> assert(ret == 0);
> }
> @@ -111,31 +115,21 @@ void puts(const char *s)
> spin_unlock(&uart_lock);
> }
>
> -static int do_getchar(void)
> +int __getchar(void)
> {
> - int c;
> + int c = -1;
>
> spin_lock(&uart_lock);
> - c = readb(uart0_base);
> - spin_unlock(&uart_lock);
> -
> - return c ?: -1;
> -}
> -
> -/*
> - * Minimalist implementation for migration completion detection.
> - * Without FIFOs enabled on the QEMU UART device we just read
> - * the data register: we cannot read more than 16 characters.
> - */
> -int __getchar(void)
> -{
> - int c = do_getchar();
> - static int count;
>
> - if (c != -1)
> - ++count;
> + if (is_pl011_uart) {
> + if (!(readb(uart0_base + 6 * 4) & 0x10)) /* RX not empty? */
I think it would be useful if the magic numbers were replaced by something
less opaque, something like:
if (!(readb(uart0_base + PL011_UARTFR) & PL011_UARTFR_RXFE))
> + c = readb(uart0_base);
> + } else {
> + if (readb(uart0_base + 5) & 0x01) /* RX data ready? */
Same as above, perhaps:
if (readb(uart0_base + UART16550_LSR) & UART16550_LSR_DR)
Naming of course being subject to taste.
Thanks,
Alex
> + c = readb(uart0_base);
> + }
>
> - assert(count < 16);
> + spin_unlock(&uart_lock);
>
> return c;
> }
> --
> 2.43.0
>
