Add changes in arch_test.h file, split macros into 2 files and enable multiple Privilege Modes in Trap Handler #277

UmerShahidengr · 2022-08-14T12:03:20Z

This PR is co-authored-by: allenjbaum [email protected], and modification of PR-267.
As requested by @pawks, the trap handler and macro definitions have been split into two different files, arch_test.h contains definitions of trap handler, macros to change privilege modes, and other necessary macros while macros to manage storing signatures, and ops dependent general macros are defined in test_macros.h file.
The major changes with respect to PR-267 are as follows:

Removed the bugs in RVTEST_GOTO_LOWER_MODE macro
Removed the bugs in RVTEST_GOTO_MMODE macro and defined strap_routine directive.
Updated RVTEST_TRAP_SAVEAREA and RVTEST_TRAP_EPILOG macros to refine the definitions of per mode save area macros.

riscv-test-suite/env/arch_test.h

UmerShahidengr · 2022-09-15T18:29:16Z

Hello @pawks, @allenjbaum and I have updated the trap handler, and now it is working perfectly. Please review and merge it

InspireSemi

In test_macros.h
The #warnings have been commented out. Either we uncomment these or just remove the this whole section from the file. Lines 768 - 818.
These macros have been deprecated and now is as good a time as any to remove them.

allenjbaum · 2022-09-20T18:22:55Z

I'm unclear what has been deprecated; do you mean that #warning has been deprecated in the toolchain?

…

On Tue, Sep 20, 2022 at 7:12 AM InspireSemi ***@***.***> wrote: ***@***.**** requested changes on this pull request. In test_macros.h The #warnings have been commented out. Either we uncomment these or just remove the this whole section from the file. Lines 768 - 818. These macros have been deprecated and now is as good a time as any to remove them. — Reply to this email directly, view it on GitHub <#277 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJUCZERQXXUX5DVEU3DV7HA4RANCNFSM56PYCGHQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

InspireSemi · 2022-09-20T18:25:35Z

The macros in this section of the header are marked as being deprecated. Someone commented out the #warning messages that say this macro is deprecated since 3.0…. So either uncomment the #warning messages and leave the macros in place or remove the whole block of macros. Marc Karasek Principal Software Engineer M: 678.770.3788 ***@***.*** www.inspiresemi.com<http://www.cryptocoretech.com/> THIS MESSAGE IS INTENDED ONLY FOR THE USE OF THE INDIVIDUAL OR ENTITY TO WHICH IT IS ADDRESSED AND MAY CONTAIN INFORMATION THAT IS PRIVILEGED, CONFIDENTIAL AND/OR EXEMPT FROM DISCLOSURE UNDER APPLICABLE LAW. If the reader of this message is not the intended recipient or agent responsible for delivering the message to the intended recipient, then you are hereby notified that any dissemination or copying of this communication is strictly prohibited. If you have received this electronic transmission in error, please delete it from your system without copying it and notify the sender by reply e-mail so that our address record can be corrected. Thank you. From: Allen Baum ***@***.***> Sent: Tuesday, September 20, 2022 2:23 PM To: riscv-non-isa/riscv-arch-test ***@***.***> Cc: Marc Karasek ***@***.***>; Comment ***@***.***> Subject: Re: [riscv-non-isa/riscv-arch-test] Add changes in arch_test.h file, split macros into 2 files and enable multiple Privilege Modes in Trap Handler (PR #277) I'm unclear what has been deprecated; do you mean that #warning has been deprecated in the toolchain?

On Tue, Sep 20, 2022 at 7:12 AM InspireSemi ***@***.***<mailto:***@***.***>> wrote: ***@***.****<mailto:***@***.****> requested changes on this pull request. In test_macros.h The #warnings have been commented out. Either we uncomment these or just remove the this whole section from the file. Lines 768 - 818. These macros have been deprecated and now is as good a time as any to remove them. — Reply to this email directly, view it on GitHub <#277 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJUCZERQXXUX5DVEU3DV7HA4RANCNFSM56PYCGHQ> . You are receiving this because you were mentioned.Message ID: ***@***.***<mailto:***@***.***>>

— Reply to this email directly, view it on GitHub<#277 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AR3S6YZ5ZUGV7NHPMDXHBW3V7H6IVANCNFSM56PYCGHQ>. You are receiving this because you commented.Message ID: ***@***.******@***.***>>

allenjbaum · 2022-09-20T18:36:05Z

Ah, those macros. I thought you meant some other macros that use #warning Yes, I think we can remove line 768 to the end of the file at line 818

UmerShahidengr · 2022-11-10T04:25:38Z

Hello,
As sent by @allenjbaum , this final commit is ready to be merged. Allen found and fixed a bunch of bugs related to:

code paths I haven't exercised that have vectored interrupt modes
better indications of when the trap signature area is overrun
ensuring that the trap handler keeps the trampoline 64B aligned (need for CLIC spec)
fixing the handling of when xTVEC was not initialized and also could not be written
removing code that was compensating for Sail bugs
rewriting the save_GPR macro to it now compiles
some miscellaneous optimizations
comment fixes

neelgala · 2022-11-10T05:42:28Z

A changelog entry is required. Please make this a "minor" update and in the Changelog be as detailed as possible in terms of the changes and feature additions that this PR introduces.

UmerShahidengr · 2022-11-10T06:26:40Z

A changelog entry is required. Please make this a "minor" update and in the Changelog be as detailed as possible in terms of the changes and feature additions that this PR introduces.

Done.

pawks · 2022-11-10T07:25:18Z

There are some merge conflicts between the arch_test.h files on the main and the PR. Please pull the recent changes from main and resolve the conflicts correctly.

UmerShahidengr · 2022-11-10T13:04:39Z

There are some merge conflicts between the arch_test.h files on the main and the PR. Please pull the recent changes from main and resolve the conflicts correctly.

Done!

pawks · 2022-11-10T16:01:43Z

The changes on the master are being overwritten. Especially the canary definitions here.

…ifferently in tests and defined differently in latest trap handler

UmerShahidengr · 2022-11-10T17:01:29Z

The changes on the master are being overwritten. Especially the canary definitions here.

The CANARY error has been resolved. Now all (90) tests of RV32 are passing with the updated definition of CANARY.

riscv-test-suite/env/arch_test.h

riscv-test-suite/env/test_macros.h

riscv-test-suite/env/arch_test.h

UmerShahidengr · 2022-11-22T09:31:52Z

Added changes proposed in #294 into this PR.

UmerShahidengr · 2022-11-24T08:18:24Z

