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

Replace zNext with z16 #6779

Merged
merged 1 commit into from
Nov 28, 2022
Merged

Conversation

VermaSh
Copy link
Contributor

@VermaSh VermaSh commented Oct 19, 2022

Signed-off-by: Shubham Verma shubhamv.sv@gmail.com

@VermaSh
Copy link
Contributor Author

VermaSh commented Oct 19, 2022

I have launched a personal build, will post an update once it passes
zLinux build: view/OpenJ9 - Personal/job/Pipeline-Build-Test-Personal/14666/
zOS build: view/OpenJ9 - Personal/job/Pipeline-Build-Test-Personal-s390x_zos/522/

@VermaSh
Copy link
Contributor Author

VermaSh commented Nov 4, 2022

Both builds passed without failures. @r30shah this is ready for review

@r30shah
Copy link
Contributor

r30shah commented Nov 4, 2022

@VermaSh Thanks a lot for making the changes. Is there any OpenJ9 change that depends on changes from this PR as well?

@VermaSh
Copy link
Contributor Author

VermaSh commented Nov 5, 2022

@r30shah Here's the openj9 PR eclipse-openj9/openj9#16140

@@ -141,8 +141,8 @@ OMR::Z::CPU::isAtLeastOldAPI(OMRProcessorArchitecture p)
case OMR_PROCESSOR_S390_Z15:
ans = self()->getSupportsArch(TR::CPU::z15);
break;
case OMR_PROCESSOR_S390_ZNEXT:
Copy link
Contributor

@r30shah r30shah Nov 7, 2022

Choose a reason for hiding this comment

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

We need to add Z16 here, but should not delete the ZNEXT here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have fixed it in my latest commit.

Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
@VermaSh
Copy link
Contributor Author

VermaSh commented Nov 8, 2022

FYI: This and eclipse-openj9/openj9#16140 must be picked together by the release for z16 exploits to remain usable. Having just this would result in the z16 exploits becoming disabled. Having just openj9 changes would break the build.

cc: @r30shah @pshipton @joransiu

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

LGTM

@r30shah
Copy link
Contributor

r30shah commented Nov 9, 2022

Jenkins build zos,zlinux

@r30shah
Copy link
Contributor

r30shah commented Nov 9, 2022

@pshipton @joransiu What do you think about @VermaSh's comment in #6779 (comment) ? Do we want to get both PR in together or it will be OK if we have a short period of time where builds would have z16 changes disabled till OpenJ9 PR is merged?

@pshipton
Copy link
Member

pshipton commented Nov 9, 2022

It's better to do an OpenJ9 coordinated merge/OMR promotion. There is no problem merging the OMR change (although it will block other OMR changes promoting to OpenJ9 until it can promote), but the new OMR level needs to go through the OpenJ9 acceptance build for OMR, which can take 5-6 hours (for a full build of all platforms). OpenJ9 builds will be broken during this waiting time, unless a coordinated merge/OMR promotion is done.

@r30shah
Copy link
Contributor

r30shah commented Nov 10, 2022

@pshipton to be clear, changes in this PR will not break the OpenJ9 builds. Without OpenJ9 PR, for that brief 5-6 hour period, JDK builds would have JIT exploitation of z16 disabled.
So you meant we need to get coordinated merge of these two PRs to make sure we do not have any Interim build with z16 disabled??

@pshipton
Copy link
Member

pshipton commented Nov 10, 2022

Oh ok, no coordinated merge required then. This should be merged when ready and it should promote with the next OMR acceptance, or we can run a custom one.
Acceptance runs Tue-Fri 5am, Mon-Fri 1pm.

@r30shah
Copy link
Contributor

r30shah commented Nov 25, 2022

@VermaSh I forgot, anything remained for this PR? We are done with all the personal sanity tests right? If not then we should coordinate on getting this in.
Given that today is the split, I am not sure how that would work. @pshipton once the split is done, would it be possible to get this low risk change in (Even if we have to double deliver?)

@pshipton
Copy link
Member

Just merge it and I'll get it in.

@VermaSh
Copy link
Contributor Author

VermaSh commented Nov 25, 2022

I forgot, anything remained for this PR? We are done with all the personal sanity tests right? If not then we should coordinate on getting this in.

@r30shah Nothing left for this PR. This is ready to be merged

@r30shah
Copy link
Contributor

r30shah commented Nov 25, 2022

Jenkins build zos,zlinux

@r30shah
Copy link
Contributor

r30shah commented Nov 25, 2022

Thanks @VermaSh , @0xdaryl / @pshipton Can I request anyone of you to merge this change once build passes??

@pshipton
Copy link
Member

I'm not an OMR committer ...

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

LGTM; all required PR builds have passed; merging.

@babsingh babsingh merged commit acdbafc into eclipse:master Nov 28, 2022
@r30shah
Copy link
Contributor

r30shah commented Nov 28, 2022

Thanks a lot @babsingh for merging this one.

@VermaSh VermaSh deleted the replace_zNext_with_z16 branch November 28, 2022 17:34
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.

5 participants