Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Incorrect handling of PMPADDR register reads in NA4 / NAPOT address matching modes #2656

Open
1 task done
zchamski opened this issue Dec 9, 2024 · 2 comments
Open
1 task done
Labels
notCV32A65X It is not an CV32A65X issue Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system

Comments

@zchamski
Copy link
Contributor

zchamski commented Dec 9, 2024

Is there an existing CVA6 bug for this?

  • I have searched the existing bug issues

Bug Description

The RTL code of CSR read process (https://github.com/openhwgroup/cva6/blob/master/core/csr_regfile.sv#L851C1-L857C71) incorrectly assumes that the last bit of pmpaddr registers should be read as 1 if the corresponding pmpNcfg.A[1] bit is set. Table 19 in the privileged ISA spec (v20240411) states that the value of bit 0 in a pmpaddrN register can be 0 in NA4 mode and must be 0 in NAPOT mode with 8 byte ranges:

image

@Moschn, can you please have a look at this?

@zchamski zchamski added the Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system label Dec 9, 2024
@Moschn
Copy link
Contributor

Moschn commented Dec 9, 2024

I think you are correct. I'm not sure where this bug crept in; at some point, this was working correctly (I checked the supported granularity in software a couple years ago). Feel free to propose a fix!

@JeanRochCoulon JeanRochCoulon added the notCV32A65X It is not an CV32A65X issue label Dec 9, 2024
@Moschn
Copy link
Contributor

Moschn commented Jan 17, 2025

I checked this again today. In the past, we did not support NA4, which coincides with the current behavior of the code. To clarify, we set G=2 (Granularity of a minimum of $2^{G+2} = 16$ bytes) and thus NA4 to be disabled. The important sentences in the spec:

When $G>=1$, the NA4 mode is not selectable. When $G>=2$ and $pmpcfg_i.A[1]$ is set, i.e. the mode is NAPOT, then bits $pmpaddr_i[G-2:0]$ read as all ones. When $G>=1$ and $pmpcfg_i.A[1]$ is clear, i.e. the mode is OFF or TOR, then bits $pmpaddr_i[G-1:0]$ read as all zeros.

The current code performs exactly this behavior:

if (pmpcfg_q[index].addr_mode[1] == 1'b1) // i.e., A[1] is set
  csr_rdata = {pmpaddr_q[index][CVA6Cfg.PLEN-3:1], 1'b1}; // pmpaddr_i[0:0] = 1
else csr_rdata = {pmpaddr_q[index][CVA6Cfg.PLEN-3:1], 1'b0}; // A[1] is not set -> pmpaddr_i[1:0] = 0

I was also confused at first when I reread the spec and the code, but I think there was a mistake that I thought G=2 means 8-byte granularity. Instead, G=2 means 16-byte granularity, and then also the table is correct, i.e., in NAPOT mode, always force bit 0 to 1.

I believe this still holds with respect to the recent changes (#2685).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notCV32A65X It is not an CV32A65X issue Type:Bug For bugs in the RTL, Documentation, Verification environment or Tool and Build system
Projects
None yet
Development

No branches or pull requests

3 participants