Re: [kvm-unit-tests PATCH] lib/arm/io: Fix calling getchar() multiple times

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Feb 20, 2024 at 2:56 AM AEST, Alexandru Elisei wrote:
> Hi,
>
> Thanks for writing this. I've tested it with kvmtool, which emulates a 8250
> UART:
>
> Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
>
> 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?

Looks like it was done so the test is skipped without killing the harness
due to expected migration point not being reached.

After the multi-migration series, you could just put a migrate_quiet()
there as a quick fix.

But I'm thinking we could just remove the requirement for the harness
to see at least 1 migration point. The test itself knows how many
migrations it should perform, by how many times it calls migrate*().
It is somewhat a sanity test against test being invoked the wrong way
and not doing the migration, but how much is that worth...? Actually
we could have a new sideband message like "VM migration is skipped"
that doesn't do anything except tell the test harness to not fail
due to missing migration point. That gets the best of both worlds,
just needs tests to be updated.

Thanks,
Nick

>
> 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@redhat.com>
> > ---
> >  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
> >