Skip to content

Tests and Coverpoints for the LR/SC instructions of Atomic extension #600

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

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

itsubaidrehman
Copy link

Description

Adds tests and coverpoints for LR/SC instructions in Atomic (A) Extension. Updated the arch_test.h to include the macros to run the LR/SC tests.

Related Issues

N/A

Ratified/Unratified Extensions

  • Ratified
  • Unratified

List Extensions

Atomic Extension (A)

Reference Model Used

  • SAIL
  • Spike
  • Other - < SPECIFY HERE >

Mandatory Checklist:

Optional Checklist:

  • Were the tests hand-written/modified ?
  • Have you run these on any hard DUT model ? Please specify name and provide link if possible in the description
  • If you have modified arch_test.h Please provide a detailed description of the changes in the Description section above.

@davidharrishmc
Copy link
Collaborator

Here is some feedback from a review of the testplan shared by Muhammad Maarij (10xEngineers) at
https://docs.google.com/spreadsheets/d/1kYHKfZmSDmKRlSjKyRu-vJVfNVUDYSDgLT42q_Obbxw/edit?gid=0#gid=0

I have not reviewed whether these tests implement the testplan or whether any of the concerns about the testplan have been resolved by the tests.

Aligned LR/SC Success: “matching reservation sets” is unclear. Do you mean exactly matching addresses? The size of the reservation set is implementation-dependent. The comment about testing at the extreme of granularity probably belongs here, and the one just outside granularity belongs in SC Fails.

Misaligned Exceptions: these require Zicsr. Are you separating out tests that require privileged support from tests that can run without Zicsr?

LR and SC can also generate access faults, but I don’t see entries.

SC Fails - Another SC: This is interesting to test both with SC2 the same address as well as a different one in the same set and a different one in a different set.

LR/SC normal order: I don’t know how you check “LR/SC sequences can occur before or after surrounding memory ops”

Acquire/Release semantics/Sequenntially Consistent: I don’t know how you check no ops observed before/after.

Empty LR/SC Sequence: what does “no instructions” mean? No LR and SC? Or nothing between them? How do you check for observable ordering constraints?

Constrained LR/SC Loop success: this would need flushing out to define what goes beween the LR and SC. In particular, I think having a load and a store to the reservation set and to a different address is an interesting one to check.

Constrained Loop Failure - Unsuccessful SC: if SC continues to fail, how do you exit the loop?

Constrained Loop - Max Instructions: I’m not sure what this tests? That a particular loop with 16 instructions always succeeds? Be specific about what is in the loop, besides 16 instructions.

Constrained loop > Max Instructions: How does a test determine whether it may succeed on some attempts? I don’t think there is anything we can learn from this test - if it may or may not succeed, all behaviors are legal, and it can't catch any bugs in the design, so isn’t worth performing.

Unconstrained LR/SC: Again, what bug could this catch?

SC Fail - store from another hart: This is only testable in a multicore system, and probably belongs in a different testplan than single-core LR/SC. The spec technically calls for “no write from a device other than the hart” so it is not just normal stores, but also sc or cboz or floating or compressed stores, as well as peripheral accesses that should be tested.

I think there are other easy architectural things that should be tested, such as lr and sc should be able to use every register for source/destinations (except rs1=x0), lr can read zero and non-zero values from memory and sc and write the same. lr.w on RV64 should sign-extend properly. Both .w and .d flavors need testing.

@jamesbeyond jamesbeyond added unprivileged ratified ratified specs size-M Medium efforts required labels Feb 24, 2025
@allenjbaum
Copy link
Collaborator

allenjbaum commented Feb 26, 2025 via email

@pmundkur
Copy link
Contributor

pmundkur commented Mar 5, 2025

There doesn't seem to be a test sequence with two (or more) LRs followed by an SC. The LRs may or may not be to overlapping reservations. The spec requires implies there is only one (or at most one) reservation set active at a time. It would be good to have a test for that.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Mar 5, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ratified ratified specs size-M Medium efforts required unprivileged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants