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

Add Zicfiss extension tests #417

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

Conversation

ved-rivos
Copy link
Contributor

@ved-rivos ved-rivos commented Dec 17, 2023

Description

Implements tests for Zicfiss extension.
Tests sspush, sspopchk, ssamoswap, ssrdp, c_sspush, and c_sspopchk instructions in S and U mode
Specification: https://github.com/riscv/riscv-cfi/blob/main/cfi_backward.adoc

Related Issues

N/A

Ratified/Unratified Extensions

  • Ratified
  • Unratified

List Extensions

Zicfiss: https://github.com/riscv/riscv-cfi/blob/main/cfi_backward.adoc

Reference Model Used

  • SAIL
  • Spike
  • Other - < SPECIFY HERE >

Mandatory Checklist:

  • All tests are compliant with the test-format spec present in this repo ?
  • Ran the new tests on RISCOF with SAIL/Spike as reference model successfully ?
  • Ran the new tests on RISCOF in coverage mode
  • Link to Google-Drive folder containing the new coverage reports (See this for more info): < SPECIFY HERE >
  • Link to PR in RISCV-ISAC from which the reports were generated : < SPECIFY HERE >
  • Changelog entry created with a minor patch

Optional Checklist:

  • RISCV-V CTG PR link if tests were generated using it : Add unratified Zicfiss extension riscv-software-src/riscv-ctg#94
  • 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.

/* 1. Push using x1; popchk using x5 */;\
/* 1. Push using x5; popchk using x1 */;\
/* 1. Push using x5; popchk using x5 */;\
/* After each test ensure ssp back to initial value */;\
Copy link
Collaborator

@allenjbaum allenjbaum Dec 28, 2023

Choose a reason for hiding this comment

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

I don't see where the check that "ssp is back to the initial value" is.
I see that you are comparing before/after, and adding the difference to x10,
but x10 (nor the difference in x11 ) isn't saved in the signature

Copy link
Contributor Author

@ved-rivos ved-rivos Dec 28, 2023

Choose a reason for hiding this comment

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

  mv swreg, x15                                            ;\
  RVTEST_GOTO_MMODE                                        ;\
  DISABLE_ZICFISS()                                        ;\
  RVTEST_SIGUPD(swreg, x10, offset) 

The last statement in this macro saves x10 to the signature area. Please see line number 1399.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, actually line 1318 for this macro, 1399 for the next.
So, you sum up all the deltas, and save that at the end. That can hide a lot of sins.
That's like testing lots of different add operation, combining all the results, and saving just that as the signature - which works, but not well (since compensating bugs can hide failures). And if there is a bug, you have no clue which invocation caused it. You need to replace "add x10, x10, x11" everywhere with "RVTEST_SIGUPD(swreg, x11) instead of summing them all up (you can't store the actual value because it might be loaded differently between reference and DUT). You would also need to make sure the explicit offset is handled correctly by changing the second ".if offset==0" to be ".if offset<6" I think. And. allocate more signature space, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed this comment. I have updated the test macro to retain full history by replacing add x10, x10, x11 with
seqz x11, x11; or x10, x10, x11; sllw x10, x10, 1

@jamesbeyond jamesbeyond changed the base branch from main to dev May 28, 2024 16:56
@ved-rivos ved-rivos changed the title Add unratified Zicfiss extension tests Add Zicfiss extension tests Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants