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

[Keith Packard]

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)

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

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.

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

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.

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.

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

-- 
-keith

signature.asc
Description: PGP signature