| Message ID | 465ef721-a220-74cf-70dd-9b65fa523b1b@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 14, 2018 at 12:43:40PM +0100, Paolo Bonzini wrote: > On 08/02/2018 13:59, Andrew Jones wrote: > > On Thu, Feb 08, 2018 at 12:06:07PM +0100, Thomas Huth wrote: > >> On 07.02.2018 20:03, Andrew Jones wrote: > >>> Move the unit test specific h_get_term_char() to powerpc common code > >>> and call it getchar(). Other architectures may want to implement > >>> getchar() too (ARM will in a coming patch), so put the prototype in > >>> libcflat.h > >> > >> Using the libc name getchar() here sounds wrong. h_get_term_char() > >> behaves differently compared to the libc getchar(), e.g. it does not > >> block if there are no characters available. > >> I think you should either name this function differently, or implement a > >> behavior that is more close to the libc getchar() to avoid future confusion. > > > > Good point. I guess I should rename it, because we'd need to anyway in > > order to share getchar()'s implementation among other architectures, > > unless we duplicated the looping and EOF returning. Any suggestion for > > the name? __getchar() or stdin_readb()? > > Like this? Precisely, although I'm not sure it needs its own file. Since it might be nice to eventually clean up (remove?) libcflat.h, maybe we should create lib/stdio.[ch] and lib/stdlib.[ch] files. Implementations for libc functions (like getchar), that have their prototypes in lib/stdio.h, could all get thrown together in lib/stdio.c. Could be nice to define EOF in lib/stdio.h too. Thanks, drew > > diff --git a/lib/getchar.c b/lib/getchar.c > new file mode 100644 > index 0000000..26f6b6b > --- /dev/null > +++ b/lib/getchar.c > @@ -0,0 +1,11 @@ > +#include "libcflat.h" > +#include "asm/barrier.h" > + > +int getchar(void) > +{ > + int c; > + > + while ((c = __getchar()) == -1) > + cpu_relax(); > + return c; > +} > diff --git a/lib/libcflat.h b/lib/libcflat.h > index c680b69..cc56553 100644 > --- a/lib/libcflat.h > +++ b/lib/libcflat.h > @@ -82,6 +82,7 @@ typedef u64 phys_addr_t; > #define INVALID_PHYS_ADDR (~(phys_addr_t)0) > > extern void puts(const char *s); > +extern int __getchar(void); > extern int getchar(void); > extern void exit(int code); > extern void abort(void); > diff --git a/lib/powerpc/hcall.c b/lib/powerpc/hcall.c > index d65af93..7b05265 100644 > --- a/lib/powerpc/hcall.c > +++ b/lib/powerpc/hcall.c > @@ -32,7 +32,7 @@ void putchar(int c) > hcall(H_PUT_TERM_CHAR, vty, nr_chars, chars); > } > > -int getchar(void) > +int __getchar(void) > { > register unsigned long r3 asm("r3") = H_GET_TERM_CHAR; > register unsigned long r4 asm("r4") = 0; /* 0 == default vty */ > @@ -41,5 +41,5 @@ int getchar(void) > asm volatile (" sc 1 " : "+r"(r3), "+r"(r4), "=r"(r5) > : "r"(r3), "r"(r4)); > > - return r3 == H_SUCCESS && r4 > 0 ? r5 >> 48 : 0; > + return r3 == H_SUCCESS && r4 > 0 ? r5 >> 48 : -1; > } > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common > index 21dea40..63dc166 100644 > --- a/powerpc/Makefile.common > +++ b/powerpc/Makefile.common > @@ -30,6 +30,7 @@ asm-offsets = lib/$(ARCH)/asm-offsets.h > include $(SRCDIR)/scripts/asm-offsets.mak > > cflatobjs += lib/util.o > +cflatobjs += lib/getchar.o > cflatobjs += lib/alloc_phys.o > cflatobjs += lib/alloc.o > cflatobjs += lib/devicetree.o > diff --git a/powerpc/sprs.c b/powerpc/sprs.c > index 5b5a540..6744bd8 100644 > --- a/powerpc/sprs.c > +++ b/powerpc/sprs.c > @@ -285,8 +285,7 @@ int main(int argc, char **argv) > > if (pause) { > puts("Now migrate the VM, then press a key to continue...\n"); > - while (getchar() == 0) > - cpu_relax(); > + (void) getchar(); > } else { > puts("Sleeping...\n"); > handle_exception(0x900, &dec_except_handler, NULL); > > > Paolo
On 14/02/2018 13:38, Andrew Jones wrote: > Precisely, although I'm not sure it needs its own file. Since it might be > nice to eventually clean up (remove?) libcflat.h, maybe we should create > lib/stdio.[ch] and lib/stdlib.[ch] files. Implementations for libc > functions (like getchar), that have their prototypes in lib/stdio.h, could > all get thrown together in lib/stdio.c. > > Could be nice to define EOF in lib/stdio.h too. Yes, I agree on all accounts! Paolo
On Wed, Feb 14, 2018 at 02:25:07PM +0100, Paolo Bonzini wrote: > On 14/02/2018 13:38, Andrew Jones wrote: > > Precisely, although I'm not sure it needs its own file. Since it might be > > nice to eventually clean up (remove?) libcflat.h, maybe we should create > > lib/stdio.[ch] and lib/stdlib.[ch] files. Implementations for libc > > functions (like getchar), that have their prototypes in lib/stdio.h, could > > all get thrown together in lib/stdio.c. > > > > Could be nice to define EOF in lib/stdio.h too. > > Yes, I agree on all accounts! > OK, I'll send a libc-ifying clean up series and then rebase patches 5-8 of this series on that series. Thanks, drew
diff --git a/lib/getchar.c b/lib/getchar.c new file mode 100644 index 0000000..26f6b6b --- /dev/null +++ b/lib/getchar.c @@ -0,0 +1,11 @@ +#include "libcflat.h" +#include "asm/barrier.h" + +int getchar(void) +{ + int c; + + while ((c = __getchar()) == -1) + cpu_relax(); + return c; +} diff --git a/lib/libcflat.h b/lib/libcflat.h index c680b69..cc56553 100644 --- a/lib/libcflat.h +++ b/lib/libcflat.h @@ -82,6 +82,7 @@ typedef u64 phys_addr_t; #define INVALID_PHYS_ADDR (~(phys_addr_t)0) extern void puts(const char *s); +extern int __getchar(void); extern int getchar(void); extern void exit(int code); extern void abort(void); diff --git a/lib/powerpc/hcall.c b/lib/powerpc/hcall.c index d65af93..7b05265 100644 --- a/lib/powerpc/hcall.c +++ b/lib/powerpc/hcall.c @@ -32,7 +32,7 @@ void putchar(int c) hcall(H_PUT_TERM_CHAR, vty, nr_chars, chars); } -int getchar(void) +int __getchar(void) { register unsigned long r3 asm("r3") = H_GET_TERM_CHAR; register unsigned long r4 asm("r4") = 0; /* 0 == default vty */ @@ -41,5 +41,5 @@ int getchar(void) asm volatile (" sc 1 " : "+r"(r3), "+r"(r4), "=r"(r5) : "r"(r3), "r"(r4)); - return r3 == H_SUCCESS && r4 > 0 ? r5 >> 48 : 0; + return r3 == H_SUCCESS && r4 > 0 ? r5 >> 48 : -1; } diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common index 21dea40..63dc166 100644 --- a/powerpc/Makefile.common +++ b/powerpc/Makefile.common @@ -30,6 +30,7 @@ asm-offsets = lib/$(ARCH)/asm-offsets.h include $(SRCDIR)/scripts/asm-offsets.mak cflatobjs += lib/util.o +cflatobjs += lib/getchar.o cflatobjs += lib/alloc_phys.o cflatobjs += lib/alloc.o cflatobjs += lib/devicetree.o diff --git a/powerpc/sprs.c b/powerpc/sprs.c index 5b5a540..6744bd8 100644 --- a/powerpc/sprs.c +++ b/powerpc/sprs.c @@ -285,8 +285,7 @@ int main(int argc, char **argv) if (pause) { puts("Now migrate the VM, then press a key to continue...\n"); - while (getchar() == 0) - cpu_relax(); + (void) getchar(); } else { puts("Sleeping...\n"); handle_exception(0x900, &dec_except_handler, NULL);