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.

Sign up for GitHub

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

Jump to bottom

Refactor CPUID code #11311

Closed
wants to merge 7 commits into from
Closed

Refactor CPUID code #11311

wants to merge 7 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Mar 11, 2020

We were using CPUID coded in several modules, but it was unclear how
it actually got there, and could fail randomly.

To remedy that, this change separates the CPUID C code from the rest
of cryptlib.c, and ensures the right modules get both that and the
assembler sources explicitly.

Fixes #11281

@levitte levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Mar 11, 2020
@levitte levitte added this to the 3.0.0 milestone Mar 11, 2020
@levitte levitte added this to In progress in 3.0 New Core + FIPS via automation Mar 11, 2020
@levitte levitte mentioned this pull request Mar 11, 2020
Closed
paulidale
paulidale reviewed Mar 12, 2020
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

CI failures are relevant.

@paulidale
Copy link
Contributor

I do wonder if propagating the CPUID is the way to go, would having a series of utility functions which query specific features be more readable? It won't change what needs to be done here but it would avoid magic constants throughout the code.

@levitte
Copy link
Member Author

levitte commented Mar 12, 2020

Do we want to make the CPUID functions public? Considering that legacy.so links with libcrypto.so, that's what it'll take. Or did you mean utility functions like replacing (OPENSSL_ia32cap_P[2] & (1 << 29)) == 0 with OPENSSL_ia32cap_has_SHAEXT()? I would straight up implement those as macros.

Anyway, that sounds like an effort for another PR

@paulidale
Copy link
Contributor

The latter. Agreed, it is an enhancement.

xkqian
xkqian approved these changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@xkqian xkqian left a comment

Choose a reason for hiding this comment

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

This fix works on my local machine.

@mattcaswell
Copy link
Member

Travis red cross is relevant

@levitte
Copy link
Member Author

levitte commented Mar 12, 2020

I figured out one of the failures... it's possible that the other one is fixed as well

@openssl-machine
Copy link
Collaborator

This issue is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@t8m
Copy link
Member

t8m commented Apr 14, 2020

I suppose the travis failure is still relevant, also needs rebase.

@levitte
Copy link
Member Author

levitte commented Apr 14, 2020

Rebased

@levitte levitte mentioned this pull request Apr 15, 2020
Closed
2 tasks
@slontis
Copy link
Member

slontis commented Apr 15, 2020

@levitte - travis is running now - it has some issues in liblegacy.

@t8m
Copy link
Member

t8m commented Apr 15, 2020

Hmm, looking at the ppccap.c it seems to me it would make sense to rather refactor the algorithm-specific code out than to add everything to the legacy provider.

@t8m
Copy link
Member

t8m commented Apr 15, 2020

Another option - more ugly but less invasive - #ifdef all the code not needed in the legacy provider so when the .c is built for the legacy provider this code is not compiled.

@levitte levitte mentioned this pull request Apr 24, 2020
Closed
@openssl-machine
Copy link
Collaborator

This issue is in a state where it requires action by @openssl/committers but the last update was 30 days ago

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved May 16, 2020
bernd-edlinger
bernd-edlinger previously approved these changes May 16, 2020
View reviewed changes
@t8m
Copy link
Member

t8m commented May 19, 2020

This is rather WIP - there are conflicts and issues in Travis jobs in legacy provider.

@levitte levitte changed the title Refactor CPUID code [WIP] Refactor CPUID code May 29, 2020
@levitte
Copy link
Member Author

levitte commented May 29, 2020

This is rather WIP - there are conflicts and issues in Travis jobs in legacy provider.

Yeah... this has turned out to contain a building problem that I've been avoiding for some time. I'll have to bite that bullet...

@kroeckx kroeckx modified the milestones: 3.0.0, 3.0.0 beta1 Sep 16, 2020
@levitte levitte removed this from the 3.0.0 beta1 milestone Sep 16, 2020
@levitte
Include BN assembler alongside CPUID code
ce606dd 
It turns out that some CPUID code requires the presence of some BN
assembler code, so we make sure it's included in the same manner as
the CPUID code itself.
@levitte
fixup! Include BN assembler alongside CPUID code
6a1b9b5
@levitte
Copy link
Member Author

levitte commented Dec 17, 2020

Rebased on a fresher master. Let's see what can be done with this

@levitte levitte dismissed bernd-edlinger’s stale review December 17, 2020 20:05

