search

riscv-admin / dev-partners

This repo is for tracking of RISC-V Development Partners Activities
3 stars 0 forks source link

Pointer Masking SAIL #14

Open jjscheel opened 1 year ago

jjscheel commented 1 year ago

Technical Group

Pointer Masking TG

ratification-pkg

Pointer Masking

Technical Liaison

Martin Maas, Adam Zabrocki

Task Category

SAIL model

Task Sub Category

Ratification Target

3Q2023

Statement of Work (SOW)

SOW: link

SOW Signoffs: (delete those not needed)

Waiver

Pull Request Details

Sail PR (closed): riscv/sail-riscv #452 Sail PR: riscv/sail-riscv #454

jjscheel commented 1 year ago

This work is not yet staffed.

jjscheel commented 1 year ago

@mustafa7755, @Muhammad-Saadi-Abbasi, it sounds like you guys will be working on this. I've assigned it to both of you not knowing if you're sharing or working on separate pieces. Feel free to unassign yourselves as-appropriate based on assignments.

jjscheel commented 1 year ago

If possible, I'd like an update in the April 25th meeting on when you think you'll have the following dates:

Comments in the issue with these dates would be great.

jjscheel commented 1 year ago

BTW, I should note for this issue that it's marked "At Risk" due to the fact that we had no one assigned.

Given that we have assignees, I'd like their comfort with the SOW and have a basic plan before we move back to "As Planned" status. My hope is that we'll be able to do this in the April 25th meeting at the latest.

UmerShahidengr commented 1 year ago

Update Apr 25th, 2023 => Status changed to "As planned". @mustafa7755 and @Muhammad-Saadi-Abbasi have been assigned to this task, on-boarding is complete. Next week task => Start of the developement phase

mustafa7755 commented 1 year ago

If possible, I'd like an update in the April 25th meeting on when you think you'll have the following dates:

  • [ ] Development of code complete (ready to test)
  • [ ] Testing complete (ready to submit first PR)
  • [ ] Last PR accepted (done)

Comments in the issue with these dates would be great.

Before giving any dates, there are some concerns:

Once these concerns are resolved, we can then give final dates.

Adam-pi3 commented 1 year ago

@mustafa7755:

SOW is outdated

SOW is update - I modified it on 4/20/2023 but forgot to change the "Last modified Date". I've just updated it as well.

When is the architectural review of pointer masking spec be finalized?

The spec is very close to be final. We still modified minor details (like which section should include usage of PM). However, it should not affect the functionality of the current proposal.

Opcodes and addresses of CSR's are missing from spec

@martinmaas can help with that. We do have allocated temporary CSR addresses but for the older spec with more CSRs than we need today: https://github.com/riscv/riscv-j-extension/blob/master/pointer-masking-proposal.adoc

How we are going to compare our results with spike then?

I'm starting the discussion to update QEMU implementation.

UmerShahidengr commented 1 year ago

Update May 7th, 2023 => Not much update on this task. Possibly it will get deferred to Q3, 2023 because of upcoming new work (debug native triggers)

jjscheel commented 1 year ago

@martinmaas, @Adam-pi3, you folks should be aware that given the challenges we've had with Code Size Reduction, I've got 1 resource to spend on either Pointer Masking and/or Debug. So, barring some explicit priority guidance, they will be started first-come, first serve. I'll be in touch with what and how we decide.

mustafa7755 commented 1 year ago

I'm starting the discussion to update QEMU implementation.

@Adam-pi3, any update regarding this?

UmerShahidengr commented 1 year ago

Are we talking about QEMU implementation or Spike implementation? Spike is compatible with riscof, so adding support for current specs in Spike will be ideal for us.

jjscheel commented 1 year ago

The discussion this week in the Pointer Masking TG was that @martinmaas would request the Architecture Review Committee to prioritize closure of the items preventing SAIL and ACT work in hopes of keeping this work moving forward.

@martinmaas, please provide an update when available.

UmerShahidengr commented 1 year ago

Update May 23rd, 2023 => Not much update on this task yet.

jjscheel commented 1 year ago

Understand. Thanks, @UmerShahidengr.

Have not seen update from AR about this. @martinmaas, any updates?

jjscheel commented 1 year ago

Moving to Blocked due to dependency on AR closure of content.

UmerShahidengr commented 1 year ago

Update ⇾ June 12th, 2023: Project is still blocked.

jjscheel commented 1 year ago

Agreed. Removing from Agenda. Re-visit next meeting.

jjscheel commented 1 year ago

Moving Projected completion out 6 months based on where we are to 12/30.

UmerShahidengr commented 1 year ago

Update ⇾ July 11th, 2023 Project is blocked

jjscheel commented 1 year ago

Comment from most recent ARC minutes (July 10) - link

