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

[Keith Packard]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 10/23/20 2:49 PM, Keith Packard via wrote:
>>  static bool trans_ebreak(DisasContext *ctx, arg_ebreak *a)
>>  {
>> -    generate_exception(ctx, RISCV_EXCP_BREAKPOINT);
>> +    uint32_t pre    = opcode_at(&ctx->base, ctx->base.pc_next - 4);
>> +    uint32_t ebreak = opcode_at(&ctx->base, ctx->base.pc_next);
>> +    uint32_t post   = opcode_at(&ctx->base, ctx->base.pc_next + 4);
>
> Alistair asked if this approach is ok.  I think it is.  There are other places
> in which we scan forward (usually only forward, not backward, but this is a
> special nop, so it doesn't matter).
>
> However:
>
> (1) No need to re-read the current ebreak insn.  That is how we arrived here,
> after all.

The semihosting spec requires that we land here using a 32-bit ebreak
instruction, not the compressed version, so I think we still need to
check for this.

> (2) You need to check for page boundaries before reading pre and post.
> Otherwise you could wind up with SIGSEGV (or the equivalent internal qemu
> exception) when you shouldn't.

Right, in that case the ebreak instruction should just raise a regular
break point exception. I've added a check to make sure the address of
the three functions match when and-ed with TARGET_PAGE_MASK.

I've finished re-working this patch into a series which first modifies
the ARM semihosting support to be architecture-independent, then adds
the changes necessary to support RISC-V. Expect to see that series on
the list shortly.

-- 
-keith

signature.asc
Description: PGP signature