fix #306

[davideschiavone]

SEC when X-IF is expected to fail

[davideschiavone]

@FrancescoDeMalde-synthara @StMiky @MaurizioCapra-synthara can you please check this PR?

[cairo-caplan]

@davideschiavone Thank you for the changes!

I made some questions, in summary:

1. If the modifications on cve2_ex_block are really necessary.
2. Add a comment line describing or summarizing the intended scoreboard mechanism introduced.
3. A probable mistake on the assignment of the number of the CV-X-IF in-flight instructions.

At this point, we would really benefit of a basic example that tests this code, be it here or as a test on cv32e20-dv.

[davideschiavone]

@davideschiavone Thank you for the changes!

I made some questions, in summary:

1. If the modifications on cve2_ex_block are really necessary.
2. Add a comment line describing or summarizing the intended scoreboard mechanism introduced.
3. A probable mistake on the assignment of the number of the CV-X-IF in-flight instructions.

At this point, we would really benefit of a basic example that tests this code, be it here or as a test on cv32e20-dv.

hi @cairo-caplan , thanks - I did all the modifications suggested - regarding the test, unfortunately it would need a coprocessor or something like that (or at least an UVM agent) which I believe can be discussed later

[cairo-caplan]

@davideschiavone, thank you for the changes and for the clear description of the scoreboard mechanism on the ID Stage.

I am approving it.

[davideschiavone]

@davideschiavone, thank you for the changes and for the clear description of the scoreboard mechanism on the ID Stage.

I am approving it.

Thanks @cairo-caplan - I would still like to get @StMiky review as he is the one who found the bug - then I merge.

Best
Davide

[StMiky]

I tested the core with a light-weight vector coprocessor. All the regression tests pass and the X-IF ID is maintained as expected across multiple outstanding instructions.

@davideschiavone one thing that might be worth checking is that with the current implementation the core may process interrupts/exceptions/WFI without waiting for the completion of all the offloaded instructions. As an example, a WFI issued right after an offloaded instruction taking multiple cycles to execute may prevent the core from saving the return value from the offloaded instruction.

A second, minor change that I would apply is to expose the X_ID_WIDTH parameter in the top-level (cve2_core.sv) instead of hardwiring it in cve2_pkg.sv. This would make it easier to override it based on the desired number of outstanding instructions.

[FrancescoDeMalde-synthara]

Hi @davideschiavone, @StMiky, @cairo-caplan,
I agree with the analysis and with the proposed improvements, which will be useful for future extensions of our system. The changes do not break any functionality in the system as it exists today and pass our tests.
Thank you for the update.
Best regards,
Francesco

[MikeOpenHWGroup]

@StMiky suggested the following, and I believe @FrancescoDeMalde-synthara likes it:

A second, minor change that I would apply is to expose the X_ID_WIDTH parameter in the top-level (cve2_core.sv) instead of hardwiring it in cve2_pkg.sv. This would make it easier to override it based on the desired number of outstanding instructions.

As this would be a slightly different discussion that fixing #306, I took created a GitHub Discussion, #309 for it. Please add your thoughts there.

[davideschiavone]

avideschiavone one thing that might be worth checking is that with the current implementation the core may process interrupts/exceptions/WFI without waiting for the completion of all the offloaded instructions. As an example, a WFI issued right after an offloaded instruction taking multiple cycles to execute may prevent the core from saving the return value from the offloaded instruction.

@StMiky - actually this is a very good point and you are right - I will fix and test this with an exception/systcall/debug/interrupt request to be sure the core waits for the scoreboard before taking them -

Note that this may have a few complication:

(1) today if an interrupt arrives and the core is waiting for a X-IF instruction to write back - it won't take the interrupt
(2) with the proposed changed, it will wait for ALL of them - so here we loose quite a lot of real time capabilities, but I would say that when XIF is enabled, it is ok as long as it's documented (which I will add in the comments as the XIF is not part of the core by default and doesnt target TRL-5 any time soon)

best
Davide