Re: [PATCH] riscv: Add semihosting support [v8]

[Alistair Francis]

On Fri, Oct 23, 2020 at 10:56 PM Keith Packard <keithp@keithp.com> wrote:
>
> Alistair Francis <alistair23@gmail.com> writes:
>
> Thanks much for taking time to review this patch in detail. I've left
> the indicated changes in a new version of my riscv-semihost branch here:
>
>         https://github.com/keith-packard/qemu/tree/riscv-semihost
>
> I'll post a new version once we've wound up discussion on the remaining
> issues.
>
> >> +M: Keith Packard <keithp@keithp.com>
> >
> > I don't think you should be a maintainer just yet. In general people
> > have to be actively reviewing patches to be listed as a maintainer.
>
> Cool, I'm glad to not be listed. checkpatch.pl suggested that I might
> need to add something here, so I went ahead and included it in case it
> was necessary. (I probably should do some patch review though; SiFive is
> rather dependent on QEMU continuing to be a great RISC-V emulator)

That would be helpful, we are always short on reviewers.

>
> >> +#include "exec/cpu-all.h"
> >
> > This isn't used in the header so it shouldn't be here.
>
> Worse than that -- it's already included in this file. I suspect
> this is left over from a previous version and have removed it.
>
> >> +#define RISCV_EXCP_SEMIHOST                      0x10
> >
> > I don't see this in the RISC-V spec, it seems to just be reserved, not
> > for semihosting.
>
> Hrm. It's entirely an internal implementation detail in QEMU and matches
> how semihosting works in the ARM implementation -- the presence of the
> semihosting breakpoint raises this exception which is then handled in
> the usual exception processing path.

It's not fully internal though. Someone running with the `-d int`
command line argument will see these exceptions, which don't
correspond to anything in the spec.

Is there some way we could at least convey that information to users?

>
> If there is ever a real exception that uses this number, we can
> re-define this to something else. Or if you have a favorite number you'd
> like to use instead, that'd be great.

I think it would at least be better to use a high reserved number, but
like I mentioned above this is somewhat user visible.

>
> >> + *  ARM Semihosting is documented in:
> >> + *     Semihosting for AArch32 and AArch64 Release 2.0
> >> + *     https://static.docs.arm.com/100863/0200/semihosting.pdf
> >
> > Maybe just point to the RISC-V doc instead.
>
> Good suggestion. Fixed.
>
> > Could we split all of the shared code out somewhere?
>
> Yes, that seems like a reasonable suggestion. I haven't done so because
> that brings a lot of additional obligations on the patch to not impact
> the ARM implementation, and means that future changes to either the
> RISC-V or ARM specifications would need to be careful to not impact the
> other architecture as the code is modified.

That makes sense. If they start to diverge we can also re-split them out though.

>
> Benjamin Herrenschmidt started a thread back in January about creating a
> common semihosting implementation to be shared across ARM, RISC-V and
> PPC. I'm not sure he ever published the resulting code, but we can
> probably get whatever he's done and see if we want to go that way. I
> suspect the biggest impact will be to the ARM maintainers who will end
> up on the hook for reviewing the code to make sure it doesn't break
> anything for them.

AFAIK that unfortunately never progressed too far.

>
> I can expand the semihost testing which picolibc currently performs
> under QEMU on ARM, AARCH64 and RISC-V; that might help catch regressions
> caused by this rework.

That would be helpful.

When doing that make sure to split the patches up (this one is already
a little big) so that the ARM people can just review 1 patch.

>
> >> --- a/target/riscv/translate.c
> >> +++ b/target/riscv/translate.c
> >> @@ -63,6 +63,7 @@ typedef struct DisasContext {
> >>      uint16_t vlen;
> >>      uint16_t mlen;
> >>      bool vl_eq_vlmax;
> >> +    CPUState *cs;
> >
> > I'm not sure we should do this.
>
> Yeah, the RISC-V semihosting requirement that three instructions be
> compared to determine a valid 'sequence' is the least pleasing part of
> the specification. This is the second version of this particular piece
> of code.
>
> We also changed the semihosting specification to require that all three
> instructions lie on the same page to make sure they are all available if
> any are available. In the application implementation, all that was

I saw that, that is good news.

> required to meet that was to put the sequence in a function and align
> that to a 16-byte boundary as the function consists of four 32-bit
> instructions:
>
>         .global sys_semihost
>         .balign 16
>         .option push
>         .option norvc
> sys_semihost:
>         slli zero, zero, 0x1f
>         ebreak
>         srai zero, zero, 0x7
>         ret
>         .option pop
>
> >> +static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
> >> +{
> >> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> >> +    CPUState *cpu = ctx->cs;
> >> +    CPURISCVState *env = cpu->env_ptr;
> >> +
> >> +    return cpu_ldl_code(env, pc);
> >
> > @Richard Henderson is this ok?
>
> Let me know if you've got a better plan, or even some suggestions on how
> it might be improved as it seems like it a layering violation to me.

I don't have an idea off the top of my head, hopefully Richard already
knows the answer here :)

Alistair

>
> --
> -keith