Re: [PATCH] Semihost SYS_READC implementation (v4)

["Keith Packard" <keithp@keithp.com>]

[-- Attachment #1: Type: text/plain, Size: 3051 bytes --]

Alex Bennée <alex.bennee@linaro.org> writes:

> I can see the use for this but I'd like to know what you are testing
> with. We only have very basic smoketests in check-tcg but I've tested
> with the latest arm-semihosting tests and they are all fine so no
> regressions there.

I'm adding semihosting support to picolibc
(https://keithp.com/picolibc/) and need a way to automate tests for it's
SYS_READC support, so eventually I'll have automated tests there, but
that depends on qemu support...

> Please keep version history bellow --- so they get dropped when the
> patch is applied.

Sure, I'll edit the mail before sending. In my repo, I'm leaving the
version history in git so I can keep track of it.

>> v4:
>> 	Add qemu_semihosting_console_init to stubs/semihost.c for
>> 	hosts that don't support semihosting
>
> This doesn't appear to be in the diff which is why I'm seeing a compile
> failure for non-CONFIG_SEMIHOST machines. However...

Argh! Just git operation failure -- I'm building another patch on top of
this (for RISC-V semihosting support) and the stubs/semihost.c change
got caught in there. My overall check for changes missed this mistake.

>> +
>> +#include <poll.h>
>
> Headers should go at the top.

Thanks; I've fixed this file and hw/semihosting/console.c

>>      case TARGET_SYS_READC:
>> -        qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
>> -        return 0;
>> +        return qemu_semihosting_console_inc(env);
>
> I'm not sure this would be correct if there was no character available.
> The docs imply it blocks although don't say so explicitly AFAICT.

Here's what the docs say:

        https://static.docs.arm.com/100863/0200/semihosting.pdf

        Return

        On exit, the RETURN REGISTER contains the byte read from the console.

If this call didn't block, they'd have to define a value which indicated
that no byte was available? Openocd also implements SYS_READC using
'getchar()', which definitely blocks. So, at least qemu would be the
same.

I realize it's really weird to block the whole emulation for this call,
but given the target environment (deeply embedded devices), it's quite
convenient as the whole qemu process blocks, instead of spinning.

>> +    /* connect semihosting console input if requested */
>> +    qemu_semihosting_console_init();
>> +
>
> I'd rather rename qemu_semihosting_connect_chardevs to
> qemu_semihosting_init and keep all our bits of semihosting setup in
> there rather than having multiple calls out of vl.c

I also thought this would be a nice cleanup. However, the last caller to
qemu_chr_fe_set_handlers gets the focus for input, and connect_chardevs
is called before the serial ports and monitor are initialized, so
semihosting gets pushed aside and stdio input ends up hooked to the monitor
instead.

Adding this function and placing the call after the other stdio users
get hooked up means that semihosting starts with the input focus.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]