From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41736) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f9KvK-0004F8-TQ for qemu-devel@nongnu.org; Thu, 19 Apr 2018 21:33:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f9KvJ-0004iV-4E for qemu-devel@nongnu.org; Thu, 19 Apr 2018 21:33:14 -0400 Received: from mail-it0-x229.google.com ([2607:f8b0:4001:c0b::229]:53476) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1f9KvI-0004iD-UU for qemu-devel@nongnu.org; Thu, 19 Apr 2018 21:33:13 -0400 Received: by mail-it0-x229.google.com with SMTP id m134-v6so715889itb.3 for ; Thu, 19 Apr 2018 18:33:12 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <3EFD5845-5CE7-45F5-AE4C-0C86CAFAA200@sifive.com> From: Zong Li Date: Fri, 20 Apr 2018 09:33:11 +0800 Message-ID: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [sw-dev] The problem of write misa on QEMU and BBL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Waterman Cc: Michael Clark , Zong Li , Palmer Dabbelt , RISC-V SW Dev , QEMU Developers , Richard Henderson 2018-04-20 8:11 GMT+08:00 Andrew Waterman : > > > On Thu, Apr 19, 2018 at 5:05 PM, Michael Clark wrote: >> >> >> >> On Thu, Apr 19, 2018 at 9:28 PM, Zong Li wrote: >>> >>> 2018-04-19 12:43 GMT+08:00 Michael Clark : >>> > Hi Zong, >>> > >>> >> On 19/04/2018, at 2:40 PM, Zong Li 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 behavio= r? >>> > >>> > 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 issu= e >>> > for review on the qemu-devel mailing list. The patch series will be >>> > submitted for upstream review again shortly. We were holding off on t= he >>> > series as we didn=E2=80=99t classify it as a =E2=80=9Ccritical bug=E2= =80=9D as QEMU was in >>> > soft-freeze for 2.12 and we weren=E2=80=99t able to get review in tim= e to include >>> > this fix in the 2.12 release. >>> > >>> > See =E2=80=9CNo 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 ca= lling >>> > exit. That was a mistake as CSRs that don=E2=80=99t support write are= WARL (Write >>> > Any Read Legal). It was certainly better than having the simulation e= xit as >>> > a cpu doesn=E2=80=99t typically have a way to =E2=80=9Dexit=E2=80=9D = like a C program, nevertheless >>> > trapping was wrong. My mistake. See here for the history: >>> > >>> > - >>> > https://github.com/riscv/riscv-qemu/blob/ff36f2f77ec3e6a6211c63bfe170= 7ec057b12f7d/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/targe= t/riscv/csr.c#L780-L867 >>> > >>> > See these changes: >>> > >>> > - >>> > https://github.com/riscv/riscv-qemu/commit/9d9c1bfef911c520a35bd3f8c0= ed2e14cc96bbb7 >>> > - >>> > https://github.com/riscv/riscv-qemu/commit/b5a4cd79ce6c7fbb65fdcf078f= b9a8391da1d6b1 >>> > >>> > We know have a flexible system that will allow implementations to hoo= k >>> > per-cpu control and status registers, and we have predicates that mak= e CSRs >>> > appear on some processor but not on others. i.e. if misa.S is not pre= sent, >>> > then S-mode s* CSRs will trap. Sometimes WARL is the correct behaviou= r, but >>> > sometimes trapping is the correct behaviour i.e. if the processor doe= s not >>> > implement S-mode. >>> > >>> > misa traps on write should only affect bootloaders as Supervisor=E2= =80=99s like >>> > Linux don=E2=80=99t yet have access to the isa register. It=E2=80=99s= 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 =3D (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 wri= tes >> and the assertion will trigger on legal RISC-V implementations. That cod= e >> 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. > > > The problem is that BBL cannot cope with this inconsistent scenario. If = pk > is compiled with to assume no floating-point, there had better be no > floating-point. If you remove the assertion, it will break in other ways > later during in execution. > > If you don't want the assertion to fire, compile BBL to match the ISA. It make sense, but the BBL support three scenarios here. 1. Not support floating-point 2. Support floating-point and ISA also supported 3. Support floating-point and ISA not supprted Only the third point is not working for now because the problem we discusse= d. static void fp_init() { if (!supports_extension('D') && !supports_extension('F')) <---- (1) return; assert(read_csr(mstatus) & MSTATUS_FS); #ifdef __riscv_flen <----(2) for (int i =3D 0; i < 32; i++) init_fp_reg(i); write_csr(fcsr, 0); #else <----(3) uintptr_t fd_mask =3D (1 << ('F' - 'A')) | (1 << ('D' - 'A')); clear_csr(misa, fd_mask); assert(!(read_csr(misa) & fd_mask)); #endif } So if we need to match the BBL and ISA, maybe we should remove the code about scenario 3 or just remove the assertion when misa is implemented by ignoring write anything. >> >> This assertion will always trigger in QEMU until we support the 'optiona= l' >> 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 impleme= nt >> 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 mo= del. >> >> 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 fla= g 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 va= l) >> { >> if (!riscv_feature(env, RISCV_FEATURE_MISA_WRITABLE)) { >> return -1; >> } >> /* validate misa - must contain 'I' or 'E' */ >> env->misa =3D 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 h= ave >> 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 on= ly >> jump to 4-byte aligned code. So we drop bit 1 on writes to mepc/sepc whi= le >> 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 curren= tly >> don't have support for RVE emulation in RISC-V QEMU. This will require >> changes to validate registers in translate.c and cause illegal instructi= ons >> if regno >=3D 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 c= ache >> 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' a= nd >> '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 =3D=3D PRV_= S) || >> target/riscv//csr.c: (!riscv_has_ext(env, RVU) && mpp =3D=3D 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 ca= n >> test code to handle typical legal processor variants. >> >> Michael > >