core-v-verif icon indicating copy to clipboard operation
core-v-verif copied to clipboard

Adding support for RVFI on cv32e40p env

Open YoannPruvost opened this issue 1 year ago • 6 comments

First step toward supporting RVFI based test, done by merging the original env and testbench with the cv32e40x one. For now, only the ci tests are supported

YoannPruvost avatar Aug 03 '22 16:08 YoannPruvost

Hi @YoannPruvost. Is this PR related to cv32e40p #717? Should that PR be completed before this one?

MikeOpenHWGroup avatar Aug 05 '22 03:08 MikeOpenHWGroup

Hi @YoannPruvost. Is this PR related to cv32e40p #717? Should that PR be completed before this one?

Hi @MikeOpenHWGroup yes, the pull request on the core should be completed first. This update of the env requires the RVFI to be implemented on the cv32e40p.

YoannPruvost avatar Aug 05 '22 07:08 YoannPruvost

Thanks @YoannPruvost. I added the DO NOT MERGE label. We can remove it when cv32e40p #717 has been merged. This PR will also need to update cv32e40p/sim/ExternalRepos.mk to point to the new hash of the cv32e40p repo.

MikeOpenHWGroup avatar Aug 05 '22 14:08 MikeOpenHWGroup

Hi @MikeOpenHWGroup the external repo has been updated so we can merge this PR. Cheers

YoannPruvost avatar Aug 10 '22 14:08 YoannPruvost

Thanks for the update @YoannPruvost - much appreciated. I'll approve and merge this once cv32e40p #719 is merged in.

MikeOpenHWGroup avatar Aug 12 '22 20:08 MikeOpenHWGroup

@pascalgouedo, I have (finally) determined why I could not get this PR to work. There is an issue with cv32e40p_rvfi.sv in which a set of members of class insn_trace_t are written to with non-blocking assignments. It is not legal SystemVerilog to use non-blocking assignments for automatic variables, and by default all object members in SV are automatic. It turns out that Cadence Xcelium allows this, but Metrics Dsim does not. (I believe Dsim's interpretation of the SV LRM is correct, so these should be compile-time errors under Xcelium as well.)

My first attempt to resolve the problem was to remove all non-blocking assignments (<=) with blocking assignments (=). This resolved the compile issue with Dsim but introduced functional errors in simulation. My second attempt was to declare all members of class insn_trace_t to be static variables. This should be safe because as far as I can tell, the objects constructed from this class are "newed" once and only once during a simulation.

I believe that merging this PR will work for Cadence Xcelium, but it will kill our CI flow which runs under Dsim, so I would like to discuss the updates I think are necessary for both the cv32e40p and core-v-verif repos to resolve this. You should have a meeting invitation in your Inbox already.

MikeOpenHWGroup avatar Sep 02 '22 04:09 MikeOpenHWGroup