[BUG] PMPCFG : Dead code in pmp module

[AyoubJalali]

Is there an existing CVA6 bug for this?

• I have searched the existing bug issues

Bug Description

Hello,

I noticed that the write of the value (2'b10 == NA4) in the pmpcfg.A is ignore for all the CVA6 configuration, That's means we don't support NA4, that's why we don't NA4 code in the pmp module.

We had some code coverage holes :

@OlivierBetschi what do you think ?

[OlivierBetschi]

According to https://docs.openhwgroup.org/projects/cva6-user-manual/01_cva6_user/PMP.html the NA4 mode is not selectable because of the granularity of the PMP being 1. This is confirmed in the RISC-V privileged ISA (3.7.1 Physical memory protection CSRs) : When G ≥ 1, the NA4 mode is not selectable.

So NA4 not being supported makes sense. Then the question if why the granularity is set to 1, as I don't see anything in the CVA6 requirements ( https://docs.openhwgroup.org/projects/cva6-user-manual/02_cva6_requirements/cva6_requirements_specification.html ) related to the granularity of the PMP. I don't know who could answer that ?

[AyoubJalali]

@zchamski we had some discussion on this before

[JeanRochCoulon]

To my mind, G can be equal to zero for cva6. I preparing a new PR which adds a new config parameter called PMPG (PMP granularity) to define G.
For 60x and 65x, G=1 which means granularity = 8 Bytes (to be updated in the following riscv specification chapter) and PMPCfg.A[1] is read only zero, which means that only OFF and TOR are supported. As consequence the riscv specification tells that PMPaddr[0] is read only, which is not the case in RTL (for 65x).

https://docs.openhwgroup.org/projects/cva6-user-manual/04_cv32a65x/riscv/priv.html#_address_matching:~:text=%5BCV32A65X%5D%20Although%20the,address%2Dmatching%20logic.

For other configurations, I set G=0 which means granularity = 4 Bytes.
This new PR produces a green CI for all tested configurations.

@zchamski @ASintzoff @AyoubJalali @OlivierBetschi @Moschn please warm me if you are not ok with this.

If agree, the ongoing PR will

• add the PMPG parameter,
• fix PMP RTL to remove the coverage hole ([BUG] PMPCFG : Dead code in pmp module #2665),
• fix the riscv specification (saying that G=1) and
• fix the PMPaddr CSR register to declare bit0 as read only ([BUG] [CV32A65X] Writes into PMPADDR registers do not follow specification #2657).

[Moschn]

Hi,

If I remember correctly, G=0 or NA4 is quite tricky to support in 64-bit configurations of CVA6 due to misaligned memory accesses. The RISC-V spec mandates that all bytes of an access are checked against the PMP. As far as I know, CVA6 only support memory accesses aligned to 4 bytes, but the 64-bit variant can obviously perform 8-byte reads and writes. Thus CVA6 in a 64-bit configuration could try to access an 8-byte value where the first 4 bytes pass the PMP check, and the last 4 bytes do not. The current implementation only checks the address of the access but not its width. Thus, such an access would succeed, allowing to read 4 bytes that should have been protected.

If you want to enable G=0 (or NA4) in 64-bit variants of CVA6, you need to either force the core to only make 8 byte aligned accesses or implement additional checks for the PMP.

I would probably not introduce another parameter, but rather just make G depend on X_LEN. If X_LEN is 32, then G=0, and if X_LEN=64, then G=1. I would do it this way to make it implicitly clear that one cannot have a 64-bit CVA6 instantiation with G=0. Otherwise, some users might try that and will probably not notice that something is wrong (and confidential data might leak).

Cheers,

[AyoubJalali]

to be more specific, CV32A65X config doesn't support NA4 & NAPOT matching config address, to not add more complexity to the code, I suggest adding pragma for NA4 & NAPOT codes, and of course we can disable pragma from the command line.

But in general the CVA6 doesn't support NA4 at all.

[OlivierBetschi]

I would prefer the solution from @Moschn to make G dependent on XLEN rather than adding a parameter. In my opinion having the smallest granularity being equals to XLEN makes the most sense. @JeanRochCoulon what is the rationale to keep G=1 for cv32a60x/cv32a65x ?

[JeanRochCoulon]

Some updates about PMP:

• The rational to keep G=1 is to keep the RTL as it is Today (no improvment = no new bug) @OlivierBetschi
• That's a good idea to make G dependent from XLEN
• For 60x and 65x, we plan to disable the PMP.