Zjpm (Pointer Masking): Recent feedback from AR was incorporated into an updated spec. Review of this has started, with initial discussion about whether the presence of pointer masking affects the required minimum implemented width of CSRs that hold addresses. The following conclusions were reached:

  • Hardware writes of addresses capture a transformed address in the CSR.
  • Software writes capture the write data value in the CSR unmodified.
  • The implemented WARL width of the register (i.e. how many lsb CSR bits are physically implemented with storage) remains unchanged from the current architecture's WARL specs for each such CSR. For example, if currently tval CSRs support 52 bits valid addresses and pointer masking is implemented and enabled with 16 msb's of masking, the necessary minimum implemented width of tval CSRs remains 52 bits irrespective of whether pointer masking is enabled or disabled.
jjscheel commented 1 year ago

Per ARC minutes from 8/23/2023: https://lists.riscv.org/g/tech-chairs/message/1593:

The ARC continued discussion of the Pointer Masking spec, with the primary issue being the extension and enable bit names. The previous suggestion that the enable bit names be tied to the extension names was deemed inappropriate upon reflection because Pointer Masking an attribute of the ABI for given mode of operation, and the extension names are for the bits at higher modes of operation that define the enable bits. It was also suggested that the number of address bits be included in the name (not the number of ignored bits to make it independent of RV64 vs. RV128). As a result, our recommendation is now that the Pointer Masking enable bits be named PM48E in their respective CSRs. This enables would be read-only 0 in RV32 modes of operation.

UmerShahidengr commented 11 months ago

Update ⇾ September 26th, 2023 No update on this.

UmerShahidengr commented 10 months ago

Update ⇾ October 24th, 2023 No update on this.

jjscheel commented 9 months ago

From ARC Minutes for 11/14/2023: https://lists.riscv.org/g/tech-unprivileged/message/585

An updated pointer-masking spec was received, and ARC expects to complete final review and approve next week.

Unblocking. @UmerShahidengr, as they say in racing, "Start your engines!"

UmerShahidengr commented 9 months ago

Update ⇾ November 28th, 2023 Great to hear that. @HAMZA-AFZAL404 can you look at these specs, lets start its implementation.

UmerShahidengr commented 9 months ago

@jjscheel can you direct me to the latest specs of Pointer Masking extension. I dont seem to find it.

jjscheel commented 9 months ago

The spec source repo is here: https://github.com/riscv/riscv-j-extension

It appears they are integrating the latest spec in the repo here: https://github.com/riscv/riscv-j-extension/blob/master/zjpm-spec.pdf

UmerShahidengr commented 9 months ago

Update ⇾ December 12th, 2023 @HAMZA-AFZAL404 have started working on it, specs have been understood, Sail implementation is in process.

HAMZA-AFZAL404 commented 7 months ago

Update -> February 14th, 2024

Up to this point, I have successfully implemented pointer masking support for the Base, Floating Point, and Compressed Instructions. However, work on other instructions is still pending, and I will be addressing those aspects before proceeding further.

To ensure the correctness of the implementation, I am seeking a review from someone from Sail. Currently, as there is no dev branch in the official RISC-V Sail repository, I have initiated a pull request to my own forked repository. You can review the changes and progress in the pull request here.

If you want me to submit the pull request to a specific branch in the official repository for a comprehensive review, kindly provide guidance on the recommended approach.

Thank you for your attention and support.

Best regards, Hamza

jjscheel commented 6 months ago

Thanks, @HAMZA-AFZAL404!

@billmcspadden-riscv, can you work with Hamza to provide some guidance on how to solicit review of his work and where to build his PR? Thanks!

jjscheel commented 6 months ago

@martinmaas, @Adam-pi3, Bill has been unavailable to help. Any chance on of you could work with Hamza to ensure that his implementation is correct for this first piece of work? Thanks!

UmerShahidengr commented 6 months ago

Update ⇾ March 5th, 2024 Stalled.

jjscheel commented 6 months ago

Please note that ARC has approved Pointer Masking and the Committee Chairs signoff has completed. It has thus Frozen and will be entering public review.

@martinmaas and @Adam-pi3, please try to help @HAMZA-AFZAL404 so that we can ensure this doesn't hold up Ratification.

martinmaas commented 6 months ago

I am, unfortunately, entirely unfamiliar with Sail. I can take a look and see whether I can spot anything based on my limited understanding, but I don't think this can replace a proper review by someone familiar with Sail.

UmerShahidengr commented 5 months ago

Update ⇾ April 2nd, 2024 @HAMZA-AFZAL404 has completed the Sail model, it will be released for review soon.

jjscheel commented 5 months ago

@martinmaas, there's an excellent Sail overview video listed on the wiki.riscv.org page in the "Learn" column ("Explore Sail"). That would likely be the best overview.

jjscheel commented 5 months ago

@UmerShahidengr, any PR for the Sail code yet? If so, please provide a link.

UmerShahidengr commented 5 months ago

@jjscheel we have not added the PR yet as @billmcspadden-riscv asked to update some self checking tests at riscv-tests for the Sail CI, we have been working on those self checking tests, will deliver the model once tests for CI will be ready. @billmcspadden-riscv we have not found any Priv Arch tests in assembly in riscv-tests, all tests are fairly basic ones and are written in C. If there exists any assembly test related to CSR or Priv Arch, please point us out to those tests as sample.

