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

Use physical addresses for LR/SC reservations #501

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Jun 19, 2024

This is how reservation is architecturally defined, how real CPUs are likely to be implemented, and doesn't require spurious cancellations on xRET and traps.

Fixes #360.

Copy link

github-actions bot commented Jun 19, 2024

Test Results

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

Results for commit 585e6ff. ± Comparison against base commit d96f89b.

♻️ This comment has been updated with latest results.

@Timmmm Timmmm requested a review from allenjbaum July 4, 2024 13:00
@Timmmm Timmmm force-pushed the user/timh/physical_reservation_address branch from 11e63e7 to 085b37a Compare July 4, 2024 15:24
@defermelowie
Copy link
Contributor

This changes the behaviour of sc when address translation fails of an address that has no valid reservation set. In the current model the address translation is omitted and therefore no exception occurs. However after this change, the failing address translation causes an exception. Not sure which behaviour is considered "correct".

At first glance the new behaviour seems to match spike: https://github.com/riscv-software-src/riscv-isa-sim/blob/98d2c29e431f3b14feefbda48c5f70c2f451acf2/riscv/mmu.h#L266

@Alasdair
Copy link
Collaborator

Alasdair commented Jul 8, 2024

I suspect the answer is it's implementation defined what happens. Right now the RISC-V axiomatic memory model in the ISA manual does not specify anything regarding virtual memory.

If we do end up with an axiomatic model with virtual memory, we might need something like load_reservation(vAddr, pAddr) to capture all the allowed behaviors, assuming implementations have a choice in how they implement LR and SC.

* For now we set them on virtual addresses, since it makes the
* sequential model of SC a bit simpler, at the cost of an explicit
* call to load_reservation in LR and cancel_reservation in SC.
* However the reservation is architecturally defined in terms of physical
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this actually the case? The LR and SC section of the unpriv spec never uses the word physical. "a set of bytes that subsumes the bytes in the addressed word." seems intentionally fuzzy to allow a range of implementations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OTOH, when I search the priv spec: "For implementations with both page-based virtual memory and the "A" standard extension, the LR/SC
reservation set must lie completely within a single base physical page (i.e., a naturally aligned 4 KiB
physical-memory region)."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is based on this comment; maybe the language is a little too strong for something in an second hand email... Maybe I should just remove that comment? Though the quote you found does indirectly support it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok comment removed.

This is how reservation is architecturally defined, how real CPUs are likely to be implemented, and doesn't require spurious cancellations on xRET and traps.

Fixes riscv#360.
@Timmmm Timmmm force-pushed the user/timh/physical_reservation_address branch from 085b37a to 585e6ff Compare July 15, 2024 14:58
@Timmmm
Copy link
Collaborator Author

Timmmm commented Jul 15, 2024

@billmcspadden-riscv ok this is ready now.

@billmcspadden-riscv billmcspadden-riscv merged commit b6ca03a into riscv:master Jul 15, 2024
2 checks passed
@Timmmm Timmmm deleted the user/timh/physical_reservation_address branch August 30, 2024 15:33
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.

LR/SC reservation is based on virtual addresses not physical
4 participants