openhwgroup / cve2

The CORE-V CVE2 is a small 32 bit RISC-V CPU core (RV32IMC/EMC) with a two stage pipeline, based on the original zero-riscy work from ETH Zurich and Ibex work from lowRISC.
https://docs.openhwgroup.org/projects/cve2-user-manual/en/latest/
Apache License 2.0
26 stars 19 forks source link

RVFI depends on the testbench #271

Open MikeOpenHWGroup opened 1 month ago

MikeOpenHWGroup commented 1 month ago

Bug Description

Hi @mario. In #184 you indicated that compiling RVFI requires inclusion of the cv32e20-dv environment. This sounds odd. Looking at cve2_cs_registers.sv we see:

`ifdef RVFI
    logic [63:0] mstatus_extended_read;
    logic [63:0] mstatus_extended_write;

    assign  mstatus_extended_read[CSR_MSTATUS_MIE_BIT]                              = mstatus_q.mie;
    assign  mstatus_extended_read[CSR_MSTATUS_MPIE_BIT]                             = mstatus_q.mpie;
    assign  mstatus_extended_read[CSR_MSTATUS_MPP_BIT_HIGH:CSR_MSTATUS_MPP_BIT_LOW] = mstatus_q.mpp;
    assign  mstatus_extended_read[CSR_MSTATUS_MPRV_BIT]                             = mstatus_q.mprv;
    assign  mstatus_extended_read[CSR_MSTATUS_TW_BIT]                               = mstatus_q.tw;

... (code snipped)    

    assign rvfi_csr_if.rvfi_csr_addr =  rvfi_csr_addr;
    assign rvfi_csr_if.rvfi_csr_rdata = rvfi_csr_rdata;
    assign rvfi_csr_if.rvfi_csr_wdata = rvfi_csr_wdata;
    assign rvfi_csr_if.rvfi_csr_rmask = rvfi_csr_rmask;
    assign rvfi_csr_if.rvfi_csr_wmask = rvfi_csr_wmask;
    assign rvfi_csr_rmask_q =  ((~csr_wr & csr_op_en_i & ~illegal_csr_insn_o)) ? -1 : 0;
    assign rvfi_csr_wmask_q =   ((csr_wr & csr_op_en_i & ~illegal_csr_insn_o)) ? -1 : 0;
    always @(posedge clknrst_if.clk) begin
        rvfi_csr_addr  = csr_addr_i;
        rvfi_csr_rdata = csr_rdata_int;
        rvfi_csr_wdata = csr_wdata_int;
        rvfi_csr_rmask = (rvfi_csr_rmask_q);
        rvfi_csr_wmask = (rvfi_csr_wmask_q);
    end
...

The reference to clknrst_if.clk is problematic as it places a dependency on the testbench upon the RTL. This is bad practise in general. Is there a way to implement the RVFI without that dependance?

MarioOpenHWGroup commented 1 month ago

Yes, probably this should not be bind to RVFI variable instead of a UVM variable

MikeOpenHWGroup commented 1 month ago

Indeed. AFAIK, there is no phase difference between the clk_i input to this module and clknrst_if.clk in the UVM "clk-and-reset" interface. So, maybe it is just as simple as replacing:

always @(posedge clknrst_if.clk) begin

with

always @(posedge clk_i) begin

Will that work?