Codesize exclude psa wrappers #7980

xkqian · 2023-07-25T06:20:28Z

Description

When psa_crypto_driver_wrappers.c is created from psa_crypto_driver_wrappers.c.jinja but neither PSA_CRYPTO_ACCELERATOR_DRIVER_PRESENT nor PSA_CRYPTO_DRIVER_TEST are set, it should be possible to omit the entire wrapper function.
This PR will provide one prototype which can omit some wrapper functions, such as 2~3 functions.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog ~~provided, or~~ not required (Part of the epic code size)
backport ~~done, or~~ not required (Refine of code, 2.28 no needed)
tests ~~provided, or~~ not required(current test cases are enough)

tom-cosgrove-arm · 2023-08-29T08:23:46Z

@bensze01 The ABI-API checker is (reasonably) complaining that the driver wrapper API functions are no longer visible. However, they are internal, and this is an expected outcome from this PR.

Unfortunately, that causes the CI to fail. Will we just have to bypass merge queues when it comes time to merge this PR, or can we make the ABI-API check "not fatal"?

gilles-peskine-arm · 2023-08-29T08:39:36Z

@tom-cosgrove-arm The ABi-API check is not fatal. The checks that have to pass for the merge queue are the ones that have the “Required” annotation.

gilles-peskine-arm · 2023-09-05T11:10:59Z

Makefile

@@ -14,7 +14,7 @@ no_test: programs
 programs: lib mbedtls_test
 	$(MAKE) -C programs

-lib:
+lib: generated_files


In principle, this is a good thing: it's good to ensure that the generated files are up-to-date. However this is likely to cause problems for users who work in an environment where they can't re-generate the files (no python, or not the required python modules, or no host C compiler), and where the timestamps on the generated files aren't newer than the primary sources (because the timestamps is in the random order in which the files were copied).

This is more or less went wrong in https://jenkins-mbedtls.oss.arm.com/blue/organizations/jenkins/mbed-tls-pr-head/detail/PR-7980-head/12/pipeline/582 : in this case, the build environment doesn't support make generated_files (although that's actually a bug in the makefile). Nothing would need to actually be rebuilt since the timestamps of generated files are newer than primary sources, but in this case, the failure happens before make can check this.

I think the main issue (if make runs ok but the code to generate the files might fail) can be resolved with order-only prerequisites. But it would have to be on generated_files in the sub-makefiles: an order-only prerequisite doesn't make sense when the prerequisite is phony.

This is a rather complicated makefile change which should be done in its own pull request. Do you really need it here?