billmcspadden-riscv commented 5 months ago

Go to: https://github.com/riscv-software-src/riscv-tests Then look under the isa/ directory. All tests under this directory are assembly language tests, although they all make use of various macros for test expansion (which can make reading the test a little more complicated). Tests in isa/rv32si/ and isa/rv64si/ all are run in supervisor mode.

I hope this helps.

Bill Mc.

On Tue, Apr 16, 2024 at 9:09 AM Umer Shahid @.***> wrote:

@jjscheel https://github.com/jjscheel we have not added the PR yet as @billmcspadden-riscv https://github.com/billmcspadden-riscv asked to update some self checking tests at riscv-tests for the Sail CI, we have been working on those self checking tests, will deliver the model once tests for CI will be ready. @billmcspadden-riscv https://github.com/billmcspadden-riscv we have not found any Priv Arch tests in assembly in riscv-tests, all tests are fairly basic ones and are written in C. If there exists any assembly test related to CSR or Priv Arch, please point us out to those tests as sample.

— Reply to this email directly, view it on GitHub https://github.com/riscv-admin/dev-partners/issues/14#issuecomment-2059188446, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXROLOCX7QZKVLRO6ERQW5DY5UWIJAVCNFSM6AAAAAAV5YT5U2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJZGE4DQNBUGY . You are receiving this because you were mentioned.Message ID: @.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

UmerShahidengr commented 4 months ago

@jjscheel , @billmcspadden-riscv Pointer Masking PR has been added, here is the link to it.

martinmaas commented 4 months ago

Thanks a lot for the PR, and for all the work on this! I added a review at

https://github.com/riscv/sail-riscv/pull/452#pullrequestreview-2010554427

UmerShahidengr commented 4 months ago

Thanks alot @martinmaas for the review. @HAMZA-AFZAL404 will reply to your remarks soon. Just one more thing, this PR, which you have reviewed, was closed due to the conflicts issues in Sail repo. @HAMZA-AFZAL404 has removed those conflict, and added a new PR which is here.

jjscheel commented 4 months ago

@martinmaas and @HAMZA-AFZAL404, thanks for all of your work here!

@martinmaas, to resolve the Freeze waiver for Sail, we need a functionally complete PR for Sail (withing the bounds of what's implementable). If you'd be so kind as to acknowledge that in this issue, when we've achieved that goal, it will help me in the broader RISC-V process.

martinmaas commented 3 months ago

Thanks @HAMZA-AFZAL404 for all the work on this. I believe there is one more change, which is a modification to the spec that happened during public review:

MPRV and SPVP affect pointer masking as well, causing the pointer masking settings of the effective privilege mode to be applied. When MXR is in effect at the effective privilege mode where explicit memory access is performed, pointer masking does not apply.

https://github.com/riscv/riscv-j-extension/pull/74/files

(Based on the comment thread for the PR, I believe that the MPRV and SPVP portion is already handled, but the MXR exception will likely still need to be implemented.)

UmerShahidengr commented 3 months ago

Update June 11th, 2024: The updated PR is available in Sail repo, @martinmaas please review this one and let us know what further changes are required

jjscheel commented 3 months ago

@billmcspadden-riscv, I'd appreciate your review too with an eye toward completeness for "Freeze", not full PR acceptance.

UmerShahidengr commented 2 months ago

Update June 25th, 2024: The PR is yet to be reviewed.

billmcspadden-riscv commented 2 months ago

We talked about this at the tech-golden-model meeting on Monday. We scanned through the PR (PR #454). It's fairly large: 18 files and roughly 325 lines of new code.

We need to identify an SME to review this. There are 1 or 2 others who expressed an interest in reviewing the PR (from a Sail perspective).

Bill Mc.

On Tue, Jun 25, 2024 at 7:54 AM Umer Shahid @.***> wrote:

Update June 25th, 2024: The PR is yet to be reviewed.

— Reply to this email directly, view it on GitHub https://github.com/riscv-admin/dev-partners/issues/14#issuecomment-2188860846, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXROLOHIK5SWE2AEAQDJCBTZJFSBDAVCNFSM6AAAAAAV5YT5U2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBYHA3DAOBUGY . You are receiving this because you were mentioned.Message ID: @.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

UmerShahidengr commented 1 month ago

Update July 23rd, 2024: Thanks to @Timmmm for reviewing the PR. @HAMZA-AFZAL404 has been working on rebasing the PR (as more than 20 PRs have been merged in Sail since this PR, and the latest changes are conflicting with this one). Hamza will update the PR as per Tim's comments.

jjscheel commented 1 month ago

@martinmaas and @billmcspadden-riscv, the outstanding question at this time is whether the PR is functionally complete enough to consider the Freeze waiver for Sail resolved. Please share your thoughts.