Re: [PATCH] Semihost SYS_READC implementation (v4)

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 14 Nov 2019 at 18:05, Keith Packard <keithp@keithp.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > 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 can't even find the implementation of cpu_ldl_code; the qemu source
> code is somewhat obscure in this area. But, longjmp'ing out of the
> middle of that seems like a bad idea.

It's the way QEMU works -- generally load/store operations
that work on virtual addresses are expected to only return
in the success case; on failure they longjmp out to cause
the guest exception. (Load/stores on physical addresses
generally return a memory transaction status for the caller
to check and handle.) I agree that within the translation code
it's a bit weird and it might be nicer for the translate.c
code to explicitly handle failures to load an insn, but it
would be a bit of an upheaval to try to rewrite it at this point.

cpu_ldl_code() is provided by include/exec/cpu_ldst.h, incidentally
(via preprocessor macros and repeated inclusion of some template
.h files, which is why a grep for the function name misses it).

> > Does your semihosting spec expect to have the semihosting
> > call work if the sequence crosses a page boundary, the
> > code is being executed by a userspace process, and one of
> > the two pages has been paged out by the OS ?
>
> You've seen the entirety of the RISC-V semihosting spec already.  For
> now, perhaps we should limit RISC-V semihosting support to devices
> without paging support and await a more complete spec.
>
> As you suggest, disallowing the sequence from crossing a page boundary
> would be a simple fix, but that would require wording changes to the
> spec.

Yeah, I'm implicitly suggesting that a bit more thought/revision
of the spec might not be a bad idea. Things that are
effectively supposed to be treated like a single instruction
but which can cross cacheline or page boundaries turn out
to be a fertile source of implementation bugs. (The arm
translate.c code has to be quite careful about handling
32-bit Thumb insns that cross pages. The x86 translate.c
code is less careful and may well be buggy in this area.)

thanks
-- PMM