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

[Michael Clark]

On Fri, Apr 20, 2018 at 12:12 PM, Andrew Waterman <address@hidden> wrote:

>
>
> On Thu, Apr 19, 2018 at 5:11 PM, Michael Clark <address@hidden> wrote:
>
>>
>>
>> On Fri, Apr 20, 2018 at 12:05 PM, Michael Clark <address@hidden> wrote:
>>
>>>
>>>
>>> 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/ff36f2f77ec3e6a6211
>>>> c63bfe1707ec057b12f7d/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-upstr
>>>> eam/target/riscv/csr.c#L780-L867
>>>> >
>>>> > See these changes:
>>>> >
>>>> > - https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a
>>>> 35bd3f8c0ed2e14cc96bbb7
>>>> > - https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb6
>>>> 5fdcf078fb9a8391da1d6b1
>>>> >
>>>> > 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.
>>>
>>
>> - Check regno >= 16 && riscv_has_ext(env, RVE) then illegal instruction
>> trap (time-consuming - needs care to not miss any register)
>>
>> Are F & D legal with E? 16 register FPU? I guess not. We may need to mask
>> and drop some extension writes in the validate misa logic (WARL).
>>
>
> No, the manual forbids this case.  E should preclude FD.
>

Good to know.

In any case it seems we need some pretty major changes to translate.c
before we can make misa writable in qemu-riscv. Almost every gen routine
with the exception of RVI will need predication based on extensions. It
makes one pause and think whether adding if statements is a good approach
or whether having exension metadata available in the decoder so that it can
be done generically. Adding lots of riscv_has_ext checks would be nasty.

We will need to add misa to DisasContext so that we can
remove CPURISCVState *env from gen methods.

It also doesn't make sense to start this until we have merged Emilio's
DisasContextBase changes.

It would be nice if Emilio's changes can be merged early in the 2.13 cycle
so that folk are able to make progress on extension checking to
target/riscv/translate.c

I'll be happy if we can nuke CPURISCVState *env and perform the misa checks
on DisasContext. CSR instructions end translation blocks so its safe to
read the 'misa' state at the start of a block.

Michael