search

openhwgroup / cv32e40p

CV32E40P is an in-order 4-stage RISC-V RV32IMFCXpulp CPU based on RI5CY from PULP-Platform
https://docs.openhwgroup.org/projects/cv32e40p-user-manual/en/latest
Other
941 stars 412 forks source link

cv32e40p_prefetch_buffer: Is it ok to assert hwlp_jump_i when there are no outstanding instr_ requests (ie cnt_q == 0)? #954

Closed bdiz closed 7 months ago

bdiz commented 7 months ago

I don't have a good understanding of the HWLoop feature at the boundary of the cv32e40p_prefetch_buffer. Not sure if I found a bug or if my stimulus is illegal. Can you tell me which is the case and, if the latter, I will file an issue?

Let me start with a waveform: image

At 2,325ns hwlp_jump_i asserts with fifo_empty_i. hwlp_wait_resp_flush also asserts causing assignment of hwlp_flush_cnt_delayed_q to cnt_q - 1. There is a comment stating, "// cnt_q > 0 checked by an assertion".

https://github.com/openhwgroup/cv32e40p/blob/6adc6b3d71d93dfbb5006cd3fa095331df38e8fd/rtl/cv32e40p_prefetch_controller.sv#L282-L285

Here, cnt_q is 0 so hwlp_flush_cnt_delayed_q is set to 3. This propogates to flush_cnt_q which deasserts fetch_valid_o because flush_cnt_q is greater than 0.

https://github.com/openhwgroup/cv32e40p/blob/6adc6b3d71d93dfbb5006cd3fa095331df38e8fd/rtl/cv32e40p_prefetch_controller.sv#L123

But fifo_valid remains asserted allowing fifo_pop at 2,385ns.

https://github.com/openhwgroup/cv32e40p/blob/6adc6b3d71d93dfbb5006cd3fa095331df38e8fd/rtl/cv32e40p_prefetch_controller.sv#L212

The fetch interface never gets to see the data 'h54f1_875f from inst_rdata_i at 2,345ns because it was popped out of the fifo at 2,385ns while fetch_valid_o is deasserted. My testbench flags the missing data.

Am I doing something wrong?

Thanks.

pascalgouedo commented 7 months ago

Hi, Please give all information (core-v-verif and cv32e40p git repos and hashes together with command used to launch uvmt test-bench simulation) to be able to reproduce.

bdiz commented 7 months ago

I'm just looking to talk high level about it. But if at a glance you suspect it may be an issue, I can file an issue with the extra information you are requesting. That is, except for the run command as it is a standalone testbench for the prefetch_buffer.

pascalgouedo commented 7 months ago

Hi, I suspected that you were doing module level verification after this #950 issue. Verification of CV32E40P is done using CORE-V Verification methodology meaning generating assembly tests and executing them on the whole CPU. There is no such module level verification because first it is useless to breakdown verification on such a small CPU and also because there is no module level design specifications. So you are maybe (seems so with this new issue) generating scenario which are not real/possible and it would take too much time to analyse every issues you could create. Module level verification is indeed interesting for units like Instruction or Data Caches, Interrupt Controller or anything which is self-sufficient and has detailed functional and design specifications. But extracting prefetch buffer (or any other CPU unit) from its context well it is a waste of time in my opinion.

bdiz commented 7 months ago

I agree. Being that the prefetch_buffer has no design spec means it has no requirements for which to do verification against.

The primary reason I am creating a testbench for the prefetch_buffer is for the purpose of demonstrating automated VIP creation. I started the discussion to see if I needed to alter my stimulus. If, as a side effect, I found an issue that would help the cv32e40p project become healthier, that would have just been a bonus.

I guess even general discussions of internal module behaviors are moot, unless the behavior is observed in a testbench with the whole CPU.