The approval is stale and was made too early

3.0 New Core + FIPS automation moved this from Reviewer approved to Needs review Dec 17, 2020
@levitte
Copy link
Member Author

levitte commented Jan 21, 2021

I've done several tests lately, that were failing previously, but that now build and run correctly, given a fresher master.
With that in mind, I'm wondering if this fix is still necessary. If someone still gets clashes when linking, and can give me a way to reproduce it on Linux (I run Debian Sid), then this may be worth another look. If not, I'm considering closing this, to possibly pick up on another rainy day.

@t8m
Copy link
Member

t8m commented Jan 22, 2021

I remember the problems were mostly on non x86 architectures. PPC was the most problematic IMO.

@xkqian
Copy link
Contributor

xkqian commented Jan 22, 2021

I remember the problems were mostly on non x86 architectures. PPC was the most problematic IMO.

Seems it happened on x86 32bit platform. I remember that Richard had one fix before, I verified it locally and the issue had gone. But that fix seems cause Travis red cross. I will verify it locally to see whether the issue has gone now.

@levitte
Copy link
Member Author

levitte commented Jan 23, 2021

Seems it happened on x86 32bit platform.

Yes, and I could reproduce that... then.

I will verify it locally to see whether the issue has gone now.

Please do.

@xkqian
Copy link
Contributor

xkqian commented Jan 25, 2021

Seems it happened on x86 32bit platform.

Yes, and I could reproduce that... then.

I will verify it locally to see whether the issue has gone now.

Please do.

Done, the issue has gone with the openssl master branch code, as least the commit: daa86f9

paulidale
paulidale previously approved these changes Jan 25, 2021
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

One question but approved nonetheless.

.travis.yml
@@ -59,6 +59,9 @@ jobs:
arch: s390x
compiler: gcc
env: CONFIG_OPTS="--strict-warnings"
- os: linux-ppc64le
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering why?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's quite possible that this platform demonstrated the issue at the time.
Since we don't do Travis any more, that's a bit hard to test for the moment... I'm not sure if it's even possible to specify specific hardware with GitHub Actions

3.0 New Core + FIPS automation moved this from Needs review to Reviewer approved Jan 25, 2021
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 25, 2021
@paulidale paulidale changed the title [WIP] Refactor CPUID code Refactor CPUID code Jan 25, 2021
@paulidale
Copy link
Contributor

I also removed the [WIP] from the subject here.

@levitte
Copy link
Member Author

levitte commented Jan 25, 2021

I also removed the [WIP] from the subject here.

... because?

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@levitte levitte removed the approval: done This pull request has the required number of approvals label Jan 26, 2021
@levitte levitte dismissed paulidale’s stale review January 26, 2021 10:06

Approval dismissed, because there's currently nothing that demonstrates that this fixes the problem at this point in time

3.0 New Core + FIPS automation moved this from Reviewer approved to Needs review Jan 26, 2021
@levitte
Copy link
Member Author

levitte commented Jan 26, 2021

Done, the issue has gone with the openssl master branch code, as least the commit: daa86f9

After a conversation with the rest of the team, it's been decided to close this for now. If the issue turns out to still be present, we can pick it up then.

@levitte levitte closed this Jan 26, 2021
3.0 New Core + FIPS automation moved this from Needs review to Done Jan 26, 2021
3.0.0 estimator automation moved this from review pending to Done Jan 26, 2021
@levitte levitte mentioned this pull request Mar 31, 2021
Closed
@levitte
Copy link
Member Author

levitte commented Mar 31, 2021

Re-opening this, it's time to get it done, now that there's a workflow to test against (#14753)

@mattcaswell mattcaswell mentioned this pull request Apr 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xkqian xkqian xkqian approved these changes

@bernd-edlinger bernd-edlinger bernd-edlinger left review comments

@paulidale paulidale paulidale left review comments

Assignees
No one assigned
Labels
branch: master Merge to master branch triaged: OTC evaluated This issue/pr was triaged by OTC
Projects
No open projects
3.0 New Core + FIPS
  
Done
3.0.0 estimator
Done
Milestone
3.0.0 beta1
Development

Successfully merging this pull request may close these issues.

Build OpenSSL on ubuntu, compile failed in x86_32(providers/legacy.so)
10 participants
@levitte @paulidale @mattcaswell @openssl-machine @t8m @slontis @xkqian @bernd-edlinger @romen @kroeckx