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

For OpenRISC we currently only use semihosting for system exit and
reset.  This patch implements that.

The implementation uses a helper to delegate to the semihosting
facility.  The helper is marked as having side effects but currently
does not have any.  I have defined it like this as our other
unimplemented semihosting calls will have side effects and return
results in register r11.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 configs/devices/or1k-softmmu/default.mak |  2 +
 qemu-options.hx                          | 16 ++++---
 target/openrisc/cpu.h                    |  2 +
 target/openrisc/helper.h                 |  1 +
 target/openrisc/meson.build              |  1 +
 target/openrisc/openrisc-semi.c          | 54 ++++++++++++++++++++++++
 target/openrisc/sys_helper.c             |  5 +++
 target/openrisc/translate.c              |  6 +++
 8 files changed, 82 insertions(+), 5 deletions(-)
 create mode 100644 target/openrisc/openrisc-semi.c

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.

> +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.


> +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...

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "or1k-semihosting: unsupported "
> +                      "semihosting syscall %d\n", k);

%u.

>   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;
         ...
         }
     }


r~

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

On 6/4/22 17:57, Stafford Horne wrote:
> 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.

I agree that "virt" would to more for openrisc devel than these nops.

> Also, if we have virt I can't imagine anyone using the semihosting much.

IMO, semihosting is most valuable for writing regression tests and not much more.  (You 
have no control over the exit status of qemu with normal shutdown, as compared with 
semihosting exit.)


r~