Skip to content

Conversation

@mpg
Copy link
Contributor

@mpg mpg commented Jul 17, 2019

Add MBEDTLS_SHA512_SMALLER, parallel to MBEDTLS_SHA256_SMALLER.

Sizes, as measured with arm-none-eabi-gcc -Wall -Wextra -Iinclude -Os -mcpu=cortex-m0plus -mthumb -c library/sha512.c && arm-none-eabi-size sha512.o:

  • without MBEDTLS_SHA512_SMALLER: 5691
  • with MBEDTLS_SHA512_SMALLER: 3411
  • saving: 2280 bytes

Speed impact on my laptop's i7, building with GCC -O2 and running programs/test/benchmark sha512:

  • larger version: 294000 KB/s
  • smaller version: 252000 KB/s
  • slowdown: -14%

Todo:

  • measure speed impact on target and expand config.h documentation

mpg added 4 commits July 17, 2019 13:05
Saves 356 bytes on sha512.o compiling for Cortex-M0+ with ARM-GCC

Size measured with:
arm-none-eabi-gcc -Wall -Wextra -Iinclude -Os -mcpu=cortex-m0plus -mthumb -c library/sha512.c
arm-none-eabi-size sha512.o

GCC version:
arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2018-q2-update) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907]
Saves 108 bytes (measured as in previous commit).
Saves 1924 bytes (same measurement as before).
@mpg mpg added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. needs: work The pull request needs rework before it can be merged. labels Jul 17, 2019
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

I'm happy with this. Will MBEDTLS_SHA512_SMALLER be tested in the full config tests in all.sh?

@mpg
Copy link
Contributor Author

mpg commented Jul 22, 2019

@Patater Thanks for your review! Yes, since the option is not excluded from config.pl full it will be tested as part of full-config tests in all.sh. (Arguably it may make sense to exclude this option as it's about an alternative implementation, not an additional feature - and then it would need another way of testing in all.sh - but I mirrored what was done with the corresponding SHA-256 option, which is included in full configs.)

Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm 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, but I left suggestions for improvements.

PUT_UINT64_BE(n, b, i);
}
#else
#define sha512_put_uint64_be PUT_UINT64_BE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we make it a function all the time and count on compilers to inline when compiling for performance?


P( A[0], A[1], A[2], A[3], A[4], A[5], A[6], A[7], W[i], K[i] );

temp1 = A[7]; A[7] = A[6]; A[6] = A[5]; A[5] = A[4]; A[4] = A[3];
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can save a few more bytes with

        temp1 = A[7];
        for( k = 1; k < 8; k++ )
            A[k] = A[k - 1];
        A[0] = temp1;

On the other hand, I tried removing the rotation of A and running

        P( A[(80-i+0)%8], A[(80-i+1)%8], A[(80-i+2)%8], A[(80-i+3)%8], A[(80-i+4)%8], A[(80-i+5)%8], A[(80-i+6)%8], A[(80-i+7)%8],
           W[i], K[i] );

but that has almost the same code size as your version. I wonder if there's more to gain there by tweaking P and its auxiliary macros to use temporaries for certain calculations.

@Patater Patater removed needs: review The pull request is ready for review. This generally means that it has no known issues. needs: work The pull request needs rework before it can be merged. labels Aug 23, 2019
@Patater
Copy link
Contributor

Patater commented Sep 4, 2019

CI on merge result is all passing. Jenkins failed to notify GitHub with the result.

@Patater Patater merged commit f66e7ea into ARMmbed:development Sep 4, 2019
@mpg mpg deleted the sha512-smaller branch September 11, 2019 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants