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

Allow hard-wiring of PMP regions #1769

Merged

Conversation

michael-platzer
Copy link
Contributor

This PR adds three new fields to the cva6_cfg_t configuration struct, which allow to specify reset values for the PMP configuration and address CSRs as well as optionally making individual PMP entries read-only. The purpose is to allow hard-wiring of certain regions' privileges, which is explicitly allowed by the RISC-V Privileged Architecture specification Machine-Level ISA, v1.12 (see Sect. 3.7).

Hard-wiring of PMP regions is useful whenever a SoC has memory regions with fixed properties and protection rules. Instead of relying on SW to setup the PMP regions correctly on every boot, with this PR regions may be initialized using the new parameters and optionally made read-only.

core/include/config_pkg.sv Show resolved Hide resolved
core/include/config_pkg.sv Show resolved Hide resolved
core/include/config_pkg.sv Outdated Show resolved Hide resolved
core/include/config_pkg.sv Show resolved Hide resolved
core/include/cv32a60x_config_pkg.sv Outdated Show resolved Hide resolved
core/include/cv64a6_imafdc_sv39_config_pkg.sv Outdated Show resolved Hide resolved
core/include/cv64a6_imafdc_sv39_hpdcache_config_pkg.sv Outdated Show resolved Hide resolved
core/include/cv64a6_imafdc_sv39_openpiton_config_pkg.sv Outdated Show resolved Hide resolved
core/include/cv64a6_imafdc_sv39_wb_config_pkg.sv Outdated Show resolved Hide resolved
core/include/cv64a6_imafdcv_sv39_config_pkg.sv Outdated Show resolved Hide resolved
Copy link
Contributor

❌ failed run, report available here.

@zarubaf
Copy link
Contributor

zarubaf commented Jan 16, 2024

lgtm, I think we miss the configuration in some testbench :-) Maybe @cathales can help? 🥹

@michael-platzer michael-platzer force-pushed the michael-platzer/feature/constant-pmps branch from 3b15abe to 1ae5ec5 Compare January 17, 2024 08:06
Copy link
Contributor

❌ failed run, report available here.

@michael-platzer michael-platzer force-pushed the michael-platzer/feature/constant-pmps branch 2 times, most recently from e397f5e to 3cc446a Compare January 17, 2024 08:53
Copy link
Contributor

❌ failed run, report available here.

1 similar comment
Copy link
Contributor

❌ failed run, report available here.

@michael-platzer michael-platzer force-pushed the michael-platzer/feature/constant-pmps branch from 3cc446a to 7a97ba5 Compare January 17, 2024 09:22
Copy link
Contributor

❌ failed run, report available here.

@michael-platzer
Copy link
Contributor Author

I tried a few things as far as I could parse the error messages from the logs but now it appears that the CI issues are unrelated to my changes. Does anyone have some feedback/suggestions as to how I could resolve these issues?

@cathales
Copy link
Contributor

Hi! We encountered similar issues while reviewing #1760.

  1. It seems you are missing a modification in cva6.sv to insert your values inside the extended configuration. So the fields of the extended configuration are shifted, the behavior becomes inconsistent and it might be hard to debug (a little bit like C’s undefined behavior).
  2. It didn’t reach FPGA build, but mind ariane_xilinx.sv too

This commit adds three new fields to the cva6_cfg_t configuration
struct, which allow to specify reset values for the PMP configuration
and address CSRs as well as optionally making invidiual PMP entries
read-only.  The purpose is to allow hard-wiring of certain regions'
privileges, which is explicitely allowed by the RISC-V Privileged
Architecture specification Machine-Level ISA, v1.12 (see Sect. 3.7).
@michael-platzer michael-platzer force-pushed the michael-platzer/feature/constant-pmps branch from 7a97ba5 to 84be727 Compare January 17, 2024 15:47
@michael-platzer
Copy link
Contributor Author

Thanks @cathales for the suggestions! Added the missing modifications, so let's see whether it works now 🙂

Copy link
Contributor

✔️ successful run, report available here.

Copy link
Contributor

@zarubaf zarubaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks

@zarubaf zarubaf merged commit 78111aa into openhwgroup:master Jan 17, 2024
19 checks passed
@michael-platzer michael-platzer deleted the michael-platzer/feature/constant-pmps branch January 18, 2024 08:59
rohan-10xe pushed a commit to 10x-Engineers/cva6 that referenced this pull request Jan 23, 2024
…up#1769)

This commit adds three new fields to the `cva6_cfg_t` configuration
struct, which allows to specify reset values for the PMP configuration
and address CSRs as well as optionally making individual PMP entries
read-only.  The purpose is to allow hard-wiring of certain regions'
privileges, which is explicitly allowed by the RISC-V Privileged
Architecture specification Machine-Level ISA, v1.12 (see Sect. 3.7).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants