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

Fix little-endian Microblaze does not work when MBEDTLS_HAVE_ASM is defined #38

Closed
wants to merge 1 commit into from

Conversation

alpsayin
Copy link

@alpsayin alpsayin commented Nov 10, 2022

Description

MbedTLS multiplication assembly sources for big numbers not accounting for little endian. Causing zephyr mbedtls unit test to fail when executing mbedtls self-test MPI test #1 (mul_mpi).

Cherry-picked from Mbed-TLS/mbedtls@b88dbdd

Depends on zephyrproject-rtos/zephyr#53899 because this is based on mbedTLS v3.2 which needs TFM v1.7

Status

READY

Requires Backporting

No, because microblaze support isn't there yet. Therefore this can be considered as part of a new feature.

Additional comments

Issue was first reported in upstream mbedtls here:
Mbed-TLS/mbedtls#2020
And the fix was implemented in upstream mbedtls here:
Mbed-TLS/mbedtls#4686

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

Steps to test or reproduce

compiler/zephyr-sdk: microblaze-gcc from topic-microblaze branch of sdk-ng which based on 0.15
Get the toolchain from here: https://github.com/zephyrproject-rtos/sdk-ng/suites/9030507693/artifacts/417018783

zephyr version: v3.2.0
Get the microblaze port here (rebased on v3.2.0-branch): https://github.com/alpsayin/zephyr/tree/microblaze_v3.2.0_public
zephyr version: main
Get the microblaze port here (rebased on main): https://github.com/alpsayin/zephyr/tree/microblaze_public

command: ./scripts/twister -v -p qemu_microblaze -O /scratch/esnap_$USER/twister_out -T tests/crypto/mbedtls

@carlescufi
Copy link
Member

@alpsayin please open a corresponding Zephyr PR that updates the manifest to this PR.

@joerchan
Copy link
Collaborator

@alpsayin Can you please update the commit message to say "(Cherry-picked from )" instead?
Doing 'git cherry-pick -x' automatically adds this line, for future reference or if you want to redo the cherry-pick to get it exactly like that.

Easier to find all downstream patches if you can search for that line.

@alpsayin
Copy link
Author

alpsayin commented Jan 12, 2023

Sure thing, aaand done! I only used "adopted" to imply it wasn't a direct cherry-pick, but I now realise the value.
edit: ooh wow GH seems to have automatically find the reference and autolink it too, I'd put hardlinks for no reason

@carlescufi
Copy link
Member

Sure thing, aaand done! I only used "adopted" to imply it wasn't a direct cherry-pick, but I now realise the value.
edit: ooh wow GH seems to have automatically find the reference and autolink it too, I'd put hardlinks for no reason

No, actually it still is valuable @alpsayin, because then we get the reference with the Git cmd line as well. Please do git cherry-pick -x as @joerchan suggested.

@joerchan
Copy link
Collaborator

No, actually it still is valuable @alpsayin, because then we get the reference with the Git cmd line as well. Please do git cherry-pick -x as @joerchan suggested.

@carlescufi He did, it's good now.

@alpsayin
Copy link
Author

No, actually it still is valuable @alpsayin, because then we get the reference with the Git cmd line as well. Please do git cherry-pick -x as @joerchan suggested.

@carlescufi He did, it's good now.

To be clear, I didn't do git commit -x. I've manually edited the message. I don't know if there's a difference i.e. if git embeds some cherry-pick information into the commit somewhere.

@joerchan
Copy link
Collaborator

To be clear, I didn't do git commit -x. I've manually edited the message. I don't know if there's a difference i.e. if git embeds some cherry-pick information into the commit somewhere.

It's only the message, and it looks identical, so no problem.

…TLS_HAVE_ASM is defined.

Signed-off-by: Kazuyuki Kimura <[email protected]>
Signed-off-by: Dave Rodgman <[email protected]>
(cherry picked from commit b88dbdd)

Signed-off-by: Alp Sayin <[email protected]>
@alpsayin
Copy link
Author

alpsayin commented Jan 27, 2023

Since the bump to v3.2 I've cherry-picked the actual commit as-is, which should make life easier when it's time to bump to v3.3.
I've also re-triggered the CI here at zephyrproject-rtos/zephyr#53692 via a rebase on main.
I've also taken the original commit message without the "fix issue #2020".
The commit message also now includes sign-offs from original authors.
Is that right, or should the sign-off belong to the latest committer?

@alpsayin
Copy link
Author

Depends on zephyrproject-rtos/zephyr#53899 because this is based on mbedTLS v3.2 which needs TFM v1.7

@joerchan
Copy link
Collaborator

@alpsayin Zephyr now uses mbedtls 3.3.0, so this change is now included. I will therefore close this PR.
Let me know if you still have issues.

@joerchan joerchan closed this Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants