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

8309258: RISC-V: Add riscv_hwprobe syscall #1942

Closed
wants to merge 4 commits into from

Conversation

robehn
Copy link
Contributor

@robehn robehn commented Nov 2, 2023

Hi, please consider.

RISC-V has many extensions and it's not feasible to set them correctly on the command line for users.
There is no instruction to get the cpu, such as cpuid on x86.

Still running tests, it will take a while.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • JDK-8309258 needs maintainer approval
  • JDK-8315206 needs maintainer approval
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • JDK-8316859 needs maintainer approval

Issues

  • JDK-8309258: RISC-V: Add riscv_hwprobe syscall (Enhancement - P4 - Approved)
  • JDK-8315206: RISC-V: hwprobe query is_set return wrong value (Bug - P2 - Approved)
  • JDK-8316859: RISC-V: Disable detection of V through HWCAP (Task - P4 - Approved)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/1942/head:pull/1942
$ git checkout pull/1942

Update a local copy of the PR:
$ git checkout pull/1942
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/1942/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1942

View PR using the GUI difftool:
$ git pr show -t 1942

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/1942.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 2, 2023

👋 Welcome back rehn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title Backport 31b6fd775f1c4f2841d9a52ad5f275ad446ee661 8309258: RISC-V: Add riscv_hwprobe syscall Nov 2, 2023
@openjdk
Copy link

openjdk bot commented Nov 2, 2023

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport approval rfr Pull request is ready for review labels Nov 2, 2023
@mlbridge
Copy link

mlbridge bot commented Nov 2, 2023

Webrevs

@robehn
Copy link
Contributor Author

robehn commented Nov 2, 2023

I did I 'accidentally' removed OS_HEADER_INLINE, fixing it back.

Copy link
Member

@RealFYang RealFYang left a comment

Choose a reason for hiding this comment

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

Looks good to me assuming that following two related fixes will also be backported:

8315206: RISC-V: hwprobe query is_set return wrong value
8316859: RISC-V: Disable detection of V through HWCAP

@openjdk
Copy link

openjdk bot commented Nov 6, 2023

⚠️ @robehn This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

@robehn
Copy link
Contributor Author

robehn commented Nov 6, 2023

Looks good to me assuming that following two related fixes will also be backported:

8315206: RISC-V: hwprobe query is_set return wrong value
8316859: RISC-V: Disable detection of V through HWCAP

Hi, thanks, yes, and "8315195: RISC-V: Update hwprobe query for new extensions"

@GoeLin
Copy link
Member

GoeLin commented Nov 6, 2023

Hi @robehn, could you please enable GHA actions on your repo? Thanks!

@robehn
Copy link
Contributor Author

robehn commented Nov 7, 2023

@GoeLin @RealFYang should I do all four in this PR, it may be easier ?

@RealFYang
Copy link
Member

@GoeLin @RealFYang should I do all four in this PR, it may be easier ?

Hi, I personally perfer the first three in one go as they are somehow directly related.
@GoeLin : I guess that's OK from a maintainer's perspective?

@GoeLin
Copy link
Member

GoeLin commented Nov 7, 2023

Hi @robehn, yes, you can push them on top in this branch. If so, please use the /issue command. But you can also do separate changes. I'd leave it to you and @RealFYang to decide, you are the riskv experts.

@GoeLin
Copy link
Member

GoeLin commented Nov 7, 2023

/approve yes

@openjdk
Copy link

openjdk bot commented Nov 7, 2023

@GoeLin
8309258: The approval request has been approved.

@openjdk
Copy link

openjdk bot commented Nov 7, 2023

@robehn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8309258: RISC-V: Add riscv_hwprobe syscall
8315206: RISC-V: hwprobe query is_set return wrong value
8316859: RISC-V: Disable detection of V through HWCAP

Reviewed-by: fyang

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 47 new commits pushed to the master branch:

  • 8b9a72e: 8317374: Add Let's Encrypt ISRG Root X2
  • a4fe7ac: 8316461: Fix: make test outputs TEST SUCCESS after unsuccessful exit
  • 1240b34: 8276819: javax/print/PrintServiceLookup/FlushCustomClassLoader.java fails to free
  • 5cb86e3: 8273831: PrintServiceLookup spawns 2 threads in the current classloader, getting orphaned
  • 2486ba2: 8315415: OutputAnalyzer.shouldMatchByLine() fails in some cases
  • fc7650f: 8161536: sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java fails with ProviderException
  • d760d9d: 8314242: Update applications/scimark/Scimark.java to accept VM flags
  • aaa26d5: 8307123: Fix deprecation warnings in DPrinter
  • 64c2048: 8299241: jdk/jfr/api/consumer/streaming/TestJVMCrash.java generates unnecessary core file
  • f60d396: 8317920: JDWP-agent sends broken exception event with onthrow option
  • ... and 37 more: https://git.openjdk.org/jdk17u-dev/compare/73c3316c2fb658c4d495433c79d616a1fbe61209...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed approval labels Nov 7, 2023
@robehn
Copy link
Contributor Author

robehn commented Nov 8, 2023

/issue add 8315206
/issue add 8316859

@openjdk
Copy link

openjdk bot commented Nov 8, 2023

@robehn
Adding additional issue to issue list: 8315206: RISC-V: hwprobe query is_set return wrong value.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Nov 8, 2023
@openjdk
Copy link

openjdk bot commented Nov 8, 2023

@robehn
Adding additional issue to issue list: 8316859: RISC-V: Disable detection of V through HWCAP.

@robehn
Copy link
Contributor Author

robehn commented Nov 8, 2023

/approval request 8315206 Bug fix for hwprobe. Applied clean on the hwprobe add backport.
/approval request 8315206 Related bug fix, V should not be enabled by hwcap. Applied clean on the hwprobe add backport.

@openjdk
Copy link

openjdk bot commented Nov 8, 2023

@robehn
8309258: Approval has already been requested and approved.
8315206: The approval request has been created successfully.
8316859: The approval request has been created successfully.

@openjdk
Copy link

openjdk bot commented Nov 8, 2023

@robehn
8309258: Approval has already been requested and approved.
8315206: The approval request has been updated successfully.
8316859: The approval request has been updated successfully.

@openjdk openjdk bot added the approval label Nov 8, 2023
@robehn
Copy link
Contributor Author

robehn commented Nov 8, 2023

We do the three first here, and I'll do the fourth in a separate PR, as suggest by @RealFYang.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed approval labels Nov 9, 2023
@robehn
Copy link
Contributor Author

robehn commented Nov 10, 2023

I found no issue in testing, I'm integrating and starting a PR for the last back port for this.

@robehn
Copy link
Contributor Author

robehn commented Nov 10, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Nov 10, 2023

Going to push as commit b506559.
Since your change was applied there have been 48 commits pushed to the master branch:

  • 312f8c0: 8319525: RISC-V: Rename *_riscv64.ad files to *_riscv.ad under riscv/gc
  • 8b9a72e: 8317374: Add Let's Encrypt ISRG Root X2
  • a4fe7ac: 8316461: Fix: make test outputs TEST SUCCESS after unsuccessful exit
  • 1240b34: 8276819: javax/print/PrintServiceLookup/FlushCustomClassLoader.java fails to free
  • 5cb86e3: 8273831: PrintServiceLookup spawns 2 threads in the current classloader, getting orphaned
  • 2486ba2: 8315415: OutputAnalyzer.shouldMatchByLine() fails in some cases
  • fc7650f: 8161536: sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java fails with ProviderException
  • d760d9d: 8314242: Update applications/scimark/Scimark.java to accept VM flags
  • aaa26d5: 8307123: Fix deprecation warnings in DPrinter
  • 64c2048: 8299241: jdk/jfr/api/consumer/streaming/TestJVMCrash.java generates unnecessary core file
  • ... and 38 more: https://git.openjdk.org/jdk17u-dev/compare/73c3316c2fb658c4d495433c79d616a1fbe61209...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 10, 2023
@openjdk openjdk bot closed this Nov 10, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 10, 2023
@openjdk
Copy link

openjdk bot commented Nov 10, 2023

@robehn Pushed as commit b506559.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
4 participants