From ede97384f6f760a1a2c8b15f329e3e7c574a0301 Mon Sep 17 00:00:00 2001 From: Scott Johnson Date: Fri, 24 Sep 2021 18:43:42 -0700 Subject: [PATCH 1/4] Fix logging of FCSR and VCSR It looks like `csrw fcsr` was intended to log writes to FFLAGS, FRM, and FCSR, but only logged FCSR (because LOG_CSR() always used `which` to record which CSR was written). This changes it to log writes to FRM and FFLAGS, the two registers which compose FCSR. There's no need to log FCSR since that is already covered by the other two. The logging of `csrw vcsr` was intended to log writes to VXSAT and VXRM but instead would report a write to VCSR that only showed the contents of VXRM. --- riscv/processor.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/riscv/processor.cc b/riscv/processor.cc index fcbc6676..b87d276b 100644 --- a/riscv/processor.cc +++ b/riscv/processor.cc @@ -960,7 +960,7 @@ void processor_t::set_csr(int which, reg_t val) { #if defined(RISCV_ENABLE_COMMITLOG) #define LOG_CSR(rd) \ - STATE.log_reg_write[((which) << 4) | 4] = {get_csr(rd), 0}; + STATE.log_reg_write[((rd) << 4) | 4] = {get_csr(rd), 0}; #else #define LOG_CSR(rd) #endif @@ -1021,7 +1021,6 @@ void processor_t::set_csr(int which, reg_t val) case CSR_FCSR: LOG_CSR(CSR_FFLAGS); LOG_CSR(CSR_FRM); - LOG_CSR(CSR_FCSR); break; case CSR_VCSR: LOG_CSR(CSR_VXSAT); From 2b26a3cdf1cf391893a0a1a31815ac59d96ff05c Mon Sep 17 00:00:00 2001 From: Scott Johnson Date: Fri, 24 Sep 2021 22:02:14 -0700 Subject: [PATCH 2/4] Convert frm & fflags to csr_t Adds proper logging of fflags on FP arithmetic ops. --- riscv/csrs.cc | 16 ++++++++++++++++ riscv/csrs.h | 9 +++++++++ riscv/decode.h | 12 ++++++------ riscv/insns/vfcvt_x_f_v.h | 6 +++--- riscv/insns/vfcvt_xu_f_v.h | 6 +++--- riscv/insns/vfmv_f_s.h | 2 +- riscv/insns/vfmv_s_f.h | 2 +- riscv/insns/vfncvt_x_f_w.h | 6 +++--- riscv/insns/vfncvt_xu_f_w.h | 6 +++--- riscv/insns/vfwcvt_x_f_v.h | 4 ++-- riscv/insns/vfwcvt_xu_f_v.h | 4 ++-- riscv/processor.cc | 38 +++++-------------------------------- riscv/processor.h | 4 ++-- 13 files changed, 56 insertions(+), 59 deletions(-) diff --git a/riscv/csrs.cc b/riscv/csrs.cc index 5060f482..461d4d12 100644 --- a/riscv/csrs.cc +++ b/riscv/csrs.cc @@ -1116,3 +1116,19 @@ void dcsr_csr_t::write_cause_and_prv(uint8_t cause, reg_t prv) noexcept { this->prv = prv; log_write(); } + + +float_csr_t::float_csr_t(processor_t* const proc, const reg_t addr, const reg_t mask, const reg_t init): + masked_csr_t(proc, addr, mask, init) { +} + +void float_csr_t::verify_permissions(insn_t insn, bool write) const { + require_fp; + if (!proc->extension_enabled('F')) + throw trap_illegal_instruction(insn.bits()); +} + +bool float_csr_t::unlogged_write(const reg_t val) noexcept { + dirty_fp_state; + return masked_csr_t::unlogged_write(val); +} diff --git a/riscv/csrs.h b/riscv/csrs.h index fd207828..cf726a2b 100644 --- a/riscv/csrs.h +++ b/riscv/csrs.h @@ -577,4 +577,13 @@ class dcsr_csr_t: public csr_t { typedef std::shared_ptr dcsr_csr_t_p; + +class float_csr_t: public masked_csr_t { + public: + float_csr_t(processor_t* const proc, const reg_t addr, const reg_t mask, const reg_t init); + virtual void verify_permissions(insn_t insn, bool write) const override; + protected: + virtual bool unlogged_write(const reg_t val) noexcept override; +}; + #endif diff --git a/riscv/decode.h b/riscv/decode.h index a54f1f09..2429ae7d 100644 --- a/riscv/decode.h +++ b/riscv/decode.h @@ -234,7 +234,7 @@ private: #define BRANCH_TARGET (pc + insn.sb_imm()) #define JUMP_TARGET (pc + insn.uj_imm()) #define RM ({ int rm = insn.rm(); \ - if(rm == 7) rm = STATE.frm; \ + if(rm == 7) rm = STATE.frm->read(); \ if(rm > 4) throw trap_illegal_instruction(insn.bits()); \ rm; }) @@ -281,7 +281,7 @@ private: #define set_fp_exceptions ({ if (softfloat_exceptionFlags) { \ dirty_fp_state; \ - STATE.fflags |= softfloat_exceptionFlags; \ + STATE.fflags->write(STATE.fflags->read() | softfloat_exceptionFlags); \ } \ softfloat_exceptionFlags = 0; }) @@ -1848,12 +1848,12 @@ for (reg_t i = 0; i < P.VU.vlmax && P.VU.vl != 0; ++i) { \ (P.VU.vsew == e32 && p->extension_enabled('F')) || \ (P.VU.vsew == e64 && p->extension_enabled('D'))); \ require_vector(true);\ - require(STATE.frm < 0x5);\ + require(STATE.frm->read() < 0x5);\ reg_t vl = P.VU.vl; \ reg_t rd_num = insn.rd(); \ reg_t rs1_num = insn.rs1(); \ reg_t rs2_num = insn.rs2(); \ - softfloat_roundingMode = STATE.frm; + softfloat_roundingMode = STATE.frm->read(); #define VI_VFP_LOOP_BASE \ VI_VFP_COMMON \ @@ -2264,12 +2264,12 @@ for (reg_t i = 0; i < P.VU.vlmax && P.VU.vl != 0; ++i) { \ require((P.VU.vsew == e8 && p->extension_enabled(EXT_ZFH)) || \ (P.VU.vsew == e16 && p->extension_enabled('F')) || \ (P.VU.vsew == e32 && p->extension_enabled('D'))); \ - require(STATE.frm < 0x5);\ + require(STATE.frm->read() < 0x5);\ reg_t vl = P.VU.vl; \ reg_t rd_num = insn.rd(); \ reg_t rs1_num = insn.rs1(); \ reg_t rs2_num = insn.rs2(); \ - softfloat_roundingMode = STATE.frm; \ + softfloat_roundingMode = STATE.frm->read(); \ for (reg_t i=P.VU.vstart; i(rd_num, i) = f16_to_i16(vs2, STATE.frm, true); + P.VU.elt(rd_num, i) = f16_to_i16(vs2, STATE.frm->read(), true); }, { - P.VU.elt(rd_num, i) = f32_to_i32(vs2, STATE.frm, true); + P.VU.elt(rd_num, i) = f32_to_i32(vs2, STATE.frm->read(), true); }, { - P.VU.elt(rd_num, i) = f64_to_i64(vs2, STATE.frm, true); + P.VU.elt(rd_num, i) = f64_to_i64(vs2, STATE.frm->read(), true); }) diff --git a/riscv/insns/vfcvt_xu_f_v.h b/riscv/insns/vfcvt_xu_f_v.h index 725cbda2..51c00ca9 100644 --- a/riscv/insns/vfcvt_xu_f_v.h +++ b/riscv/insns/vfcvt_xu_f_v.h @@ -1,11 +1,11 @@ // vfcvt.xu.f.v vd, vd2, vm VI_VFP_VV_LOOP ({ - P.VU.elt(rd_num, i) = f16_to_ui16(vs2, STATE.frm, true); + P.VU.elt(rd_num, i) = f16_to_ui16(vs2, STATE.frm->read(), true); }, { - P.VU.elt(rd_num, i) = f32_to_ui32(vs2, STATE.frm, true); + P.VU.elt(rd_num, i) = f32_to_ui32(vs2, STATE.frm->read(), true); }, { - P.VU.elt(rd_num, i) = f64_to_ui64(vs2, STATE.frm, true); + P.VU.elt(rd_num, i) = f64_to_ui64(vs2, STATE.frm->read(), true); }) diff --git a/riscv/insns/vfmv_f_s.h b/riscv/insns/vfmv_f_s.h index 3309e471..06d93b22 100644 --- a/riscv/insns/vfmv_f_s.h +++ b/riscv/insns/vfmv_f_s.h @@ -4,7 +4,7 @@ require_fp; require((P.VU.vsew == e16 && p->extension_enabled(EXT_ZFH)) || (P.VU.vsew == e32 && p->extension_enabled('F')) || (P.VU.vsew == e64 && p->extension_enabled('D'))); -require(STATE.frm < 0x5); +require(STATE.frm->read() < 0x5); reg_t rs2_num = insn.rs2(); uint64_t vs2_0 = 0; diff --git a/riscv/insns/vfmv_s_f.h b/riscv/insns/vfmv_s_f.h index d12fd916..4e7f82e8 100644 --- a/riscv/insns/vfmv_s_f.h +++ b/riscv/insns/vfmv_s_f.h @@ -4,7 +4,7 @@ require_fp; require((P.VU.vsew == e16 && p->extension_enabled(EXT_ZFH)) || (P.VU.vsew == e32 && p->extension_enabled('F')) || (P.VU.vsew == e64 && p->extension_enabled('D'))); -require(STATE.frm < 0x5); +require(STATE.frm->read() < 0x5); reg_t vl = P.VU.vl; diff --git a/riscv/insns/vfncvt_x_f_w.h b/riscv/insns/vfncvt_x_f_w.h index a342e3fd..a8a6dfb1 100644 --- a/riscv/insns/vfncvt_x_f_w.h +++ b/riscv/insns/vfncvt_x_f_w.h @@ -2,15 +2,15 @@ VI_VFP_CVT_SCALE ({ auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f16_to_i8(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f16_to_i8(vs2, STATE.frm->read(), true); }, { auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f32_to_i16(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f32_to_i16(vs2, STATE.frm->read(), true); }, { auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f64_to_i32(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f64_to_i32(vs2, STATE.frm->read(), true); }, { require(p->extension_enabled(EXT_ZFH)); diff --git a/riscv/insns/vfncvt_xu_f_w.h b/riscv/insns/vfncvt_xu_f_w.h index 6fda694a..bff733e3 100644 --- a/riscv/insns/vfncvt_xu_f_w.h +++ b/riscv/insns/vfncvt_xu_f_w.h @@ -2,15 +2,15 @@ VI_VFP_CVT_SCALE ({ auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f16_to_ui8(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f16_to_ui8(vs2, STATE.frm->read(), true); }, { auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f32_to_ui16(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f32_to_ui16(vs2, STATE.frm->read(), true); }, { auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f64_to_ui32(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f64_to_ui32(vs2, STATE.frm->read(), true); }, { require(p->extension_enabled(EXT_ZFH)); diff --git a/riscv/insns/vfwcvt_x_f_v.h b/riscv/insns/vfwcvt_x_f_v.h index 60ebbcab..5e0c064d 100644 --- a/riscv/insns/vfwcvt_x_f_v.h +++ b/riscv/insns/vfwcvt_x_f_v.h @@ -5,11 +5,11 @@ VI_VFP_CVT_SCALE }, { auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f16_to_i32(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f16_to_i32(vs2, STATE.frm->read(), true); }, { auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f32_to_i64(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f32_to_i64(vs2, STATE.frm->read(), true); }, { ; diff --git a/riscv/insns/vfwcvt_xu_f_v.h b/riscv/insns/vfwcvt_xu_f_v.h index a6a05912..f3243c8f 100644 --- a/riscv/insns/vfwcvt_xu_f_v.h +++ b/riscv/insns/vfwcvt_xu_f_v.h @@ -5,11 +5,11 @@ VI_VFP_CVT_SCALE }, { auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f16_to_ui32(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f16_to_ui32(vs2, STATE.frm->read(), true); }, { auto vs2 = P.VU.elt(rs2_num, i); - P.VU.elt(rd_num, i, true) = f32_to_ui64(vs2, STATE.frm, true); + P.VU.elt(rd_num, i, true) = f32_to_ui64(vs2, STATE.frm->read(), true); }, { ; diff --git a/riscv/processor.cc b/riscv/processor.cc index b87d276b..4727d0c5 100644 --- a/riscv/processor.cc +++ b/riscv/processor.cc @@ -521,8 +521,8 @@ void state_t::reset(processor_t* const proc, reg_t max_isa) csrmap[addr] = std::make_shared(proc, addr); } - fflags = 0; - frm = 0; + csrmap[CSR_FFLAGS] = fflags = std::make_shared(proc, CSR_FFLAGS, FSR_AEXC >> FSR_AEXC_SHIFT, 0); + csrmap[CSR_FRM] = frm = std::make_shared(proc, CSR_FRM, FSR_RD >> FSR_RD_SHIFT, 0); serialized = false; #ifdef RISCV_ENABLE_COMMITLOG @@ -977,18 +977,10 @@ void processor_t::set_csr(int which, reg_t val) case CSR_SENTROPY: es.set_sentropy(val); break; - case CSR_FFLAGS: - dirty_fp_state; - state.fflags = val & (FSR_AEXC >> FSR_AEXC_SHIFT); - break; - case CSR_FRM: - dirty_fp_state; - state.frm = val & (FSR_RD >> FSR_RD_SHIFT); - break; case CSR_FCSR: dirty_fp_state; - state.fflags = (val & FSR_AEXC) >> FSR_AEXC_SHIFT; - state.frm = (val & FSR_RD) >> FSR_RD_SHIFT; + state.fflags->write((val & FSR_AEXC) >> FSR_AEXC_SHIFT); + state.frm->write((val & FSR_RD) >> FSR_RD_SHIFT); break; case CSR_VCSR: dirty_vs_state; @@ -1012,16 +1004,6 @@ void processor_t::set_csr(int which, reg_t val) #if defined(RISCV_ENABLE_COMMITLOG) switch (which) { - case CSR_FFLAGS: - LOG_CSR(CSR_FFLAGS); - break; - case CSR_FRM: - LOG_CSR(CSR_FRM); - break; - case CSR_FCSR: - LOG_CSR(CSR_FFLAGS); - LOG_CSR(CSR_FRM); - break; case CSR_VCSR: LOG_CSR(CSR_VXSAT); LOG_CSR(CSR_VXRM); @@ -1071,21 +1053,11 @@ reg_t processor_t::get_csr(int which, insn_t insn, bool write, bool peek) if (!write) break; ret(es.get_sentropy()); - case CSR_FFLAGS: - require_fp; - if (!extension_enabled('F')) - break; - ret(state.fflags); - case CSR_FRM: - require_fp; - if (!extension_enabled('F')) - break; - ret(state.frm); case CSR_FCSR: require_fp; if (!extension_enabled('F')) break; - ret((state.fflags << FSR_AEXC_SHIFT) | (state.frm << FSR_RD_SHIFT)); + ret((state.fflags->read() << FSR_AEXC_SHIFT) | (state.frm->read() << FSR_RD_SHIFT)); case CSR_VCSR: require_vector_vs; if (!extension_enabled('V')) diff --git a/riscv/processor.h b/riscv/processor.h index a9b75fd1..fd16812f 100644 --- a/riscv/processor.h +++ b/riscv/processor.h @@ -199,8 +199,8 @@ struct state_t static const int max_pmp = 16; pmpaddr_csr_t_p pmpaddr[max_pmp]; - uint32_t fflags; - uint32_t frm; + csr_t_p fflags; + csr_t_p frm; bool serialized; // whether timer CSRs are in a well-defined state // When true, execute a single instruction and then enter debug mode. This From bb09cd92b2074f87802c58a67c190c1ea82abbfb Mon Sep 17 00:00:00 2001 From: Scott Johnson Date: Sat, 25 Sep 2021 09:39:51 -0700 Subject: [PATCH 3/4] Remove unnecessary double-setting of mstatus.FS=Dirty fflags->write() already sets that. --- riscv/decode.h | 1 - riscv/processor.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/riscv/decode.h b/riscv/decode.h index 2429ae7d..eb061017 100644 --- a/riscv/decode.h +++ b/riscv/decode.h @@ -280,7 +280,6 @@ private: #define require_vm do { if (insn.v_vm() == 0) require(insn.rd() != 0);} while(0); #define set_fp_exceptions ({ if (softfloat_exceptionFlags) { \ - dirty_fp_state; \ STATE.fflags->write(STATE.fflags->read() | softfloat_exceptionFlags); \ } \ softfloat_exceptionFlags = 0; }) diff --git a/riscv/processor.cc b/riscv/processor.cc index 4727d0c5..c049869b 100644 --- a/riscv/processor.cc +++ b/riscv/processor.cc @@ -978,7 +978,6 @@ void processor_t::set_csr(int which, reg_t val) es.set_sentropy(val); break; case CSR_FCSR: - dirty_fp_state; state.fflags->write((val & FSR_AEXC) >> FSR_AEXC_SHIFT); state.frm->write((val & FSR_RD) >> FSR_RD_SHIFT); break; From 7fecd35cda28bb7ee09730e7c839dd4496b61fb7 Mon Sep 17 00:00:00 2001 From: Scott Johnson Date: Fri, 24 Sep 2021 22:24:27 -0700 Subject: [PATCH 4/4] Convert FCSR to csr_t --- riscv/csrs.cc | 24 ++++++++++++++++++++++++ riscv/csrs.h | 17 +++++++++++++++++ riscv/processor.cc | 11 ++--------- 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/riscv/csrs.cc b/riscv/csrs.cc index 461d4d12..5a9dbe4a 100644 --- a/riscv/csrs.cc +++ b/riscv/csrs.cc @@ -1132,3 +1132,27 @@ bool float_csr_t::unlogged_write(const reg_t val) noexcept { dirty_fp_state; return masked_csr_t::unlogged_write(val); } + + +composite_csr_t::composite_csr_t(processor_t* const proc, const reg_t addr, csr_t_p upper_csr, csr_t_p lower_csr, const unsigned upper_lsb): + csr_t(proc, addr), + upper_csr(upper_csr), + lower_csr(lower_csr), + upper_lsb(upper_lsb) { +} + +void composite_csr_t::verify_permissions(insn_t insn, bool write) const { + // It is reasonable to assume that either underlying CSR will have + // the same permissions as this composite. + upper_csr->verify_permissions(insn, write); +} + +reg_t composite_csr_t::read() const noexcept { + return (upper_csr->read() << upper_lsb) | lower_csr->read(); +} + +bool composite_csr_t::unlogged_write(const reg_t val) noexcept { + upper_csr->write(val >> upper_lsb); + lower_csr->write(val); + return false; // logging is done only by the underlying CSRs +} diff --git a/riscv/csrs.h b/riscv/csrs.h index cf726a2b..482064dd 100644 --- a/riscv/csrs.h +++ b/riscv/csrs.h @@ -586,4 +586,21 @@ class float_csr_t: public masked_csr_t { virtual bool unlogged_write(const reg_t val) noexcept override; }; + +// For a CSR like FCSR, that is actually a view into multiple +// underlying registers. +class composite_csr_t: public csr_t { + public: + // We assume the lower_csr maps to bit 0. + composite_csr_t(processor_t* const proc, const reg_t addr, csr_t_p upper_csr, csr_t_p lower_csr, const unsigned upper_lsb); + virtual void verify_permissions(insn_t insn, bool write) const override; + virtual reg_t read() const noexcept override; + protected: + virtual bool unlogged_write(const reg_t val) noexcept override; + private: + csr_t_p upper_csr; + csr_t_p lower_csr; + const unsigned upper_lsb; +}; + #endif diff --git a/riscv/processor.cc b/riscv/processor.cc index c049869b..2c8827f4 100644 --- a/riscv/processor.cc +++ b/riscv/processor.cc @@ -523,6 +523,8 @@ void state_t::reset(processor_t* const proc, reg_t max_isa) csrmap[CSR_FFLAGS] = fflags = std::make_shared(proc, CSR_FFLAGS, FSR_AEXC >> FSR_AEXC_SHIFT, 0); csrmap[CSR_FRM] = frm = std::make_shared(proc, CSR_FRM, FSR_RD >> FSR_RD_SHIFT, 0); + assert(FSR_AEXC_SHIFT == 0); // composite_csr_t assumes fflags begins at bit 0 + csrmap[CSR_FCSR] = std::make_shared(proc, CSR_FFLAGS, frm, fflags, FSR_RD_SHIFT); serialized = false; #ifdef RISCV_ENABLE_COMMITLOG @@ -977,10 +979,6 @@ void processor_t::set_csr(int which, reg_t val) case CSR_SENTROPY: es.set_sentropy(val); break; - case CSR_FCSR: - state.fflags->write((val & FSR_AEXC) >> FSR_AEXC_SHIFT); - state.frm->write((val & FSR_RD) >> FSR_RD_SHIFT); - break; case CSR_VCSR: dirty_vs_state; VU.vxsat = (val & VCSR_VXSAT) >> VCSR_VXSAT_SHIFT; @@ -1052,11 +1050,6 @@ reg_t processor_t::get_csr(int which, insn_t insn, bool write, bool peek) if (!write) break; ret(es.get_sentropy()); - case CSR_FCSR: - require_fp; - if (!extension_enabled('F')) - break; - ret((state.fflags->read() << FSR_AEXC_SHIFT) | (state.frm->read() << FSR_RD_SHIFT)); case CSR_VCSR: require_vector_vs; if (!extension_enabled('V'))