Re: [RFC PATCH 1/3] target/openrisc: Add basic support for semihosting

[Stafford Horne <shorne@gmail.com>]

On Thu, Jun 02, 2022 at 08:39:21AM -0700, Richard Henderson wrote:
> On 5/27/22 10:27, Stafford Horne wrote:
> > +void do_or1k_semihosting(CPUOpenRISCState *env, uint32_t k);
> ...
> > +DEF_HELPER_FLAGS_2(nop, 0, void, env, i32)
> 
> Just call the helper "semihosting" and be done with it.
> And the helper wants an ifdef for system mode.
> 
> > @@ -10,6 +10,7 @@ openrisc_ss.add(files(
> >     'fpu_helper.c',
> >     'gdbstub.c',
> >     'interrupt_helper.c',
> > +  'openrisc-semi.c',
> >     'sys_helper.c',
> >     'translate.c',
> >   ))
> 
> You want to add the new file for system mode only.
> Or, now that I think of it, conditional on CONFIG_SEMIHOSTING itself.

That's right, I'll update it.

> > +static void or1k_semi_return_u32(CPUOpenRISCState *env, uint32_t ret)
> > +{
> > +    cpu_set_gpr(env, 11, ret);
> > +}
> 
> Let's drop this until you actually use it.  This appears to be attempting to
> mirror other, more complete semihosting, but missing the third "error"
> argument

Sure, I did mention I kept these here for future (real) semihosting support.
But I don't think that will happen.  So I can remove.

> > +void do_or1k_semihosting(CPUOpenRISCState *env, uint32_t k)
> > +{
> > +    uint32_t result;
> > +
> > +    switch (k) {
> > +    case HOSTED_EXIT:
> > +        gdb_exit(cpu_get_gpr(env, 3));
> > +        exit(cpu_get_gpr(env, 3));
> > +    case HOSTED_RESET:
> > +#ifndef CONFIG_USER_ONLY
> > +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > +        return;
> > +#endif
> 
> Do you in fact want to exit to the main loop after asking for reset?
> That's the only way that "no return value" makes sense to me...

OK. I'll look at this more.
 
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "or1k-semihosting: unsupported "
> > +                      "semihosting syscall %d\n", k);
> 
> %u.

OK.

> >   static bool trans_l_nop(DisasContext *dc, arg_l_nop *a)
> >   {
> > +    if (semihosting_enabled() &&
> > +        a->k != 0) {
> > +        gen_helper_nop(cpu_env, tcg_constant_i32(a->k));
> > +    }
> 
> Perhaps cleaner to move the semihosting dispatch switch here, instead of
> leaving it to the runtime?  The reason we have a runtime switch for other
> guests is that the semihosting syscall number is in a register.  This would
> then be
> 
>     if (semihosting_enabled()) {
>         switch (a->k) {
>         case 0:
>             break; /* normal nop */
>         case HOSTED_EXIT:
>             gen_helper_semihost_exit(cpu_R(dc, 3));
>             break;
>         case HOSTED_RESET:
>             gen_helper_semihost_reset();
>             tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
> 
>             dc->base.is_jmp = DISAS_EXIT;
>             break;
>         ...
>         }
>     }

Yeah, that makes sense. I had written it in a way that would allow expanding for
real semi-hosting.  But I don't think we will do that with OpenRISC, so this is
good enough.

I am not sure if you saw the cover letter. I sent this RFC series to help
illustrate two options for providing OpenRISC targets that support poweroff and
reset.

One option being using these NOP's, the second is to create a virt target with
reset/poweroff hardware.

I am kind of leaning towards dropping the semi-hosting patches and only moving
forward with the virt patches.  The reason being that 1. we would not need to
expand the architecture spec to support the qemu virt platform, and we would
need to document the NOP's formally, and 2. OpenRISC doesn't really support the
full "semihosting" facilities for file open/close/write etc.

Any thoughts?  I guess this "semihosting" patch is pretty trivial.  But, maybe
it causes more confusion compared to just going with the virt route.  Also, if
we have virt I can't imagine anyone using the semihosting much.

-Stafford