Were you able to run the tests under rv64i_m/C directory? With the latest changes, all C extension tests fail to compile. This is probably because of a .option norvc in the template macros which is not clubbed with a .option push ... .option pop sequence. An instance of the error is shown below:
Error: unrecognized opcode `c.addi16sp x2,0x100'

The tests were passing at my side, but before commit, I made one random change in LI macro (I swapped the places of .option pull and .endif in LI macro) which caused this problem for all C type tests. The file has been refreshed and all C type tests are now passing

…ifferent address mappings

Signed-off-by: Umer Shahid <[email protected]>

… from outside code region + SET_REL_TVAL_MSK & CODE_REL_TVAL_MSK has been updated + some minor fixes

riscv-test-suite/env/test_macros.h

allenjbaum · 2022-12-23T08:14:50Z

An overflow?Not sure what that means. The code above (or below, I guess) is not the correct fix. All Instances of SREG or FSREG, offset+SIGALIGN(_BR) should be changed to FSREG, offset(_BR) because the CHK_OFFSET(_BR,SIGALIGN,1) pre-increment the offset.

…

On Thu, Dec 22, 2022 at 9:17 PM S Pawan Kumar ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In riscv-test-suite/env/test_macros.h <#277 (comment)> : > +/* It stores both an Xreg and an Freg, first adjusting base & offset to */ +/* to keep offset < 2048. SIGALIGN is set to the max(FREGWIDTH, REGWIDTH)*/ +/* _BR - Base Reg, _R - FReg, _F - Fstatus Xreg */ +#define RVTEST_SIGUPD_F(_BR,_R,_F,...) ;\ + .if NARG(__VA_ARGS__) == 1 ;\ + .set offset,_ARG1(__VA_OPT__(__VA_ARGS__,0)) ;\ + .endif ;\ + .if (offset&(SIGALIGN-1))!=0 ;\ +/* Throw warnings then modify offset to target */ ;\ + .warning "Incorrect signature Offset Alignment." ;\ + .set offset, offset&(SIGALIGN-1)+SIGALIGN ;\ + .endif ;\ + CHK_OFFSET(_BR, SIGALIGN, 0) ;\ + FSREG _R,offset(_BR) ;\ + CHK_OFFSET(_BR, SIGALIGN, 1) ;\ + SREG _F,offset+SIGALIGN(_BR) ;\ The latest macros on the main have been fixed(link <https://github.com/riscv-non-isa/riscv-arch-test/blob/e302d3bab41a46ec388691b1d961aa09fe2a4bc4/riscv-test-suite/env/arch_test.h#L661>). By introducing the offset+SIGALIGN the tests do not compile because an overflow occurs after a few instances of the macro in the test, specifically at line 106. In its current form, it also results in the flag values of preceding test instance getting overwritten by the result of the next instance. — Reply to this email directly, view it on GitHub <#277 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJRNWILXUQCILOPA3FDWOUYV5ANCNFSM56PYCGHQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

allenjbaum · 2022-12-23T08:16:47Z

More correctly, in case this is misinterpreted: All Instances of SREG/FSREG, offset+SIGALIGN(_BR) should be changed to SREG/FSREG, offset(_BR) On Fri, Dec 23, 2022 at 12:14 AM Allen Baum ***@***.***> wrote:

…

An overflow?Not sure what that means. The code above (or below, I guess) is not the correct fix. All Instances of SREG or FSREG, offset+SIGALIGN(_BR) should be changed to FSREG, offset(_BR) because the CHK_OFFSET(_BR,SIGALIGN,1) pre-increment the offset. On Thu, Dec 22, 2022 at 9:17 PM S Pawan Kumar ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In riscv-test-suite/env/test_macros.h > <#277 (comment)> > : > > > +/* It stores both an Xreg and an Freg, first adjusting base & offset to */ > +/* to keep offset < 2048. SIGALIGN is set to the max(FREGWIDTH, REGWIDTH)*/ > +/* _BR - Base Reg, _R - FReg, _F - Fstatus Xreg */ > +#define RVTEST_SIGUPD_F(_BR,_R,_F,...) ;\ > + .if NARG(__VA_ARGS__) == 1 ;\ > + .set offset,_ARG1(__VA_OPT__(__VA_ARGS__,0)) ;\ > + .endif ;\ > + .if (offset&(SIGALIGN-1))!=0 ;\ > +/* Throw warnings then modify offset to target */ ;\ > + .warning "Incorrect signature Offset Alignment." ;\ > + .set offset, offset&(SIGALIGN-1)+SIGALIGN ;\ > + .endif ;\ > + CHK_OFFSET(_BR, SIGALIGN, 0) ;\ > + FSREG _R,offset(_BR) ;\ > + CHK_OFFSET(_BR, SIGALIGN, 1) ;\ > + SREG _F,offset+SIGALIGN(_BR) ;\ > > The latest macros on the main have been fixed(link > <https://github.com/riscv-non-isa/riscv-arch-test/blob/e302d3bab41a46ec388691b1d961aa09fe2a4bc4/riscv-test-suite/env/arch_test.h#L661>). > By introducing the offset+SIGALIGN the tests do not compile because an > overflow occurs after a few instances of the macro in the test, > specifically at line 106. In its current form, it also results in the flag > values of preceding test instance getting overwritten by the result of the > next instance. > > — > Reply to this email directly, view it on GitHub > <#277 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AHPXVJRNWILXUQCILOPA3FDWOUYV5ANCNFSM56PYCGHQ> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

…ted in CHK_OFFSET

Signed-off-by: S Pawan Kumar <[email protected]>

…into main

allenjbaum

There are numerous problems with this fix. There are some fixes, also, but this isn't ready for prime time.

pawks

All changes look good to me. The current tests pass. LGTM

UmerShahidengr changed the title ~~Updated arch_test.h file~~ Add changes in arch_test.h file, split macros into 2 files and enable multiple Privilege Modes in Trap Handler Aug 14, 2022

pawks requested changes Aug 16, 2022

View reviewed changes

riscv-test-suite/env/arch_test.h Outdated Show resolved Hide resolved

riscv-test-suite/env/arch_test.h Outdated Show resolved Hide resolved

pawks reviewed Aug 16, 2022

View reviewed changes

riscv-test-suite/env/arch_test.h Outdated Show resolved Hide resolved

UmerShahidengr requested a review from pawks August 17, 2022 09:21

UmerShahidengr closed this Sep 15, 2022

UmerShahidengr force-pushed the main branch from 4d78fe3 to da638ba Compare September 15, 2022 06:28

UmerShahidengr reopened this Sep 15, 2022

InspireSemi suggested changes Sep 20, 2022

View reviewed changes

bilalsakhawat mentioned this pull request Nov 9, 2022

Add changes to enable multiple Privilege Modes in Trap Handler #267

Closed

UmerShahidengr closed this Nov 10, 2022

UmerShahidengr force-pushed the main branch from 72f18d0 to b8e2315 Compare November 10, 2022 12:57

Resolved conflict, now it is ready to be merged with main

f0bcd22

UmerShahidengr reopened this Nov 10, 2022

Resolved issue related to CANARY (I didnt know CANARY is being used d…

cb44729

…ifferently in tests and defined differently in latest trap handler

pawks requested changes Nov 19, 2022

View reviewed changes

pawks reviewed Nov 19, 2022

View reviewed changes

riscv-test-suite/env/arch_test.h Outdated Show resolved Hide resolved

UmerShahidengr added 2 commits November 22, 2022 14:03

Patch up on all comments on Nov16,2022

6c4b5d5

Patch for CANARY definition PR#294

ce55f4c

UmerShahidengr added 4 commits December 19, 2022 13:47

Bug Fixes ==> All tests are passing even if Sail and DUT are having d…

3435e8c

…ifferent address mappings

Conflict Resolving ==> Updated changelog

3b357ab

Merge branch 'main' into main

b697512

Signed-off-by: Umer Shahid <[email protected]>

Bug Fixes ==> Adjusted mepc & mtval if instruction-access-fault comes…

7767e56

… from outside code region + SET_REL_TVAL_MSK & CODE_REL_TVAL_MSK has been updated + some minor fixes

pawks reviewed Dec 22, 2022

View reviewed changes

riscv-test-suite/env/test_macros.h Outdated Show resolved Hide resolved

pawks reviewed Dec 22, 2022

View reviewed changes

riscv-test-suite/env/test_macros.h Outdated Show resolved Hide resolved

UmerShahidengr and others added 9 commits December 27, 2022 10:14

Bug Fixes ==> Eliminited offset+SIGALIGN as it has already been adjus…

dacbe04

…ted in CHK_OFFSET

LI() Macro definition updated

72297d1

Reverted LI() macro to previous commit

c0c266e

Updated changelog version number

8129c4c

Merge branch 'main' into main

9a2dfae

Signed-off-by: S Pawan Kumar <[email protected]>

Modified definition of LI() which is working, updated LOAD_MEM_VAL macro

7feec7e

Merge branch 'main' of https://github.com/Abdulwadoodd/riscv-arch-test …

6934f24

…into main

Recommit LI() macro

6e93c6d

LI() macro fixed along with a few minor changes related to offset

007540a

allenjbaum requested changes Jan 18, 2023

View reviewed changes

UmerShahidengr added 2 commits January 18, 2023 12:24

Patch ==> Sync with Allen's version

b5c4f9b

Final Patch ==> Cleared all corner cases

60f0218

pawks approved these changes Jan 25, 2023

View reviewed changes

allenjbaum approved these changes Jan 25, 2023

View reviewed changes

allenjbaum merged commit af1d7e1 into riscv-non-isa:main Jan 25, 2023

neelgala mentioned this pull request Jan 26, 2023

Compile warnings for SIGALIGN when FLEN is 64 in arch_test.h #304

Closed

This was referenced Feb 9, 2023

CLIC or vectored mode leads to misaligned load #285

Closed

Test I-ECALL-01.S #146

Closed

Test I-MISALIGN_JMP.S #148

Closed

jjscheel mentioned this pull request Mar 16, 2023

Virtual Memory ACT riscv-admin/dev-partners#11

Open

13 tasks

Abdulwadoodd mentioned this pull request May 22, 2023

Update core-v-verif for CVA6 to support the latest riscv-arch-tests and its infrastructure. openhwgroup/core-v-verif#1919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add changes in arch_test.h file, split macros into 2 files and enable multiple Privilege Modes in Trap Handler #277

Add changes in arch_test.h file, split macros into 2 files and enable multiple Privilege Modes in Trap Handler #277

UmerShahidengr commented Aug 14, 2022

UmerShahidengr commented Sep 15, 2022

InspireSemi left a comment

allenjbaum commented Sep 20, 2022 via email

InspireSemi commented Sep 20, 2022 via email

allenjbaum commented Sep 20, 2022 via email •

edited

Loading

UmerShahidengr commented Nov 10, 2022

neelgala commented Nov 10, 2022

UmerShahidengr commented Nov 10, 2022

pawks commented Nov 10, 2022

UmerShahidengr commented Nov 10, 2022

pawks commented Nov 10, 2022

UmerShahidengr commented Nov 10, 2022

UmerShahidengr commented Nov 22, 2022

UmerShahidengr commented Nov 24, 2022

allenjbaum commented Dec 23, 2022 via email

allenjbaum commented Dec 23, 2022 via email

allenjbaum left a comment

pawks left a comment

Add changes in arch_test.h file, split macros into 2 files and enable multiple Privilege Modes in Trap Handler #277

Add changes in arch_test.h file, split macros into 2 files and enable multiple Privilege Modes in Trap Handler #277

Conversation

UmerShahidengr commented Aug 14, 2022

UmerShahidengr commented Sep 15, 2022

InspireSemi left a comment

Choose a reason for hiding this comment

allenjbaum commented Sep 20, 2022 via email

InspireSemi commented Sep 20, 2022 via email

allenjbaum commented Sep 20, 2022 via email • edited Loading

UmerShahidengr commented Nov 10, 2022

neelgala commented Nov 10, 2022

UmerShahidengr commented Nov 10, 2022

pawks commented Nov 10, 2022

UmerShahidengr commented Nov 10, 2022

pawks commented Nov 10, 2022

UmerShahidengr commented Nov 10, 2022

UmerShahidengr commented Nov 22, 2022

UmerShahidengr commented Nov 24, 2022

allenjbaum commented Dec 23, 2022 via email

allenjbaum commented Dec 23, 2022 via email

allenjbaum left a comment

Choose a reason for hiding this comment

pawks left a comment

Choose a reason for hiding this comment

allenjbaum commented Sep 20, 2022 via email •

edited

Loading