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.

Sign up for GitHub

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

Jump to bottom

Add Svinval extension. #463

Merged
merged 5 commits into from
May 8, 2024
Merged

Add Svinval extension. #463

merged 5 commits into from
May 8, 2024

Conversation

martinberger
Copy link
Collaborator

@martinberger martinberger commented Apr 29, 2024 •
edited
Loading

These changes add the "Svinval" Standard Extension for Fine-Grained
Address-Translation Cache Invalidation, Version 1.0 to the sail-riscv model.

This extension defines five new instructions: SINVAL.VMA, SFENCE.W.INVAL,
SFENCE.INVAL.IR, HINVAL.VVMA, HINVAL.GVMA.

HINVAL.VVMA & HINVAL.GVMA are omitted since they build on the
Hypervisor Extension which is yet to be included in the model.

SFENCE.W.INVAL & SFENCE.INVAL.IR are treated as nops pending integration
of the coherency model (rmem) with sail.

The specification says that SINVAL.VMA behaves just as SFENCE.VMA,
except there are additional ordering constraints with respect to the new
SFENCE.W.INVAL & SFENCE.INVAL.IR instructions. Since these are nops, we
can treat SINVAL.VMA as if it were SFENCE.VMA.

@martinberger
Add Svinval extension.
ff98dcc 
    These changes add the "Svinval" Standard Extension for Fine-Grained
    Address-Translation Cache Invalidation, Version 1.0 to the sail-riscv model.

    This extension defines five new instructions: SINVAL.VMA, SFENCE.W.INVAL,
    SFENCE.INVAL.IR, HINVAL.VVMA, HINVAL.GVMA.

    HINVAL.VVMA & HINVAL.GVMA are omitted since they build on the
    Hypervisor Extension which is yet to be included in the model.

    SFENCE.W.INVAL & SFENCE.INVAL.IR are treated as nops pending integration
    of the coherency model (rmem) with sail.

    The specification says that SINVAL.VMA behaves just as SFENCE.VMA,
    except there are additional ordering constraints with respect to the new
    SFENCE.W.INVAL & SFENCE.INVAL.IR instructions. Since these are nops, we
    can treat SINVAL.VMA as if it were SFENCE.VMA.
@martinberger
fixup! Add Svinval extension.
f06a04e
@martinberger
fixup! fixup! Add Svinval extension.
db931c4
@martinberger
Copy link
Collaborator Author

martinberger commented Apr 29, 2024 •
edited
Loading

This is essentially the same PR as #297 from @kristinbarber but I've added the missing haveSvinval() gates (and configuration support) in the encdec functions. I've opened a new PR, because I can't push to Kristin's https://github.com/kristinbarber/sail-riscv/tree/svinval

@martinberger
Copy link
Collaborator Author

martinberger commented Apr 29, 2024 •
edited
Loading

One question I have is: should the new commands for Svinval be in a separate riscv_insts_svinval.sail file?

@martinberger martinberger mentioned this pull request Apr 29, 2024
Closed
@jrtc27
Copy link
Collaborator

jrtc27 commented Apr 29, 2024

One question I have is: should the new commands for Svinval be in a separate riscv_insts_svinval.sail file?

Probably. One extension per file is an easy clear-cut rule (until you ask "which extension?") rather than having to debate it every time.

If you've copied code you should have Co-authored-by: as a trailer in your commit message.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 29, 2024 •
edited
Loading

Test Results

712 tests  ±0   712 ✅ ±0   0s ⏱️ ±0s
  6 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 41071ea. ± Comparison against base commit c5ee87d.

♻️ This comment has been updated with latest results.

arichardson
arichardson reviewed Apr 30, 2024
View reviewed changes
c_emulator/riscv_sim.c Outdated Show resolved Hide resolved
arichardson
arichardson reviewed Apr 30, 2024
View reviewed changes
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.

The original PR had comments explaining that implementing these as NOPs is legal for the model, which I think probably makes sense to keep.

Other than this and the one minor comment on the #define this looks good to me.

@martinberger
Add Svinval extension.
41071ea 
These changes add the "Svinval" Standard Extension for Fine-Grained
Address-Translation Cache Invalidation, Version 1.0 to the sail-riscv
model.

This extension defines five new instructions: SINVAL.VMA,
SFENCE.W.INVAL, SFENCE.INVAL.IR, HINVAL.VVMA, HINVAL.GVMA.

HINVAL.VVMA & HINVAL.GVMA are omitted since they build on the
Hypervisor Extension which is yet to be included in the model.

SFENCE.W.INVAL & SFENCE.INVAL.IR are treated as nops pending
integration of the coherency model (rmem) with sail.

The specification says that SINVAL.VMA behaves just as SFENCE.VMA,
except there are additional ordering constraints with respect to the
new SFENCE.W.INVAL & SFENCE.INVAL.IR instructions. Since these are
nops, we can treat SINVAL.VMA as if it were SFENCE.VMA.

Co-authored-by: Kristin Barber <kristinbarber@google.com>
@martinberger
Copy link
Collaborator Author

The original PR had comments explaining that implementing these as NOPs is legal for the model, which I think probably makes sense to keep.

Added short comments about NOPs

@martinberger
Copy link
Collaborator Author

If you've copied code you should have Co-authored-by: as a trailer in your commit message.

Added Kristin as coauthor.

@Alasdair Alasdair self-requested a review May 8, 2024 00:48
Alasdair
Alasdair approved these changes May 8, 2024
View reviewed changes
Copy link
Collaborator

@Alasdair Alasdair left a comment

Choose a reason for hiding this comment

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

Since there isn't much in terms of implementation, I checked the encoding of each instruction against the PDF spec and they look fine.

Once we merge this, I can add the cache invalidate operations that actually interface with our weak memory tooling (once my RVWMO PR is merged), although we don't have a particularly clear semantics for these operations they would at least be visible in our tooling.

@billmcspadden-riscv billmcspadden-riscv merged commit 5c55b5b into master May 8, 2024
3 checks passed
@Alasdair
Copy link
Collaborator

Alasdair commented May 8, 2024

Would have been better to squash merge this so all the fixup commits weren't visible I think.

@martinberger martinberger deleted the svinval-mb branch May 8, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arichardson arichardson arichardson left review comments

@Alasdair Alasdair Alasdair approved these changes

@billmcspadden-riscv billmcspadden-riscv Awaiting requested review from billmcspadden-riscv

@jrtc27 jrtc27 Awaiting requested review from jrtc27

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martinberger @jrtc27 @Alasdair @arichardson @billmcspadden-riscv