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

adds fix to generate _armv8_sha512_probe func #18086

Conversation

yavtuk
Copy link

@yavtuk yavtuk commented Apr 11, 2022

Instruction should be generated by using .inst directive or
using sha512su0 directly. In another case, symtab section
contains an attribute indicating the presence of data inside
the function, which does not allow disassembly this func correctly

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Apr 11, 2022
@yavtuk yavtuk force-pushed the fix/asm-aarch64-instr-generation-from-perl branch from db7d67f to 9cc678e Compare April 11, 2022 21:34
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Apr 11, 2022
@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug cla: trivial One of the commits is marked as 'CLA: trivial' approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Apr 12, 2022
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Apr 12, 2022
@t8m
Copy link
Member

t8m commented Apr 12, 2022

I'm OK with CLA: trivial.

@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 Apr 12, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Apr 13, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Apr 13, 2022
@yavtuk
Copy link
Author

yavtuk commented Apr 18, 2022

thanks 😃

@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Apr 18, 2022

Why not change all of the .longs to .inst in this PR? If it's needed for one, it's surely needed for all the others...

Obviously a change to the commit message required too, something along the lines of

Prefer .inst rather than .long for probe instructions in arm64cpuid.pl

Fixes an issue disassembling the functions because the symtab contains an
attribute indicating the presence of data within them.

@yavtuk
Copy link
Author

yavtuk commented Apr 18, 2022

No problem, I’ll check and fix other places

@yavtuk
Copy link
Author

yavtuk commented Apr 18, 2022

@tom-cosgrove-arm one more question about data in text for armv8,
what do you think about to move data from text to rodata section?
example,
using assembler for armv8
https://github.com/openssl/openssl/blob/master/crypto/sha/asm/keccak1600-armv8.pl#L85
here we explicitly specify that the data has to be in .text section

using C
https://github.com/openssl/openssl/blob/master/crypto/sha/keccak1600.c#L87
here compiler put it to .rodata section

@tom-cosgrove-arm
Copy link
Contributor

@yavtuk Moving data from one section to another is quite a different change than using .inst instead of .long, even if logically it's just the converse. We would need to be certain that it would work with all reasonable assemblers and systems, and that the Perl xlate scripts handle it correctly. While it's probably okay, it's also probably an unnecessary risk if it's not causing any actual problems in practice. And at the very least it would be a different PR. But others might have a different opinion on this - @zorrorffm what are your thoughts?

@yavtuk yavtuk force-pushed the fix/asm-aarch64-instr-generation-from-perl branch from 9cc678e to 50847ab Compare April 18, 2022 17:42
Fixes an issue disassembling the functions because the symtab contains
an attribute indicating the presence of data within them.

CLA: trivial
@yavtuk yavtuk force-pushed the fix/asm-aarch64-instr-generation-from-perl branch from acccb7b to 28b80f2 Compare April 18, 2022 17:47
@yavtuk
Copy link
Author

yavtuk commented Apr 18, 2022

fixed in 3 more places in the crypto/arm64cpuid.pl file

@zorrorffm
Copy link
Contributor

I would prefer to keep the constant data in text section instead of rodata section in the sense of performance. The data in rodata section maybe is far from the place where it is loaded, so ldr instruction should be used to load rodata data. If the constant data is in text section, adr/adrp instruction can be used to load that data. adr and adrp are much more efficient than ldr instruction.

@yavtuk
Copy link
Author

yavtuk commented Apr 19, 2022

@zorrorffm thanks

@t8m
Copy link
Member

t8m commented Apr 19, 2022

Still OK by me also with CLA: trivial. @paulidale please re-approve.

@t8m t8m requested a review from paulidale April 19, 2022 15:49
@t8m t8m added approval: review pending This pull request needs review by a committer and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Apr 19, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 19, 2022
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.

Good and agreed trivial.

@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 Apr 28, 2022
@yavtuk
Copy link
Author

yavtuk commented Apr 28, 2022

thanks

@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.

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 29, 2022
@t8m
Copy link
Member

t8m commented Apr 29, 2022

Merged to master branch. Thank you for your contribution.

@t8m t8m closed this Apr 29, 2022
openssl-machine pushed a commit that referenced this pull request Apr 29, 2022
Fixes an issue disassembling the functions because the symtab contains
an attribute indicating the presence of data within them.

CLA: trivial

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18086)
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 14, 2022
Fixes an issue disassembling the functions because the symtab contains
an attribute indicating the presence of data within them.

CLA: trivial

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#18086)

(cherry picked from commit 4d63eaf)
openssl-machine pushed a commit that referenced this pull request Nov 21, 2022
Fixes an issue disassembling the functions because the symtab contains
an attribute indicating the presence of data within them.

CLA: trivial

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18086)

(cherry picked from commit 4d63eaf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch cla: trivial One of the commits is marked as 'CLA: trivial' severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants