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

Make CRE not a WARL field #332

Merged
merged 8 commits into from
Jul 30, 2024
Merged

Make CRE not a WARL field #332

merged 8 commits into from
Jul 30, 2024

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Jul 23, 2024

Change the CRE bits so they are always writable, but the effective CRE depends on multiple values. This is significantly simpler to specify, avoids an entire cross-CSR WARL field (WARL fields are a pain), and removes some undefined behaviour (do the bits retain their values when they are read-only zero).

Fixes #331

Note, I deleted the text about SXLEN/UXLEN. Those didn't seem fully specified tbh, but I'll need to add them back somehow.

Change the CRE bits so they are always writable, but the effective CRE depends on multiple values. This is significantly simpler to specify, avoids an entire cross-CSR WARL field (WARL fields are a pain), and removes some undefined behaviour (do the bits retain their values when they are read-only zero).

Fixes riscv#331
Copy link
Collaborator

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

As noted in #332, I'm fine with this change but I'd wait for others to comment before merging.

A few small comments inline.

src/attributes.adoc Outdated Show resolved Hide resolved
src/insns/require_cre.adoc Outdated Show resolved Hide resolved
src/attributes.adoc Outdated Show resolved Hide resolved
src/riscv-hybrid-integration.adoc Outdated Show resolved Hide resolved
src/riscv-hybrid-integration.adoc Outdated Show resolved Hide resolved
@buxtonpaul
Copy link
Contributor

Agree this makes implementation simpler (at least in Qemu), and addresses the undefined behaviour regarding if bits are cleared. But there still isn't a way for S/U mode code to know what the effective CRE is as it is dependent on registers they cannot read. Unless they try an instruction and catch the resulting exception.

tariqkurd-repo and others added 3 commits July 24, 2024 11:54
Co-authored-by: Alexander Richardson <[email protected]>
Signed-off-by: Tariq Kurd <[email protected]>
Co-authored-by: Alexander Richardson <[email protected]>
Signed-off-by: Tariq Kurd <[email protected]>
Co-authored-by: Alexander Richardson <[email protected]>
Signed-off-by: Tariq Kurd <[email protected]>
@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 24, 2024

Agree this makes implementation simpler (at least in Qemu), and addresses the undefined behaviour regarding if bits are cleared. But there still isn't a way for S/U mode code to know what the effective CRE is as it is dependent on registers they cannot read. Unless they try an instruction and catch the resulting exception.

That’s what the device tree or ACPI whatever is for.

@buxtonpaul
Copy link
Contributor

Agree this makes implementation simpler (at least in Qemu), and addresses the undefined behaviour regarding if bits are cleared. But there still isn't a way for S/U mode code to know what the effective CRE is as it is dependent on registers they cannot read. Unless they try an instruction and catch the resulting exception.

That’s what the device tree or ACPI whatever is for.
That makes sense, I guess there aren't many use cases for dynamically changing the CRE for a running system.

@Timmmm Timmmm changed the title Make CRE not a WARL field (draft) Make CRE not a WARL field Jul 25, 2024
@Timmmm
Copy link
Contributor Author

Timmmm commented Jul 25, 2024

I've updated it so now the text talks about "CHERI register access" being enabled/disabled rather than the CRE bits or effective CRE, because the former takes account of mstatus.MBE and XLEN<MXLEN which can also disable CHERI register access. Hopefully this makes sense!

src/insns/require_cre.adoc Outdated Show resolved Hide resolved
src/riscv-hybrid-integration.adoc Outdated Show resolved Hide resolved
Timmmm and others added 2 commits July 30, 2024 10:14
Co-authored-by: Andres Amaya Garcia <[email protected]>
Signed-off-by: Tim Hutt <[email protected]>
Co-authored-by: Andres Amaya Garcia <[email protected]>
Signed-off-by: Tim Hutt <[email protected]>
@andresag01 andresag01 merged commit 8b1b3b9 into riscv:main Jul 30, 2024
3 checks passed
tariqkurd-repo added a commit to tariqkurd-repo/riscv-cheri that referenced this pull request Oct 9, 2024
Change the CRE bits so they are always writable, but the effective CRE
depends on multiple values. This is significantly simpler to specify,
avoids an entire cross-CSR WARL field (WARL fields are a pain), and
removes some undefined behaviour (do the bits retain their values when
they are read-only zero).

Fixes riscv#331

Note, I deleted the text about SXLEN/UXLEN. Those didn't seem fully
specified tbh, but I'll need to add them back somehow.

---------

Signed-off-by: Tariq Kurd <[email protected]>
Signed-off-by: Tim Hutt <[email protected]>
Co-authored-by: Tariq Kurd <[email protected]>
Co-authored-by: Alexander Richardson <[email protected]>
Co-authored-by: Andres Amaya Garcia <[email protected]>
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.

Does senvcfg.CRE retain its value?
6 participants