On Fri, Oct 23, 2020 at 10:56 PM Keith Packard <kei...@keithp.com> wrote: > > Alistair Francis <alistai...@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 <kei...@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)
That would be helpful, we are always short on reviewers. > > >> +#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. It's not fully internal though. Someone running with the `-d int` command line argument will see these exceptions, which don't correspond to anything in the spec. Is there some way we could at least convey that information to users? > > 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. I think it would at least be better to use a high reserved number, but like I mentioned above this is somewhat user visible. > > >> + * 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. That makes sense. If they start to diverge we can also re-split them out though. > > 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. AFAIK that unfortunately never progressed too far. > > 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. That would be helpful. When doing that make sure to split the patches up (this one is already a little big) so that the ARM people can just review 1 patch. > > >> --- 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 I saw that, that is good news. > 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. I don't have an idea off the top of my head, hopefully Richard already knows the answer here :) Alistair > > -- > -keith