[dd, aon_timer cov_closure] CCOV exclusion file for aon_timer

[antmarzam]

All the exclusions in this file are unreachable

[rswarbrick]

Hmm. It's a 2-bit signal whose higher bit is constant zero! The code comes from 23d3bb5 and I wonder whether Tim was thinking about safety against fault injection (in which case we probably would have wanted values 01 and 10).

@vogelpi: As a security expert who might have a bit more background on this, what do you think is supposed to happen here? Are we worried about fault injection? (If so, I claim we should change the enum encoding). If not, maybe this should be a single bit?

[rswarbrick]

Hmm: I think this is an example where it would make more sense to make this explicit in the RTL. I'm imagining something like just:

  assign intr_test_qe           = reg2hw.intr_test.wkup_timer_expired.qe;

But with an assertion to make sure we don't accidentally change things in the future:

  `ASSERT(IntrTestFieldsQeMatch_A,
          reg2hw.intr_test.wkup_timer_expired.qe == reg2hw.intr_test.wdog_timer_bark.qe)

[antmarzam]

Can you try to select the lines you're referring to? I have a hard time reading this exclusion file otherwise haha.
I think you mean lines 50-54?

Your change would definitely make closing coverage easier. However, are we confident the register generator (or spec?) will stay as is? I'm not familiar with the tool. But we if we grew the interrupt vector, would that mean that aon_timer.intr_test_q would be ORed with all the separate intr_test fiedls? Is there a chance that if the intr_test was greater than 8-bit the RTL may generate a per-byte Qe?

I guess your assertion would catch the difference if that will ever happen.

Also, should we have a CCOV closure label for assertions that are just a logic check due to simplifying the logic to make closing coverage easier?

[rswarbrick]

I was talking about the lines that are referred to by this annotation. As you say, that's the CONDITIONs on lines 53 and 54.

I'm not confident! Which is why I suggested adding an assertion :-)

I'm not sure what you mean about closure labels? The easiest approach is probably

• Make the RTL change as a PR
• Run coverage collection locally with your change and keep working(!)
• Ignore this coverage hole: it will disappear once the RTL change lands.

[antmarzam]

I've pushed the changes in a new PR with your suggestion. PR is #25811

I'll push the changes with the amended exclusion file in a sec

[rswarbrick]

This is another example where I think an RTL change is the right move. As your note says, there's even an assertion that checks the behaviour won't change :-)

[antmarzam]

This RTL change is in PR #25812

I'll push the amended exclusion file in a sec

[rswarbrick]

I'm not sure that's quite correct. These signals come through prim_sync_reqack, where dst_update_req is visible as src_req_i and dst_update_ack is visible as src_ack_o.

I was slightly surprised that this module doesn't have an assumption that says src_ack_o -> src_req_i. But I don't think it's true! If src_req_i is asserted and acknowledged, what happens when src_req_i drops again?

[antmarzam]

I don't think dst_update_req can't be unset until the dst_update_ack is set just based on how this FSM is working:

opentitan/hw/ip/prim/rtl/prim_reg_cdc_arb.sv

Lines 186 to 228 in 7780c6b

	always_comb begin
	state_d = state_q;
	dst_hold_req = '0;
	// depending on when the request is received, we
	// may latch d or q.
	dst_lat_q = '0;
	dst_lat_d = '0;
	busy = 1'b1;
	unique case (state_q)
	StIdle: begin
	busy = '0;
	if (dst_req) begin
	// there's a software issued request for change
	state_d = StWait;
	dst_lat_d = 1'b1;
	end else if (dst_update) begin
	state_d = StWait;
	dst_lat_d = 1'b1;
	end else if (dst_qs_o != dst_qs_i) begin
	// there's a direct destination update
	// that was blocked by an ongoing transaction
	state_d = StWait;
	dst_lat_q = 1'b1;
	end
	end
	StWait: begin
	dst_hold_req = 1'b1;
	if (dst_update_ack) begin
	state_d = StIdle;
	end
	end
	default: begin
	state_d = StIdle;
	end
	endcase // unique case (state_q)
	end // always_comb
	assign dst_update_req = dst_hold_req | dst_lat_d | dst_lat_q;

Do you disagree?

[rswarbrick]

Looking now, I think I agree. But I guess the prim_sync_reqack module still couldn't have that assertion because it depends on the code around where the module is instantiated.

[rswarbrick]

I think we can see this happen?

• Suppose we are sitting in StIdle and nothing is going on.

• At cycle 0, suppose that dst_req_i is high.

• dst_req_q will get a nonblocking assignment of 1'b1.

• Also (because dst_req_i is high) dst_req will be high too.

• As a result, state_d will be StWait.

• On cycle 1, state_q will be StWait

• As a result, busy will be true.

• Also, dst_req_q will be 1.

So we'll see a hit unless dst_req_i has dropped.

Is that correct? (And is it possible for this to happen?)

[rswarbrick]

I think this is the same comment as the one for line 117?

[antmarzam]

I've created a PR for this: #25814

[rswarbrick]

I know that I haven't got to the bottom of the list, but I think there are a few items to work through so far :-)