On Thu, 14 Nov 2019 at 18:05, Keith Packard <kei...@keithp.com> wrote: > > Peter Maydell <peter.mayd...@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