Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL

[Michael Clark]

On Thu, Apr 19, 2018 at 9:28 PM, Zong Li <address@hidden> wrote:

> 2018-04-19 12:43 GMT+08:00 Michael Clark <address@hidden>:
> > Hi Zong,
> >
> >> On 19/04/2018, at 2:40 PM, Zong Li <address@hidden> 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