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

Mark c0-authorized loads and stores as extensible #403

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

nwf
Copy link
Collaborator

@nwf nwf commented Oct 2, 2024

Say explicitly that c0-authorized load/store instructions, which are guaranteed to trap regardless of other operands' values, are available for use by future extensions.

@jrtc27
Copy link
Collaborator

jrtc27 commented Oct 2, 2024

Why? They’re not for the base ISA.

@nwf
Copy link
Collaborator Author

nwf commented Oct 3, 2024

They stand a chance of working in the base ISA (ETA: or in Zcherihybrid), if there's memory near 0. This is specifically for instructions in cap mode.

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.

If you plan to repurposed this "dead" opcode, I wonder if it would be better to have them raise an illegal instruction? Although that would require special casing in the decoder so maybe the current behaviour makes more sense?

@tariqkurd-repo
Copy link
Collaborator

@nwf is pointing out that if the auth cap is c0 you definitely get a tag violation, allowing future extensions to redefine that case, as oppose to it having to be a tag violation for all time.

Do we need to cal this out explicitly? I suppose we do to prevent it always being a tag violation.

@nwf
Copy link
Collaborator Author

nwf commented Oct 3, 2024

@arichardson Yeah, I agree that in some sense "illegal instruction" is "better" than "CHERI tag violation" if the intent is to allow the instruction to be repurposed (and yes, indeed), but I don't think we need to mandate that complexity in the decoder. It's perfectly fine, IMHO, for a Zcheripurecap core to really try operating on the NULL cap here while also allowing extensions to operate on something else (by way of concrete examples, perhaps, DDC like we did back in CHERI-RISC-V or the Zstid *tidc registers).

@tariqkurd-repo I do not know how RISC-V in general feels about extensions replacing instructions without permission of the specifications they extend, but that seems kind of rude. It seemed more polite, if nothing else, to explicitly grant permission to do so. :)

@tariqkurd-repo
Copy link
Collaborator

I'll note that there is more redundancy in the encodings as the load/store addresses must be 16-byte aligned and so the 4 LSBs (1/3) of the immediate are wasted, as well as c0 being useless.

It would be better to make all of those cases reserved.

I personally think these things will come out of ARC review when we get to that stage, and they allocate encodings.

@davidchisnall
Copy link

I'll note that there is more redundancy in the encodings as the load/store addresses must be 16-byte aligned and so the 4 LSBs (1/3) of the immediate are wasted, as well as c0 being useless.

This is not wasted space. The alignment of the address + immediate needs to be 16-byte aligned, but there's no guarantee that the address is already 16-byte aligned.

In particular, it is very common in managed languages to have tagged pointers where the low 3 bits store type information. This can then be reset to zero in the load / store instructions with a -1 to -7 displacement added to the field offset.

@nwf
Copy link
Collaborator Author

nwf commented Oct 3, 2024

Just to make sure it isn't missed... In addition to the clever trick David's pointing out, this marking as extensible is intended to apply to more than just high-alignment-requiring LC/SC instructions (including LB/SB, which impose no alignment requirements).

@tariqkurd-repo
Copy link
Collaborator

thanks @davidchisnall I didn't know those bits would be used that way, so if they don't offer those bits when we finally get allocated encodings, we should point to that use case

@nwf I guess you're saying that load/stores via c0 can be replaced by anything you like

@nwf
Copy link
Collaborator Author

nwf commented Oct 3, 2024

@tariqkurd-repo Yes, though I imagine they'll be replaced with load/stores still, since all the decoder logic is right there, but in principle...

@davidchisnall
Copy link

thanks @davidchisnall I didn't know those bits would be used that way, so if they don't offer those bits when we finally get allocated encodings, we should point to that use case

There should be some rationale in the base RISC-V spec. We had this discussion 10 years ago there as well.

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.

Could you add cs0 != 0 to the encoding diagrams?

src/insns/load_store_c0.adoc Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@nwf nwf left a comment

Choose a reason for hiding this comment

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

The changes LGTM, just one bit of grammatical nit-picking.

src/insns/load_store_c0.adoc Outdated Show resolved Hide resolved
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.

LGTM

tariqkurd-repo and others added 2 commits October 15, 2024 10:28
Co-authored-by: Nathaniel Wesley Filardo <[email protected]>
Signed-off-by: Tariq Kurd <[email protected]>
@tariqkurd-repo tariqkurd-repo merged commit cf7e656 into riscv:main Oct 15, 2024
3 checks passed
@nwf nwf deleted the 202410-reserve-ls-c0 branch October 19, 2024 22:52
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.

5 participants