Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
Peter Maydell writes: > On Fri, 10 May 2019 at 17:59, Alex Bennée wrote: >> >> >> Peter Maydell writes: >> >> > On Thu, 9 May 2019 at 17:59, Alex Bennée wrote: >> >> >> >> For running system tests we want to be able to re-direct output to a >> >> file like we do with serial output. This does the wiring to allow us >> >> to treat semihosting like just another character output device. >> >> >> >> diff --git a/qemu-options.hx b/qemu-options.hx >> >> index 51802cbb266..6aa3a08c2fb 100644 >> >> --- a/qemu-options.hx >> >> +++ b/qemu-options.hx >> >> @@ -3975,12 +3975,12 @@ STEXI >> >> Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only). >> >> ETEXI >> >> DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config, >> >> -"-semihosting-config >> >> [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \ >> >> +"-semihosting-config >> >> [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \ >> >> "semihosting configuration\n", >> >> QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 | >> >> QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2) >> > >> > As you can see in the docs here, semihosting is supported on >> > five guest architectures, so we should implement this new >> > feature for all of them, not just arm. >> >> As I've introduced this for testing I see no reason not to add support >> for other architectures. However I was hoping this is something that >> could be done organically as other system tests get enabled. > > IME transitions done "organically" really means "slowly, and > nobody ever gets round to actually completing them". > Semihosting is a user-facing feature, so if we want to add > the user feature of allowing output to go to a chardev we > should add it properly, I think. So a quick review of the current semi output: - MIPS This has a fairly generalised open/read/write support with special handling for open/close on /dev/std[out/err/in]. There is also a UHI_plog which currently just printf's to stdout - xtensa This already has support for a sim_console char device as part of the xtensa sim platform. Otherwise the TARGET_SYS_open can open paths directly (which I assume could include stdio) which then read/write. - m68k This has the usual open/read/write/close support directly to the FD's as well as support for integrating with the gdbstub via gdb_do_syscall. - lm32 Although based on the m68k semithosting support it lacks the gdbstub integration. It has the usual open/read/write/close stuff. - NIOS2 Again based on the m68k semihosting but looks like it was taken later because it retains the gdbsub integration support. Generally all the other semihosting stuff looks a lot cleaner - probably an indication of being done later and avoiding some of the warts of the early arm semihosting code. One difference with ARM is it has specific calls aside from the open/read/write/close (WRITEC/WRITE0) which are specifically aimed at "console" type logging. They don't seem to require an explicit open at the start and assume you can write to them from the get go. One question that would need to be answered is should the chardev support be generalised for all semihosts that can read/write to the stdio outputs or should we restrict it to the "console" log operations (xtensa sim, mips plog and ARM)? -- Alex Bennée
Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
Peter Maydell writes: > On Thu, 9 May 2019 at 17:59, Alex Bennée wrote: >> >> For running system tests we want to be able to re-direct output to a >> file like we do with serial output. This does the wiring to allow us >> to treat semihosting like just another character output device. >> >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 51802cbb266..6aa3a08c2fb 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -3975,12 +3975,12 @@ STEXI >> Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only). >> ETEXI >> DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config, >> -"-semihosting-config >> [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \ >> +"-semihosting-config >> [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \ >> "semihosting configuration\n", >> QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 | >> QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2) > > As you can see in the docs here, semihosting is supported on > five guest architectures, so we should implement this new > feature for all of them, not just arm. As I've introduced this for testing I see no reason not to add support for other architectures. However I was hoping this is something that could be done organically as other system tests get enabled. -- Alex Bennée
Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
On Fri, 10 May 2019 at 17:59, Alex Bennée wrote: > > > Peter Maydell writes: > > > On Thu, 9 May 2019 at 17:59, Alex Bennée wrote: > >> > >> For running system tests we want to be able to re-direct output to a > >> file like we do with serial output. This does the wiring to allow us > >> to treat semihosting like just another character output device. > >> > >> diff --git a/qemu-options.hx b/qemu-options.hx > >> index 51802cbb266..6aa3a08c2fb 100644 > >> --- a/qemu-options.hx > >> +++ b/qemu-options.hx > >> @@ -3975,12 +3975,12 @@ STEXI > >> Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only). > >> ETEXI > >> DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config, > >> -"-semihosting-config > >> [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \ > >> +"-semihosting-config > >> [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \ > >> "semihosting configuration\n", > >> QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 | > >> QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2) > > > > As you can see in the docs here, semihosting is supported on > > five guest architectures, so we should implement this new > > feature for all of them, not just arm. > > As I've introduced this for testing I see no reason not to add support > for other architectures. However I was hoping this is something that > could be done organically as other system tests get enabled. IME transitions done "organically" really means "slowly, and nobody ever gets round to actually completing them". Semihosting is a user-facing feature, so if we want to add the user feature of allowing output to go to a chardev we should add it properly, I think. thanks -- PMM
Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
On Thu, 9 May 2019 at 17:59, Alex Bennée wrote: > > For running system tests we want to be able to re-direct output to a > file like we do with serial output. This does the wiring to allow us > to treat semihosting like just another character output device. > > diff --git a/qemu-options.hx b/qemu-options.hx > index 51802cbb266..6aa3a08c2fb 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3975,12 +3975,12 @@ STEXI > Enable semihosting mode (ARM, M68K, Xtensa, MIPS, Nios II only). > ETEXI > DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config, > -"-semihosting-config > [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \ > +"-semihosting-config > [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \ > "semihosting configuration\n", > QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32 | > QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2) As you can see in the docs here, semihosting is supported on five guest architectures, so we should implement this new feature for all of them, not just arm. thanks -- PMM
Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
On Fri, 10 May 2019 at 15:05, Alex Bennée wrote: > Only for the first one though.. that said I'm sure the write string is > leaking when we do gdb output with whatever lock_user_string is trying > to achieve. Yes, there looks like there's a leak there. (The fix is complicated because we need to check whether the string buffer is required to hang around until the asynchronous gdb operation is finished and the arm_semi_cb is invoked, or whether we can free it as soon as arm_gdb_syscall() returns.) lock_user_string is basically "give me a host pointer to the string at this address in guest memory": * on softmmu, the 'lock' operation copies the contents of guest memory into a local buffer, and 'unlock' then frees the buffer (possibly copying the updated local buffer contents back to the guest) * on linux-user, 'lock' does the guest-addr-to-host-addr conversion, and if DEBUG_REMAP is defined then it will also copy it into a separate buffer (and unlock will copy it back) thanks -- PMM
Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
Richard Henderson writes: > On 5/9/19 11:55 PM, Alex Bennée wrote: >> >> Richard Henderson writes: >> >>> On 5/9/19 9:58 AM, Alex Bennée wrote: @@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void) { return NULL; } + +static inline Chardev *semihosting_get_chardev(void) +{ +return NULL; +} >>> >>> Isn't the point of this function to avoid... >> >> Yes... but... >> >>> -return write(STDERR_FILENO, &c, 1); +#ifdef CONFIG_SOFTMMU + Chardev *chardev = semihosting_get_chardev(); + if (chardev) { + return qemu_chr_write_all(chardev, (uint8_t *) &c, >>> 1); >> >> The qemu_chr device stuff doesn't have such stubs and is softmmu only as >> well. *sigh* > > Ah, I see. Yes that's a problem. > > Well at least you don't need the "else\n#endif\n{" ugliness. You have the > return out of the then block. Only for the first one though.. that said I'm sure the write string is leaking when we do gdb output with whatever lock_user_string is trying to achieve. > > > r~ -- Alex Bennée
Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
On 5/9/19 11:55 PM, Alex Bennée wrote: > > Richard Henderson writes: > >> On 5/9/19 9:58 AM, Alex Bennée wrote: >>> @@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void) >>> { >>> return NULL; >>> } >>> + >>> +static inline Chardev *semihosting_get_chardev(void) >>> +{ >>> +return NULL; >>> +} >> >> Isn't the point of this function to avoid... > > Yes... but... > >> >>> -return write(STDERR_FILENO, &c, 1); >>> +#ifdef CONFIG_SOFTMMU >>> + Chardev *chardev = semihosting_get_chardev(); >>> + if (chardev) { >>> + return qemu_chr_write_all(chardev, (uint8_t *) &c, >> 1); > > The qemu_chr device stuff doesn't have such stubs and is softmmu only as > well. *sigh* Ah, I see. Yes that's a problem. Well at least you don't need the "else\n#endif\n{" ugliness. You have the return out of the then block. r~
Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
Richard Henderson writes: > On 5/9/19 9:58 AM, Alex Bennée wrote: >> @@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void) >> { >> return NULL; >> } >> + >> +static inline Chardev *semihosting_get_chardev(void) >> +{ >> +return NULL; >> +} > > Isn't the point of this function to avoid... Yes... but... > >> -return write(STDERR_FILENO, &c, 1); >> +#ifdef CONFIG_SOFTMMU >> + Chardev *chardev = semihosting_get_chardev(); >> + if (chardev) { >> + return qemu_chr_write_all(chardev, (uint8_t *) &c, >1); The qemu_chr device stuff doesn't have such stubs and is softmmu only as well. *sigh* I guess stub it out in the header as well? >> + } else >> +#endif >> + { >> + return write(STDERR_FILENO, &c, 1); >> + } > > ... this ifdef? > > Better to change > > - char c; > + uint8_t c; > > above to avoid the cast in the call to qemu_chr_write_all? > Or perhaps we should adjust qemu_chr_write_all to take void*? > > > r~ -- Alex Bennée
Re: [Qemu-devel] [PATCH v1 05/23] semihosting: enable chardev backed output
On 5/9/19 9:58 AM, Alex Bennée wrote: > @@ -51,12 +51,18 @@ static inline const char *semihosting_get_cmdline(void) > { > return NULL; > } > + > +static inline Chardev *semihosting_get_chardev(void) > +{ > +return NULL; > +} Isn't the point of this function to avoid... > -return write(STDERR_FILENO, &c, 1); > +#ifdef CONFIG_SOFTMMU > + Chardev *chardev = semihosting_get_chardev(); > + if (chardev) { > + return qemu_chr_write_all(chardev, (uint8_t *) &c, 1); > + } else > +#endif > + { > + return write(STDERR_FILENO, &c, 1); > + } ... this ifdef? Better to change - char c; + uint8_t c; above to avoid the cast in the call to qemu_chr_write_all? Or perhaps we should adjust qemu_chr_write_all to take void*? r~