| Message ID | 20200128103459.19413-13-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: Add ITS tests | expand |
On 28/01/2020 11.34, Eric Auger wrote: > Let's link getchar.o to use puts and getchar from the > tests. > > Then allow tests belonging to the migration group to > trigger the migration from the test code by putting > "migrate" into the uart. Then the code can wait for the > migration completion by using getchar(). > > The __getchar implement is minimalist as it just reads the > data register. It is just meant to read the single character > emitted at the end of the migration by the runner script. > > It is not meant to read more data (FIFOs are not enabled). > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v2 -> v3: > - take the lock > - assert if more than 16 chars > - removed Thomas' R-b > --- > arm/Makefile.common | 2 +- > arm/run | 2 +- > lib/arm/io.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/arm/Makefile.common b/arm/Makefile.common > index b8988f2..a123e85 100644 > --- a/arm/Makefile.common > +++ b/arm/Makefile.common > @@ -32,7 +32,7 @@ CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib > asm-offsets = lib/$(ARCH)/asm-offsets.h > include $(SRCDIR)/scripts/asm-offsets.mak > > -cflatobjs += lib/util.o > +cflatobjs += lib/util.o lib/getchar.o > cflatobjs += lib/alloc_phys.o > cflatobjs += lib/alloc_page.o > cflatobjs += lib/vmalloc.o > diff --git a/arm/run b/arm/run > index 277db9b..a390ca5 100755 > --- a/arm/run > +++ b/arm/run > @@ -61,6 +61,6 @@ fi > M+=",accel=$ACCEL" > command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" > command+=" -display none -serial stdio -kernel" > -command="$(timeout_cmd) $command" > +command="$(migration_cmd) $(timeout_cmd) $command" > > run_qemu $command "$@" > diff --git a/lib/arm/io.c b/lib/arm/io.c > index 99fd315..d8e7745 100644 > --- a/lib/arm/io.c > +++ b/lib/arm/io.c > @@ -87,6 +87,34 @@ void puts(const char *s) > spin_unlock(&uart_lock); > } > > +static int ____getchar(void) Three underscores? ... that's quite a lot already. I'd maybe rather name the function "do_getchar" or something similar instead. Or simply merge the code into the __getchar function below - it's just three lines. > +{ > + int c; > + > + spin_lock(&uart_lock); > + c = readb(uart0_base); > + spin_unlock(&uart_lock); > + > + return c ? : -1; Just a matter of taste, but I prefer the elvis operator without space in between. > +} > + > +/* > + * 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. Where are the 16 bytes buffered if FIFOs are disabled? > + */ > +int __getchar(void) > +{ > + int c = ____getchar(); > + static int count; > + > + if (c != -1) > + ++count; > + > + assert(count < 16); > + > + return c; > +} The above comments were only nits ... feel free to ignore them if you don't want to respin the series just because of this. Thomas
Hi Thomas, On 1/29/20 9:07 AM, Thomas Huth wrote: > On 28/01/2020 11.34, Eric Auger wrote: >> Let's link getchar.o to use puts and getchar from the >> tests. >> >> Then allow tests belonging to the migration group to >> trigger the migration from the test code by putting >> "migrate" into the uart. Then the code can wait for the >> migration completion by using getchar(). >> >> The __getchar implement is minimalist as it just reads the >> data register. It is just meant to read the single character >> emitted at the end of the migration by the runner script. >> >> It is not meant to read more data (FIFOs are not enabled). >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> v2 -> v3: >> - take the lock >> - assert if more than 16 chars >> - removed Thomas' R-b >> --- >> arm/Makefile.common | 2 +- >> arm/run | 2 +- >> lib/arm/io.c | 28 ++++++++++++++++++++++++++++ >> 3 files changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/arm/Makefile.common b/arm/Makefile.common >> index b8988f2..a123e85 100644 >> --- a/arm/Makefile.common >> +++ b/arm/Makefile.common >> @@ -32,7 +32,7 @@ CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib >> asm-offsets = lib/$(ARCH)/asm-offsets.h >> include $(SRCDIR)/scripts/asm-offsets.mak >> >> -cflatobjs += lib/util.o >> +cflatobjs += lib/util.o lib/getchar.o >> cflatobjs += lib/alloc_phys.o >> cflatobjs += lib/alloc_page.o >> cflatobjs += lib/vmalloc.o >> diff --git a/arm/run b/arm/run >> index 277db9b..a390ca5 100755 >> --- a/arm/run >> +++ b/arm/run >> @@ -61,6 +61,6 @@ fi >> M+=",accel=$ACCEL" >> command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" >> command+=" -display none -serial stdio -kernel" >> -command="$(timeout_cmd) $command" >> +command="$(migration_cmd) $(timeout_cmd) $command" >> >> run_qemu $command "$@" >> diff --git a/lib/arm/io.c b/lib/arm/io.c >> index 99fd315..d8e7745 100644 >> --- a/lib/arm/io.c >> +++ b/lib/arm/io.c >> @@ -87,6 +87,34 @@ void puts(const char *s) >> spin_unlock(&uart_lock); >> } >> >> +static int ____getchar(void) > > Three underscores? ... that's quite a lot already. I'd maybe rather name > the function "do_getchar" or something similar instead. Or simply merge > the code into the __getchar function below - it's just three lines. OK > >> +{ >> + int c; >> + >> + spin_lock(&uart_lock); >> + c = readb(uart0_base); >> + spin_unlock(&uart_lock); >> + >> + return c ? : -1; > > Just a matter of taste, but I prefer the elvis operator without space in > between. OK > >> +} >> + >> +/* >> + * 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. > > Where are the 16 bytes buffered if FIFOs are disabled? I think this is in the PL011 data register (UARTDR), 4 words. https://developer.arm.com/docs/ddi0183/latest/programmers-model/register-descriptions/data-register-uartdr > >> + */ >> +int __getchar(void) >> +{ >> + int c = ____getchar(); >> + static int count; >> + >> + if (c != -1) >> + ++count; >> + >> + assert(count < 16); >> + >> + return c; >> +} > > The above comments were only nits ... feel free to ignore them if you > don't want to respin the series just because of this. No Problem. Thank you for your time. Thanks Eric > > Thomas >
diff --git a/arm/Makefile.common b/arm/Makefile.common index b8988f2..a123e85 100644 --- a/arm/Makefile.common +++ b/arm/Makefile.common @@ -32,7 +32,7 @@ CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib asm-offsets = lib/$(ARCH)/asm-offsets.h include $(SRCDIR)/scripts/asm-offsets.mak -cflatobjs += lib/util.o +cflatobjs += lib/util.o lib/getchar.o cflatobjs += lib/alloc_phys.o cflatobjs += lib/alloc_page.o cflatobjs += lib/vmalloc.o diff --git a/arm/run b/arm/run index 277db9b..a390ca5 100755 --- a/arm/run +++ b/arm/run @@ -61,6 +61,6 @@ fi M+=",accel=$ACCEL" command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" command+=" -display none -serial stdio -kernel" -command="$(timeout_cmd) $command" +command="$(migration_cmd) $(timeout_cmd) $command" run_qemu $command "$@" diff --git a/lib/arm/io.c b/lib/arm/io.c index 99fd315..d8e7745 100644 --- a/lib/arm/io.c +++ b/lib/arm/io.c @@ -87,6 +87,34 @@ void puts(const char *s) spin_unlock(&uart_lock); } +static int ____getchar(void) +{ + int c; + + 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 = ____getchar(); + static int count; + + if (c != -1) + ++count; + + assert(count < 16); + + return c; +} /* * Defining halt to take 'code' as an argument guarantees that it will
Let's link getchar.o to use puts and getchar from the tests. Then allow tests belonging to the migration group to trigger the migration from the test code by putting "migrate" into the uart. Then the code can wait for the migration completion by using getchar(). The __getchar implement is minimalist as it just reads the data register. It is just meant to read the single character emitted at the end of the migration by the runner script. It is not meant to read more data (FIFOs are not enabled). Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v2 -> v3: - take the lock - assert if more than 16 chars - removed Thomas' R-b --- arm/Makefile.common | 2 +- arm/run | 2 +- lib/arm/io.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-)