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

Move TI hardware acceleration into Matter repo #9579

Merged
merged 17 commits into from
Oct 8, 2021

Conversation

srickardti
Copy link
Contributor

Problem

Fixes #9176

Change overview

Moved the SHA, AES, and ECJPAKE alt headers and source files into the Matter
repository. Fixed an issue with the semantics of SHA256 context cloning.
Changed the CHIPCryptoPALmbedTLS.cpp implementation to clone the temporary
context back over the original context when done with the non-final
finalization. Increased the SHA context opaque size in CHIPCryptoPAL.h to fit
the TI driver accelerated SHA context.

In future commits it may be possible to share a single SHA driver instance
between all the contexts in mbedtls. This has the disadvantage of requiring a
mutex for driver usage and reference counting. This would also increase
processing time due to the thrashing of the internal digest buffer. But has the
potential advantage that the SHA context can be copied directly with a memcpy.

Testing

Manually tested.

  • lock-app was built
  • example was commissioned onto a Thread network
  • CASE session was established

Moved the SHA, AES, and ECJPAKE alt headers and source files into the Matter
repository. Fixed an issue with the semantics of SHA256 context cloning.
Changed the CHIPCryptoPALmbedTLS.cpp implementation to clone the temporary
context back over the original context when done with the non-final
finalization. Increased the SHA context opaque size in CHIPCryptoPAL.h to fit
the TI driver accelerated SHA context.

In future commits it may be possible to share a single SHA driver instance
between all the contexts in mbedtls. This has the disadvantage of requiring a
mutex for driver usage and reference counting. This would also increase
processing time due to the thrashing of the internal digest buffer. But has the
potential advantage that the SHA context can be copied directly with a memcpy.
src/crypto/CHIPCryptoPAL.h Outdated Show resolved Hide resolved
src/platform/cc13x2_26x2/crypto/aes_alt.c Outdated Show resolved Hide resolved
src/platform/cc13x2_26x2/crypto/aes_alt.c Outdated Show resolved Hide resolved
src/platform/cc13x2_26x2/crypto/aes_alt.c Show resolved Hide resolved
src/platform/cc13x2_26x2/crypto/aes_alt.h Outdated Show resolved Hide resolved
src/platform/cc13x2_26x2/crypto/aes_alt.h Outdated Show resolved Hide resolved
src/platform/cc13x2_26x2/crypto/sha256_alt.h Outdated Show resolved Hide resolved
@jmartinez-silabs
Copy link
Member

Approved, but I second Andrei comment. The worst case should not be applied to kMAX_Hash_SHA256_Context_Size for all platforms. We should use a defined default value that a platform specific define can overrite.

@srickardti
Copy link
Contributor Author

I was also very confused why the Crypto PAL component seemed to compile in the size of context objects and treated them as copy-able objects. But I was not quite confident in my understanding of the side-effects to OpenSSL to start messing with those files.

@woody-apple
Copy link
Contributor

Copy link
Contributor

@tcarmelveilleux tcarmelveilleux left a comment

Choose a reason for hiding this comment

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

Cost of shared context everyone must use is too large.

Suggest:

  • Add a CHIP_CONFIG variable for SHA256 Hardware Context size, used as a default define for kMAX_Hash_SHA256_Context_Size and set to previous value before this change.
  • Update TI examples to use CHIP_CONFIG_xxxx new define and override it to (sizeof(unsigned int) * 76) which would mean ONLY TI platform users would pay that cost.

The TI BLE Manager Impl incorrectly included a header that required cpp linkage.
The Hash_SHA256_stream object is now created on the stack and let to fall out
of context at the end of a translation unit. This causes an issue if the
resources are not freed. Update the mbedtls PAL to initialize and free the
underlying SHA context structure.
@boring-cyborg boring-cyborg bot added the lib label Sep 28, 2021
@srickardti
Copy link
Contributor Author

The failures seem to be CIPD setup in the github pipeline. I don't think I can manually rerun those tests. Should I push a small change to kick them off again?

@github-actions
Copy link

github-actions bot commented Oct 6, 2021

Size increase report for "gn_qpg-example-build" from 8788f6f

File Section File VM
chip-qpg6100-lighting-example.out .text 16 16
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,184
.debug_loc,0,95
.debug_ranges,0,64
.text,16,16
.debug_line,0,13
.debug_abbrev,0,10
.debug_frame,0,8
.debug_str,0,2
[Unmapped],0,-16


@github-actions
Copy link

github-actions bot commented Oct 6, 2021

Size increase report for "esp32-example-build" from 8788f6f

File Section File VM
chip-all-clusters-app.elf .flash.text 16 16
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,113
.debug_loc,0,96
.debug_line,0,70
.debug_ranges,0,32
.debug_abbrev,0,23
.debug_frame,0,16
.flash.text,16,16
.riscv.attributes,0,2
[Unmapped],0,-16


@github-actions
Copy link

github-actions bot commented Oct 6, 2021

Size increase report for "nrfconnect-example-build" from 8788f6f

File Section File VM
chip-lock.elf text 16 16
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,174
.debug_loc,0,124
.debug_ranges,0,64
.debug_line,0,20
text,16,16
.debug_abbrev,0,10
.debug_frame,0,8

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,188
.debug_loc,0,120
.debug_ranges,0,64
.debug_line,0,26
.debug_abbrev,0,10
.debug_frame,0,8


@ti-tpan
Copy link

ti-tpan commented Oct 8, 2021

Hi all,

Thanks for the reviews here!

Any further actions needed before merging this PR (by an authorized user)?

Thanks,
Toby

@msandstedt msandstedt merged commit c4bf82e into project-chip:master Oct 8, 2021
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.

Compilation failure (libChipCrypto) while compiling examples/lock-app/cc13x2x7_26x2x7
10 participants