DV style guide: Do not use illegal_bins as a checker

[antmarzam]

No description provided.

[marnovandermaas]

A few comments from my end. Can you also link the issue where this was discussed?

[marnovandermaas]

Maybe give an example of what that check could be like an assertion.

[antmarzam]

@marnovandermaas I've addressed all your comments except for: #88 (comment)

I'm not sure how to give an example since this is very generic. It's going to depend on each TB and what type of check is easier to implement. It could be an assertion, or maybe even an UVM check if that suits better

[marnovandermaas]

Linked to the following issue: #87
What's our policy on changing these guidelines when OpenTitan doesn't comply with them? Should we first fix the OpenTitan codebase?

[antmarzam]

Linked to the following issue: #87 What's our policy on changing these guidelines when OpenTitan doesn't comply with them? Should we first fix the OpenTitan codebase?

I've taken a look at the way we use illegal_bins in OT and I couldn't find a scenario in which the keyword was used as a checker. It's mostly there so for the situations where we cross coverage we save ourselves the pain to manually exclude.

[antmarzam]

Linked to the following issue: #87 What's our policy on changing these guidelines when OpenTitan doesn't comply with them? Should we first fix the OpenTitan codebase?

I've taken a look at the way we use illegal_bins in OT and I couldn't find a scenario in which the keyword was used as a checker. It's mostly there so for the situations where we cross coverage we save ourselves the pain to manually exclude.

To follow up on my last comment:

I don't think the illegal_bins keyword is being used as a checker, but in some places is unnecessary, since all the bins are explicitly defined.
I've pushed a PR#27411 removing illegal_bins from the alert_handler, since these aren't needed there.

[martin-velay]

During our meeting I mentioned the usage of assertion. As we can't do it in a class, we should add it in the RTL itself, or if we prefer to keep it on the DV side, we should better use a UVM check as Antonio has suggested in a comment. Here is a concrete example:

Note

Code below hasn't been tested and may contain mistakes!

  // --------------------------------------------
  // ----- ILLEGAL BINS USED                -----
  // --------------------------------------------
  // This code snippet is part of the coverage class
  covergroup mem_cg;
    access_type_cp: coverpoint (access_type) {
      bins read    = {0};
      bins write   = {1};
      bins execute = {2};
    }

    addr_cp: coverpoint (addr) {
      bins addr_rwx = {[0:49]};   // Read, Write and Execute are allowed here
      bins addr_ro  = {[50:$]};   // Read Only range
    }
    
    // /!\ The illegal bins declared hareafter are used as checkers
    access_x_addr: cross access_type_cp, addr_cp {
      illegal_bins write_ro_range   = binsof (access_type_cp.write)   && binsof (addr_cp.addr_ro);
      illegal_bins execute_ro_range = binsof (access_type_cp.execute) && binsof (addr_cp.addr_ro);
    }
  endgroup : mem_cg

  // --------------------------------------------
  // ----- SOLUTION 1 - Class based check   -----
  // --------------------------------------------
  // Instead of doing so, we should better use uvm_fatal or uvm_error to report the illegal 
  // configuration. We can do this by adding a function called each time the configuration is 
  // updated. For example, each time the simulation goes into the `process_tl_access` task (or maybe 
  // only when a write is performed).
  task my_scoreboard::process_tl_access(tl_seq_item item, tl_channels_e channel, string ral_name);
    // ...
    check_config_validity();
    // ...
  endtask : process_tl_access
  
  function void my_scoreboard::check_config_validity();
    if ((access_type == "write" || access_type == "execute") && addr >= 50) begin
      `uvm_fatal(`gfn, "Invalid configuration: a write/execute access is being tried into the read-only range")
    end
  endfunction : check_config_validity

  // --------------------------------------------
  // ----- SOLUTION 2 - RTL assertion check -----
  // --------------------------------------------
  // Add an assertion in the RTL code, or do a bind to the RTL code, to check that the configuration
  // is valid.
  `ASSERT(RoRangeAssert, ((access_type == "write" || access_type == "execute") |-> addr < 50))

What do you think?

[rswarbrick]

I think I'd probably go for "solution 1"

[antmarzam]

You could also add an assertion in the class, then trigger it via a SV event or similar.
In my view, the check should just be done in whichever ways makes more sense. Assertion when is easier or a UVM check when that's easier.

[martin-velay]

Yes, I don't mind so much of the way, as long as we're sure the issue is raised properly

[antmarzam]

I just pushed the changes clarifying a bit where the check could be carried out. Please have a look and see if you're happy with it :)

[martin-velay]

LGTM, thanks Antonio!