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
446 stars 222 forks source link

ISS Error Not Counted #1503

Closed silabs-robin closed 10 months ago

silabs-robin commented 1 year ago

Type

Steps to Reproduce

Branch: cv32e40s/dev. Hash: Latest or 47a423f0

cd cv32e40s/sim/uvmt/
make test TEST=fencei USE_ISS=0

The ISS will print Error (IDV) validateRefMemoryWrite: Reference model not initialized but the test will still pass. Quit count : 0 of 5 and UVM_ERROR : 0.

Additional context

Errors should lead to test fail. Otherwise, regressions can mask problems. This was first spotted in https://github.com/openhwgroup/core-v-verif/issues/1502

silabs-hfegran commented 1 year ago

@silabs-robin is this still an issue?

silabs-robin commented 1 year ago

still an issue?

I have not heard that it has been fixed.

However, it is not blocking anything. But there is a risk that errors can go undetected.

MikeOpenHWGroup commented 1 year ago

Please assign this issue to a specific person to resolve.

silabs-hfegran commented 1 year ago

I will have a look at this later - I believe this has been fixed, but need to verify

silabs-hfegran commented 1 year ago

Still an issue - I guess the ISS needs to be configurable to return all errors as UVM_ERROR or UVM_FATAL, it is somewhat inconsistent when it in certain cases reports an error but does not adequately communicate this to the test environment.

Do you have any thoughts on this @eroom1966?

eroom1966 commented 1 year ago

I am a bit confused as to the requirement here The original post was excluding the use of the REF model in the simulation make test TEST=fencei **USE_ISS=0**

If it is excluded, what is the expectation ? Maybe this is linked to the issue regarding calling the memory write ?

@MikeOpenHWGroup What would you recommend here ? This is effectively a misuse of the API which we did not trap (we will trap this in future) but this is simply a DPI function, this has no way of affecting UVM I believe.

MikeOpenHWGroup commented 1 year ago

I'll need to spend some time on this, but won't be in a position to comment until the 31st at the earliest. Until then I have a comment and a question:

eroom1966 commented 1 year ago

@MikeOpenHWGroup How would you return a UVM_ERROR or UVM_FATAL from an imported C call ?

Here is a simple contrived example

In other words there is a C function in a shared object which is linked into the simulation process

In the C code

void myCfunc (int x) {
    // check for legal value
    if (int > 5) fprintf(stderr, "ERROR: x=%d, Usage (x > 0) && x < 5)\n", x);
}

In the SV code

import "DPI-C" context function void myCfunc(input in x);
initial myCfyunc(10); // causes an error

How can the error detected in C, cause a UVM_ERROR or UVM_FATA to be reported ?

Question: what is the "misuse of the API"? I tend to agree that we should not be abusing the API.

There was a semantic usage error, a specific function was called before rvviRefInit() This is now detected and reported from the shared object.

thx Lee

MikeOpenHWGroup commented 1 year ago

You are right @eroom1966 that the processes on the other side of the DPI cannot invoke uvm_errors directly. So we need an explicit communication of the status of any DPI call. A minimal implementation would be for myCfunc() to return an error code. The SV code that invokes myCfunc() would use the return code to invoke uvm_error or uvm_fatal as required.

A better, although perhaps over-engineered, approach would be to return an error struct, which could (for example) include an error flag and an error string.

eroom1966 commented 1 year ago

Hi Mike

A minimal implementation would be for myCfunc() to return an error code. The SV code that invokes myCfunc() would use the return code to invoke uvm_error or uvm_fatal as required.

The API functions already return a success / fail status, so this could be done today, here is an extract form the header include file, and the corresponding include for system verilog imports

/*! \brief Initialize the DV reference model.
 *
 *  \param programPath File path of the ELF file to be executed. This parameter can be NULL if required.
 *
 *  \return RVVI_TRUE if the reference was initialized successfully else RVVI_FALSE.
 *
 *  \note The reference model will begin execution from the entry point of the provided ELF file but can be overridden by the rvviRefPcSet() function.
**/
extern bool_t rvviRefInit(
    const char *programPath);
import "DPI-C" context function int rvviRefInit(
    input string programPath);

thx Lee

MikeOpenHWGroup commented 1 year ago

Perfect! As long as every error identified by the Reference Model is passed back to the SystemVerilog side I am happy. Of course, this places a burden on the SystemVerilog testbench to catch these return codes and use them appropriately (that is, verification engineers should apply good coding practises!).

@silabs-hfegran does this work for you?

silabs-robin commented 1 year ago

The API functions already return a success / fail status, so this could be done today

Seems ok to me. Then we just have to update every ISS API call to check return values.


As long as every error identified by the Reference Model is passed back to the SystemVerilog side I am happy.

What about these?

Error (CF_UE) '--override cpu/Zcmb=1' did not match anything in the platform
    PPPPPPP    AAAAAA    SSSSSS    SSSSSS   EEEEEEEE  DDDDDDD
    PP    PP  AA    AA  SS    SS  SS    SS  EE        DD    DD
    PP    PP  AA    AA  SS        SS        EE        DD    DD
    PPPPPPP   AAAAAAAA   SSSSSS    SSSSSS   EEEEE     DD    DD
    PP        AA    AA        SS        SS  EE        DD    DD
    PP        AA    AA  SS    SS  SS    SS  EE        DD    DD
    PP        AA    AA   SSSSSS    SSSSSS   EEEEEEEE  DDDDDDD
    ----------------------------------------------------------
                        SIMULATION PASSED
    ----------------------------------------------------------
silabs-hfegran commented 1 year ago

@silabs-hfegran does this work for you?

Sounds good to me

What about these?

Misconfigured ISS should ideally be caught as well

eroom1966 commented 1 year ago

Yes, these should be caught. I noticed these myself and have passed along to a colleague

silabs-robin commented 11 months ago

Yes, these should be caught. I noticed these myself and have passed along to a colleague

Hi @eroom1966 , do you remember how this tale ended?

(I'm cleaning up the Github tickets a bit.)

eroom1966 commented 11 months ago

I think this got resolved - feel free to close

silabs-robin commented 11 months ago

Thanks, Lee. I have a slight recollection of the same thing.

I'll leave it open a little while longer, to later see if I can find any traces of evidence somewhere.

silabs-hfegran commented 10 months ago

Resolved