[rtl] Fix NAPOT address matching (#1178)

[michael-platzer]

Generate correct masks for NAPOT range addresses covering the entire granule. When PMPGranularity > 0, then a NAPOT range can span the whole granule, which requires the next-lowest bit (i.e. highest bit that is not part of the granule) to be 0. Fixes #1178

[tomeroberts]

Agree with this - thanks for the fix

[byllgrim]

Hi @tomroberts-lowrisc and @michael-platzer .
I think maybe this merge introduced another bug.

Please consider line 68:
https://github.com/lowRISC/ibex/blob/master/rtl/ibex_pmp.sv#L68
assign region_addr_mask[r][b] = [...] was changed from using [b-1:PMPGranularity+2] to [b-1:PMPGranularity+1].
I think this breaks NAPOT matching for G=0.

Here is an example that demonstrates my point.
The cover property below is reachable, it also works from PMPGranularity>1, but it does not work for PMPGranularity==0.
The example demonstrates how the "ignore patterns" (e.g. yyyy...y011) works.
That is, pmp_req_addr_i is purposefully set to not match csr_pmp_addr_i in the 5 lsb (except for [1:0] which is not part of pmpaddress register!).
It is perfectly fine that they don't match because of the 01111 pattern at the end of csr_pmp_addr_i.

cov_please_napot: cover property (
  // given:
  (PMPGranularity == 1)
  && (csr_pmp_cfg_i[0].mode == ibex_pkg::PMP_MODE_NAPOT)
  && (priv_mode_i === ibex_pkg::PRIV_LVL_U)
  // then:
  && (csr_pmp_addr_i[0][33:2] == 8'b11101111)
  && (pmp_req_addr_i[33:2]    == 8'b11110101) //5-bit mismatch ignored?
  && (pmp_req_err_o == 0 ) //hope to get a match
  );

As said, G>=1 is ok, but try it with G=0.
Because, in order to ignore the bits as dictated by the low en of pmpaddress register (e.g. pmpaddress[2:0]=3'b011), then one would need to have a 1 at csr_pmp_addr_i[1] (aka pmpaddress[-1]), or else the masking logic will not work.

I think it is a bug. But since there are so many nuances of the PMP spec that I might have misunderstood, I thought it best to ask if you guys agree or not.
If it is not a bug, but some mistake on my part, then please let me know :)
If my describe was hard to interpret then also let me know and I can word myself differently :)

[rswarbrick]

Hi there and thanks for the message. We might take a couple of weeks to get back to this properly because most lowRISC employees are having a Christmas break at the moment. See you in the new year!

[byllgrim]

Btw, 3 more comments to my post above.

This seemingly only fails when csr_pmp_addr_i[1] = 0.
But that should be the case anyway, as seen here:
https://github.com/lowRISC/ibex/blob/master/rtl/ibex_cs_registers.sv#L1071
assign csr_pmp_addr_o[i] = {pmp_addr_rdata[i], 2'b00};
And the Vol II spec seem to want that as well.

Above, the merge says "3 checks passed", but if you open it up
https://github.com/lowRISC/ibex/runs/1364421198
It says "10 errors".

At a first glance, it seems like a quick (but not so elegant) fix is as follows.
I have not tested this.

      end else begin : g_others
        // We will mask this bit if it is within the programmed granule
        // i.e. addr = yyyy 0111
        //                  ^
        //                  | This bit pos is the top of the mask, all lower bits set
        // thus mask = 1111 0000
        if (PMPGranularity == 0) begin
          assign region_addr_mask[r][b] = (csr_pmp_cfg_i[r].mode != PMP_MODE_NAPOT) |
                                          ~&csr_pmp_addr_i[r][b-1:2];
        end else begin
          assign region_addr_mask[r][b] = (csr_pmp_cfg_i[r].mode != PMP_MODE_NAPOT) |
                                          ~&csr_pmp_addr_i[r][b-1:PMPGranularity+1];
        end
      end

[michael-platzer]

Hi @byllgrim ,

thanks for pointing this out and apologies for overlooking the consequences this change would have for a PMPGranularity of 0!

Changing the indexing of csr_pmp_addr_i on line 68 from [b-1:PMPGranularity+2] to [b-1:PMPGranularity+1] was correct nonetheless, since the standard requires that "When G≥2 and pmpcfgi. A[1] is set, i.e. the mode is NAPOT, then bits pmpaddri[G-2:0] read as all ones.", so bit G-1 (i.e. PMPGranularity+1) must still be taken into account.

However, as you wrote, this change breaks the behavior for a PMPGranularity of 0, which now includes bit 1 of csr_pmp_addr_i. For NAPOT this bit should be implicitely 1, but it is set to 0 in ibex_cs_registers.sv. So the situation where PMPGranularity is 0 requires dedicated handling.

The fix that you propose seems fine to me.

About the error messages in the merge checks: I do not know why these appear, but I do not think that they have been introduced by this merge, since they already appear in earlier merges (for instances in 1171 and 1177).

[GregAC]

Hi @byllgrim thanks for raising this and providing a fix. I have opened a PR #1234 to implement the fix. In general please do submit a PR when you have a fix like this. In this instance I needed the fix for some other work I was doing (Implementing extended PMP behaviour #1232) so did the PR myself.

With regards to the checks passed message along with errors this is an issue with the compliance suite expecting an exception on unaligned accesses (where Ibex deals with these in hardware which is spec compliant), so the errors are ignored for the checks, see here for more details: #100