Skip to content

cfi: fix ordering of textual SSPUSH behavior, clarify ordering.#2782

Draft
dtzSiFive wants to merge 1 commit intoriscv:mainfrom
dtzSiFive:fix/sspush_ssp_after_store
Draft

cfi: fix ordering of textual SSPUSH behavior, clarify ordering.#2782
dtzSiFive wants to merge 1 commit intoriscv:mainfrom
dtzSiFive:fix/sspush_ssp_after_store

Conversation

@dtzSiFive
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions
Copy link
Copy Markdown

Normative Rule Changes Detected

This PR modifies normatively tagged text. Please review the changes below to ensure they are intentional.

View Detected Changes

Normative Tag Change Report

riscv-spec Specification

================================================================================
Tag Changes Report
================================================================================

Reference file: ref/riscv-spec-norm-tags.json
Current file: build/riscv-spec-norm-tags.json
Modified 2 tags:
  * "norm:ss_push":
      Reference: "A shadow stack push operation is defined as decrement of the ssp by XLEN/8 followed by a store of th..."
      Current:   "A shadow stack push operation is defined as a store of the value in the link register to memory at o..."
  * "norm:sspush_c-sspush_decrement":
      Reference: "The ssp is decremented by SSPUSH and C.SSPUSH only if the store to the shadow stack completes succes..."
      Current:   "The ssp is decremented by SSPUSH and C.SSPUSH only if it is determined that the store to the shadow ..."

================================================================================
Summary: 2 total changes
  Added:    0
  Deleted:  0
  Modified: 2
================================================================================

What happens next:

  • This comment is informational only and does not block merging
  • When this PR is merged, a GitHub issue will be automatically created with the NormRules label for CSC tracking
  • If these changes are unintentional, please update the PR before merging

How to update reference files (if needed):

make update-ref
git add ref/*.json
git commit -m "Update normative tag reference files"

Note: New tags (additions) are automatically added to the reference files when PRs are merged to main. Only modifications and deletions require manual review.

This comment was automatically generated by the normative tag check workflow.

Comment thread src/unpriv-cfi.adoc
of the shadow stack.
A shadow stack push operation is defined as a store of the value in the link
register to memory at one level down from the top of the shadow stack, which is
`ssp - XLEN/8`, followed by updating `ssp` to this address.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this actually changes the memory ordering implications. Either way, the store depends on ssp. It doesn't really matter whether it depends on the incremented value or the original value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The main change intent is to not indicate ssp is decremented first "followed by" the store, when the code listing has them in the other order.

But okay I see what you mean, if the update to ssp can be said to precede the store but doesn't commit until after regardless this change is unnecessary.

Comment thread src/unpriv-cfi.adoc
Comment on lines +474 to +479
imply ordering: subsequent `SSPUSH` and `C.SSPUSH` instructions may be globally
visible before earlier `SSPUSH` and `C.SSPUSH` instructions, despite
potentially having an address dependency on the update to `ssp`. The decrement
of `ssp` and the store to the shadow stack are not ordered with respect to each
other; the store may or may not be globally visible before the decrement of
`ssp`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is true, but if this follows from RVWMO's handling of implicit CSR accesses (which I think it does, but I haven't confirmed it), then I'd rather not add any new text. I'll leave this open for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Aye, I'll check and agreed. Thanks!

@dtzSiFive dtzSiFive marked this pull request as draft March 25, 2026 23:11
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.

2 participants