Re: [PATCH v5 2/2] target/xtensa: Use semihosting/syscalls.h

[Max Filippov]

On Tue, Jun 28, 2022 at 5:36 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 6/28/22 19:08, Max Filippov wrote:
> > On Tue, Jun 28, 2022 at 4:43 AM Richard Henderson
> > <richard.henderson@linaro.org> wrote:

...

> >> diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
> >> index 946c71cb5b..5cca6a170e 100644
> >> --- a/hw/xtensa/sim.c
> >> +++ b/hw/xtensa/sim.c
> >> @@ -87,9 +87,6 @@ XtensaCPU *xtensa_sim_common_init(MachineState *machine)
> >>           xtensa_create_memory_regions(&sysram, "xtensa.sysram",
> >>                                        get_system_memory());
> >>       }
> >> -    if (serial_hd(0)) {
> >> -        xtensa_sim_open_console(serial_hd(0));
> >> -    }
> >
> > I've noticed that with this change '-serial stdio' and its variants are 
> > still
> > accepted in the command line, but now they do nothing.
>
> Pardon?  They certainly will do something, via writes to the serial hardware.

What I meant was that with '-serial' option prior to this change it was
possible to redirect the standard streams of the sim machine, to stdio,
or socket or wherever, but after this change the option will be accepted,
but the machine will always have its first three file descriptors connected
to the QEMU's first three file descriptors.

I'd print a warning here, saying that the behavior has changed and
the '-semihosting-config chardev' must be used now.

> > This quiet
> > change of behavior is unfortunate. I wonder if it would be acceptable
> > to map the '-serial stdio' option in the presence of '-semihosting' to
> > something like '-chardev stdio,id=id1 -semihosting-config chardev=id1'?
>
> I dunno.  I'm wary of having xtensa be unique here.  Alex, thoughts?

Yeah, I thought about it some more and now it doesn't look like a good
idea to me either.

> >> +            switch (regs[4]) {
> >> +            case SELECT_ONE_READ:
> >> +                events = G_IO_IN;
> >> +                break;
> >> +            case SELECT_ONE_WRITE:
> >> +                events = G_IO_OUT;
> >> +                break;
> >> +            case SELECT_ONE_EXCEPT:
> >> +                events = G_IO_PRI;
> >> +                break;
> >> +            default:
> >> +                xtensa_cb(cs, -1, EINVAL);
> >
> > This doesn't match what there used to be: it was possible to call
> > select_one with rq other than SELECT_ONE_* and that would've
> > passed NULL for all fd sets in the select invocation turning it into
> > a sleep. It would return 0 after the timeout.
>
> Hmm.  Is there any documentation of what it was *supposed* to do?  Passing rq 
> ==
> 0xdeadbeef and expecting a specific behaviour seems odd.

I haven't found any documentation for that simcall.
All I can say is that the logic in the code that used to be here is matching
exactly the logic in the code of the xtensa ISS from Cadence/Tensilica.

-- 
Thanks.
-- Max