openhwgroup / core-v-verif

Functional verification project for the CORE-V family of RISC-V cores.
https://docs.openhwgroup.org/projects/core-v-verif/en/latest/index.html
Other
398 stars 207 forks source link

Disjoint control of Interrupt interface in CORE-V-VERIF #1499

Open MikeOpenHWGroup opened 1 year ago

MikeOpenHWGroup commented 1 year ago

An "interesting implementation" has crept into core-v-verif and it affects how interrupts to the DUT are asserted and de-asserted by the verification environment. This affect all E40 cores and the E20 as well.

Problem Statement: Interrupts can be asserted using the interrupt virtual peripheral or the UVM Interrupt Agent. However, the interrupt virtual peripheral does not de-assert interrupts - this is done by the interrupt agent. Functionally, this works just fine. A directed test can assert interrupts via the virtual peripheral or a constrained-random test can enable random interrupts via the interrupt agent. In both cases, the interrupt agent will observe the interrupt acknowledge handshake and de-assert interrupts as required. Note that the interrupt virtual peripheral does not participate in clearing interrupts, even if it asserted the interrupt. Worse, the directed test-programs that use the interrupt virtual peripheral look like they can clear interrupts - but they cannot (they just don't "know" it).

Why is this a Problem? This is a problem because it is difficult to maintain. The interrupt virtual peripheral cannot be used on its own because it relies on another component, the interrupt agent, to de-assert interrupts. This is not obvious from looking at the code, and at least two Contributors have lost several hours of work trying to understand how it works. It is also sloppy: multiple components assert interrupts, but only one component de-asserts them.

Proposed Solution The proposed solution create a single point of control for interrupts to the DUT, namely the interrupt agent. Rather than asserting interrupts directly, the interrupt virtual peripheral will create and send virtual sequences to the interrupt agent. The interrupt agent will arbitrate between virtual sequences from the virtual peripheral or the UVM testcase.

This change is not anticipated to affect any existing UVM testcases or test-programs.

Before implementing a solution I would appreciate the team's thoughts on this. @YoannPruvost, @MaartenArts, @szbieg, @silabs-mateilga, @silabs-hfegran, @silabs-robin, @silabs-krdosvik

silabs-robin commented 1 year ago

Here are my immediate thoughts:

The interrupt virtual peripheral cannot be used on its own because it relies on another component, the interrupt agent, to de-assert interrupts. This is not obvious from looking at the code, and at least two Contributors have lost several hours of work trying to understand how it works. It is also sloppy: multiple components assert interrupts, but only one component de-asserts them.

This is confusing and bad. I can imagine myself getting frustrated by this too if I were to get caught up in that situation.

The proposed solution create a single point of control for interrupts to the DUT, namely the interrupt agent. Rather than asserting interrupts directly, the interrupt virtual peripheral will create and send virtual sequences to the interrupt agent.

This idea almost came to my mind as I was reading about the problem too. Seems like an orderly way to do it.

This change is not anticipated to affect any existing UVM testcases or test-programs.

If that is truly the case, then I would not personally mind that change. But I feel a slight apprehensiveness.

silabs-robin commented 1 year ago

But I also feel like maybe I don't think it is a big problem. Things work. People have overcome the problems. Changing it requires work. Changing it might break stuff. Overall, I don't have a strong opinion either way.

silabs-hfegran commented 1 year ago

While I am fully behind the reasoning for this proposition, I am a little worried about the possible implications and time that will inevitably have to be spent to get this in place. There is also another interrupt agent present, the clic_interrupt_agent that uses the same VP as the interrupt agent in case the core is configured to use clic interrupts instead of clint. Care needs to be taken to ensure that both these agents still function as intended with the proposed change.

silabs-mateilga commented 1 year ago

As one of the unfortunates who had to dig through this to figure out how it actually fits together, I absolutely agree that this needs fixing. As it is looking at the VP(40s perspective), you can see that the irq lines are driven directly, and no mention or hint at how it is to be deasserted. It takes a lot of time to wrap your head around, and limits the ability of the VP to a large degree.

The proposed solution, where the VP sends sequence items to the agent, is an intuitive and clean solution which keeps well with the UVM way of doing things. I also think it will make the separation of clic/clint agents easier to deal with, if done properly. I second the worries about changing something that works, especially at this time in our project, but a change is in order. We will need to be sure that the integration effort to 40s is kept to a minimum though, or if there seems to be any issues merging will need to be kept on hold until we are ready to deal with any fallout. However, I do not think that this is a valid argument against implementing a fix to this issue.