Re: [PATCH] Semihost SYS_READC implementation (v4)

[Richard Henderson]

On 11/14/19 5:14 PM, Peter Maydell wrote:
> On Fri, 25 Oct 2019 at 20:15, Keith Packard <address@hidden> wrote:
>> There seems to be convergence on a pretty simple interface which uses
>> ebreak surrounded by a couple of specific no-ops:
>>
>>       slli x0, x0, 0x1f
>>       ebreak
>>       srai x0, x0, 0x7
>>
>> There are implementations in rust and openocd, and I've got one for
>> picolibc. The risc-v semihosting code is sitting on a branch in my repo
>> on github:
>>
>>         https://github.com/keith-packard/qemu/tree/riscv-semihost
> 
> I had an idle glance at this implementation, and this:
> 
>    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);
> 
> (where opcode_at() is a wrapper for cpu_ldl_code()) has
> some unfortunate side effects: if the previous instruction
> is in the previous MMU page, or the following instruction
> is in the next MMU page, you might incorrectly trigger
> an exception (where QEMU will just longjmp straight out of
> the cpu_ldl_code()) if that other page isn't actually mapped
> in the guest's page table. You need to be careful not to access
> code outside the page you're actually on unless you're really
> going to execute it and are OK with it faulting.
> 
> I'm not sure what the best way on QEMU to identify this kind
> of multiple-instruction-sequence is -- Richard might have an
> idea. One approach would be to always take an exception
> (or call a helper function) for the 'ebreak', and then
> distinguish semihosting from other kinds of ebreak by doing
> the load of the preceding/succeeding instructions at runtime
> rather than translate time.

It's irritating that this 3 insn sequence is already a thing, baked into other
sim/emulators.  Seems to me that it would have been easier to abscond with

    ebreak + func3 field != 0.

For semi-hosting, it seems even better if the semi-hosting syscall instruction
is not "real", because you're explicitly requesting services from "unreal"
hardware.  It should be specified to generate a SIGILL type of exception
anywhere semi-hosting is not enabled.

That said, it is possible to do this in the translator, by considering this
sequence to be one 12-byte instruction.

You'd build in a recognizer into the decoder such that, when semi-hosting is
enabled, seeing the entry nop starts looking ahead:

  - If this is not the first insn in the TB, and there are
    not 8 more bytes remaining on the page, terminate the
    current TB.  This will re-start the machine in the next TB.

  - If the complete sequence is not found, then advance the pc
    past the entry nop and let nature take its course wrt the
    subsequent instructions.

  - If the sequence crosses a page, then so be it.  Because of
    step 1, this only happens when we *must* cross a page, and
    will have recognized any paging exception anyway.
    The generic parts of qemu will handle proper invalidation of
    a TB that crosses a page boundary.

  - This is kinda similar to the target/sh4 code to handle
    generic user-space atomic sequences: c.f. decode_gusa().

  - This implementation does mean that you can't jump directly
    to the ebreak insn and have the sequence be recognized.
    Control must flow through the entry nop.
    This is a bug or a feature depending on the reviewer.  ;-)

With that in mind, it may be simpler to handle all of this not in the
translator, but in the function that delivers the ebreak exception.  At that
point one can arrange to read memory without raising additional exceptions.


r~