2018-04-20 8:05 GMT+08:00 Michael Clark <m...@sifive.com>: > > > On Thu, Apr 19, 2018 at 9:28 PM, Zong Li <zong...@gmail.com> wrote: >> >> 2018-04-19 12:43 GMT+08:00 Michael Clark <m...@sifive.com>: >> > Hi Zong, >> > >> >> On 19/04/2018, at 2:40 PM, Zong Li <zong...@gmail.com> wrote: >> >> >> >> Hi all, >> >> >> >> For BBL part, in fp_init at machine/minit.c, >> >> it will clear the D and F bits of misa register, and assertion that >> >> the bits is cleared. >> >> But the misa is WARL register, so there is no effect for writing it, >> >> and the assertion not be true. >> >> So is there has necessary to do that if toolchain not support D and F >> >> extension? >> >> >> >> For QEMU part, when writing misa, it will trigger the illegal >> >> instruction exception, but I think that the WARL allow write behavior? >> > >> > QEMU in the riscv-all branch should have WARL behavior. >> > >> > - https://github.com/riscv/riscv-qemu/commits/riscv-all >> > >> > There is a bug in upstream. We have submitted patches to fix the issue >> > for review on the qemu-devel mailing list. The patch series will be >> > submitted for upstream review again shortly. We were holding off on the >> > series as we didn’t classify it as a “critical bug” as QEMU was in >> > soft-freeze for 2.12 and we weren’t able to get review in time to include >> > this fix in the 2.12 release. >> > >> > See “No traps on writes to misa,minstret,mcycle" >> > >> > - https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream >> > >> > The history is that there were several unimplemented CSRs that had >> > printf followed by exit. Richard Henderson said we should fix this. I >> > changed several CSRs to cause illegal instruction traps instead of calling >> > exit. That was a mistake as CSRs that don’t support write are WARL (Write >> > Any Read Legal). It was certainly better than having the simulation exit as >> > a cpu doesn’t typically have a way to ”exit” like a C program, nevertheless >> > trapping was wrong. My mistake. See here for the history: >> > >> > - >> > https://github.com/riscv/riscv-qemu/blob/ff36f2f77ec3e6a6211c63bfe1707ec057b12f7d/target-riscv/op_helper.c >> > >> > The implementation in the current tree is quite different. We have >> > recently made the CSR system more modular so that with minor changes, >> > custom >> > CPUs will be able to hook their own control and status registers. >> > >> > - >> > https://github.com/riscv/riscv-qemu/blob/qemu-2.13-for-upstream/target/riscv/csr.c#L780-L867 >> > >> > See these changes: >> > >> > - >> > https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a35bd3f8c0ed2e14cc96bbb7 >> > - >> > https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb65fdcf078fb9a8391da1d6b1 >> > >> > We know have a flexible system that will allow implementations to hook >> > per-cpu control and status registers, and we have predicates that make CSRs >> > appear on some processor but not on others. i.e. if misa.S is not present, >> > then S-mode s* CSRs will trap. Sometimes WARL is the correct behaviour, but >> > sometimes trapping is the correct behaviour i.e. if the processor does not >> > implement S-mode. >> > >> > misa traps on write should only affect bootloaders as Supervisor’s like >> > Linux don’t yet have access to the isa register. It’s not a major issuse. >> > >> > Michael. >> >> Hi Michael, >> >> Thanks for the information. The new CSR system is helpful for custom >> CPU such as ours. Thanks. >> >> In the future, maybe we can do something like this in BBL for flexible >> custom platform which has own device to control the timer, ipi and so >> on. >> >> Back to the misa problem in BBL, at fp_init in BBL initial phrase, the >> assertion will has problem because the bits of misa will not be >> cleared. >> >> the code piece like below: >> uintptr_t fd_mask = (1 << ('F' - 'A')) | (1 << ('D' - 'A')); >> clear_csr(misa, fd_mask); >> assert(!(read_csr(misa) & fd_mask)); >> >> I think that the assertion is not necessary even the clear misa. > > > I agree. The specification makes no guarantee that misa writes are not > ignored so it is legal for a processor that supports FD to drop misa writes > and the assertion will trigger on legal RISC-V implementations. That code > piece does not support legal RISC-V implementations that can't disable F and > D. Disabling F and D should not be asserted because it is harmless if an > unused extension is present. > > This assertion will always trigger in QEMU until we support the 'optional' > feature to allow changes to 'misa'. > > Just noting this is not QEMU specifc so we should drop qemu-devel if we > continue to discuss misa on RISC-V in bbl. > > Nevertheless, we do plan to support 'misa' writes however we need to do some > work in translate.c to make sure that cached translations match the current > state of misa. We may want to perform a tb_flush when we implement writable > misa. We also want writable misa to be a CPU feature so we can emulate CPUs > that don't support writable misa. eg add this to the CPU model. > > set_feature(env, RISCV_FEATURE_MISA_WRITABLE) > > Thanks for raising this because the new modular CSR implementation only > implemented 'existential' predicates for CSRs. We should add a write flag to > the predicate. Or we can just return -1 in the write_misa function. e.g. > > static int write_misa(CPURISCVState *env, int csrno, target_ulong val) > { > if (!riscv_feature(env, RISCV_FEATURE_MISA_WRITABLE)) { > return -1; > } > /* validate misa - must contain 'I' or 'E' */ > env->misa = val; > tb_flush(CPU(riscv_env_get_cpu(env))); > } > > tb_flush is pessimistic but conservative. Currently its not common to write > misa so it would be acceptable. > > There is a similar but somewhat more complex issue for disabling misa.C. The > behaviour has been discussed on the isa-dev mailing list. Iirc, we have to > ignore bit 1 in mepc/sepc in MRET/SRET if misa.C has been cleared and a > 2-byte aligned address is present in mepc/sepc, so that MRET/SRET can only > jump to 4-byte aligned code. So we drop bit 1 on writes to mepc/sepc while > misa.C is clear and we ignore bit 1 on reads from mepc/sepc while misa.C is > cleared. So the change needs slightly more work than just making 'misa' > writable. We also have to enforce that 'I' or 'E' are set, and we currently > don't have support for RVE emulation in RISC-V QEMU. This will require > changes to validate registers in translate.c and cause illegal instructions > if regno >= 16 is used. > > I'm also not sure exactly how we add misa to the translation cache index, > but tb_flush seems like the conservative way to ensure the translation cache > matches the currently set bits in misa. > > We also have to audit translate.c to make sure that misa is checked for all > allowable extensions. MAFDC. Currently it only checks 'C' so we will need to > add checks for 'M' in mul/mulw/div/divw/divu/divuw/rem/remw/remu/remuw and > 'A' for amos, 'F' and 'D' in floating point operations, etc. It's a fair > amount of work... > > $ grep -r has_ext target/riscv/ > target/riscv//csr.c: return -!riscv_has_ext(env, RVS); > target/riscv//csr.c: (!riscv_has_ext(env, RVS) && mpp == PRV_S) || > target/riscv//csr.c: (!riscv_has_ext(env, RVU) && mpp == PRV_U)) { > target/riscv//cpu.h:static inline int riscv_has_ext(CPURISCVState *env, > target_ulong ext) > target/riscv//op_helper.c: if (!riscv_has_ext(env, RVC) && (retpc & 0x3)) > { > target/riscv//op_helper.c: if (!riscv_has_ext(env, RVC) && (retpc & 0x3)) > { > target/riscv//translate.c: if (!riscv_has_ext(env, RVC)) { > target/riscv//translate.c: if (!riscv_has_ext(env, RVC)) { > target/riscv//translate.c: if (!riscv_has_ext(env, RVC) && ((ctx->pc + > bimm) & 0x3)) { > target/riscv//translate.c: if (riscv_has_ext(env, RVS)) { > target/riscv//translate.c: if (!riscv_has_ext(env, RVC)) { > > So it seems like writable misa is a fair amount of work > > - RISCV_FEATURE_MISA_WRITABLE (easy) > - ISA extension validation rules in write_misa (easy) > - Extension checks in translate.c (time-consuming but easy) > - RVC instruction pointer alignment checking rules (needs some care) > - Make sure we have CPU models with and without writable 'misa' so we can > test code to handle typical legal processor variants. > > Michael
There are some effort about the CSR can be writable or not, but it looks nice about what you plan to do.