Agreed, not in scope for this PR (but also agreed it's potentially a nice improvement for a separate PR, if you have some spare background time for it).

Agree. I will revert it and file one issue #8163 about this case: https://jenkins-mbedtls.oss.arm.com/blue/organizations/jenkins/mbed-tls-pr-head/detail/PR-7980-head/12/pipeline/582 , in case we need one new PR to solve this.

daverodgman · 2023-09-06T08:06:32Z

@xkqian is this ready for review? Please mark as needs-review and needs-reviewer if it is

xkqian · 2023-09-06T10:33:14Z

@xkqian is this ready for review? Please mark as needs-review and needs-reviewer if it is

I will put it in review once it pass the CI.

yuhaoth

psa_crypto_driver_wrappers.c should renamed to header file(.h) . It is included in another file now, it might confuse others with .c

library/CMakeLists.txt

scripts/data_files/driver_templates/psa_crypto_driver_wrappers.c.jinja

gilles-peskine-arm · 2023-09-07T16:25:03Z

library/psa_crypto_driver_wrappers_no_static.c

+/*
+ *  Functions to delegate cryptographic operations to an available
+ *  and appropriate accelerator.
+ *  Warning: This file is now auto-generated.


Apparently this file is not generated? I don't understand why.

Good point! Originally I find only there are 3 functions in this file, so just put them in the .c file, If we want to generate automatically, we should change the generate script and the the json schema, and maybe the document, that will be one big change. Anyway, I will re-work and try to generate it.

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian · 2023-09-26T09:11:10Z

@xkqian please rebase onto development to pick up the CI fixes and hopefully get a clean CI run

Rebase done and wait for the CI result.

yuhaoth

LGTM

gilles-peskine-arm · 2023-12-19T13:59:21Z

library/Makefile

+$(GENERATED_WRAPPER_FILES): ../scripts/generate_driver_wrappers.py
+$(GENERATED_WRAPPER_FILES): ../scripts/data_files/driver_templates/psa_crypto_driver_wrappers.h.jinja
+$(GENERATED_WRAPPER_FILES): ../scripts/data_files/driver_templates/psa_crypto_driver_wrappers_no_static.c.jinja


$(gen_file_dep) got lost here.

xkqian added enhancement needs-work component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Jul 25, 2023

xkqian linked an issue Jul 25, 2023 that may be closed by this pull request

Code Size: Driver Wrappers: Prototype mechanism #7395

Closed

xkqian mentioned this pull request Jul 25, 2023

Codesize exclude psa wrapper tom-daubney-arm/mbedtls#1

Closed

3 tasks

xkqian force-pushed the codesize_exclude_psa_wrappers branch from 7a15203 to 02cc720 Compare August 1, 2023 02:52

tom-cosgrove-arm mentioned this pull request Aug 8, 2023

Exclude Driver Wrapper Functions Prototype - Alternative implementation #7875

Closed

xkqian force-pushed the codesize_exclude_psa_wrappers branch 3 times, most recently from bd1d633 to 69d361f Compare August 21, 2023 10:42

xkqian force-pushed the codesize_exclude_psa_wrappers branch from 5313e2e to 0633620 Compare September 5, 2023 01:43

gilles-peskine-arm reviewed Sep 5, 2023

View reviewed changes

gilles-peskine-arm mentioned this pull request Sep 6, 2023

Compile failure with windows-mingw once we want to update the generated files before make lib #8163

Open

xkqian force-pushed the codesize_exclude_psa_wrappers branch 3 times, most recently from 2aa64d2 to 4425867 Compare September 7, 2023 07:43

yuhaoth suggested changes Sep 7, 2023

View reviewed changes

library/CMakeLists.txt Show resolved Hide resolved

scripts/data_files/driver_templates/psa_crypto_driver_wrappers.c.jinja Outdated Show resolved Hide resolved

gilles-peskine-arm reviewed Sep 7, 2023

View reviewed changes

xkqian force-pushed the codesize_exclude_psa_wrappers branch 6 times, most recently from 9c44da9 to a7a247c Compare September 12, 2023 03:50

xkqian added 12 commits September 26, 2023 09:09

Change the extension type of the file psa_crypto_driver_wrapper

fe9666b

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Change comments to psa_crypto_driver_wrappers.h

845693c

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Change include guards of psa_crypto_driver_wrappers_no_static.h

1b61d6e

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Move the dependency adjacent to the generated file

0e5b53c

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Change to os.path functions base on comment

7d546c7

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Remove the UN_USED_DISABLE attribute

c23c351

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Remove useless empty line

9e11126

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Remove useless spaces in Makefile

b909aea

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Change the description of auto-generated driver dispatch files

1198e43

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

No need to include the 3rd party entry point head file

e9dc63e

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Change the documenti about psa_crypto_driver_wrappers.c{h}

76e55a2

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Fix a typo in psa-crypto-implementation-structure.md

db3035b

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian force-pushed the codesize_exclude_psa_wrappers branch from ca444b3 to db3035b Compare September 26, 2023 09:10

daverodgman approved these changes Sep 26, 2023

View reviewed changes

daverodgman requested a review from gabor-mezei-arm September 26, 2023 09:25

yuhaoth approved these changes Sep 26, 2023

View reviewed changes

daverodgman added needs-ci Needs to pass CI tests approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, labels Sep 26, 2023

daverodgman merged commit d472a8f into Mbed-TLS:development Sep 26, 2023
4 of 6 checks passed

xkqian linked an issue Sep 27, 2023 that may be closed by this pull request

Code Size: Driver Wrappers: Implementation #7396

Closed

gilles-peskine-arm mentioned this pull request Sep 27, 2023

Test making generated files with cmake #8256

Closed

This was referenced Sep 27, 2023

Port CMake code for driver wrapper exclusion Mbed-TLS/TF-PSA-Crypto#66

Closed

Add CMake include path for generated header #8261

Merged

xkqian mentioned this pull request Sep 28, 2023

Code Size: Driver Wrappers: Implementation #7396

Closed

davidhorstmann-arm mentioned this pull request Sep 28, 2023

Update CMake driver wrappers code Mbed-TLS/TF-PSA-Crypto#70

Merged

tom-cosgrove-arm added the size-optimisation label Oct 4, 2023

gilles-peskine-arm reviewed Dec 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Codesize exclude psa wrappers #7980

Codesize exclude psa wrappers #7980

xkqian commented Jul 25, 2023

tom-cosgrove-arm commented Aug 29, 2023

gilles-peskine-arm commented Aug 29, 2023

gilles-peskine-arm Sep 5, 2023

daverodgman Sep 6, 2023

xkqian Sep 6, 2023

daverodgman commented Sep 6, 2023

xkqian commented Sep 6, 2023

yuhaoth left a comment

gilles-peskine-arm Sep 7, 2023

xkqian Sep 8, 2023

xkqian commented Sep 26, 2023

yuhaoth left a comment

gilles-peskine-arm Dec 19, 2023

Codesize exclude psa wrappers #7980

Codesize exclude psa wrappers #7980

Conversation

xkqian commented Jul 25, 2023

Description

PR checklist

tom-cosgrove-arm commented Aug 29, 2023

gilles-peskine-arm commented Aug 29, 2023

gilles-peskine-arm Sep 5, 2023

Choose a reason for hiding this comment

daverodgman Sep 6, 2023

Choose a reason for hiding this comment

xkqian Sep 6, 2023

Choose a reason for hiding this comment

daverodgman commented Sep 6, 2023

xkqian commented Sep 6, 2023

yuhaoth left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Sep 7, 2023

Choose a reason for hiding this comment

xkqian Sep 8, 2023

Choose a reason for hiding this comment

xkqian commented Sep 26, 2023

yuhaoth left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Dec 19, 2023

Choose a reason for hiding this comment