Driver dispatch Codegen 1.1 #5396

silabs-ArchanaM · 2022-01-09T12:15:31Z

Description

This is rev 1.1 of the Codegen that fixes #5137

Status

IN DEVELOPMENT

Requires Backporting

NO
Which branch?

Migrations

If there is any API change, what's the incentive and logic for it.

YES | NO

Additional comments

Any additional information that could be of interest

Todos

Tests
Documentation
Changelog updated
Backported

Steps to test or reproduce

NA

silabs-ArchanaM · 2022-01-09T12:21:04Z

@gilles-peskine-arm, This PR has pretty much what I had in mind for rev 1.1. It covers, JSON addition, header generation and key management templating.

I expect CI to fail as-is, since the following two seem to be problems
(1) CodeGen now requires jsonschema, would be the sensible thing to run validation against.
(2) With the codegen templating some new constant are generated for driver ID. The way its setup this maps to MBEDTLS_* for the test drivers keeping in mind the nomenclature for the functions, but looks like the scripts are calling the new macros as errors (these are autogen and could ideally be ignored). Do we want to migrate all function names to have a PSA_CRYPTO prefix (that seems like a cleanup task), any thoughts on what the mbedTLS project is aligned towards ?

scripts/generate_driver_wrappers.py

silabs-ArchanaM · 2022-01-11T11:34:51Z

tests/docker/bionic/Dockerfile

+# The version of Pylint is set to 2.4.4 to match CI.
+RUN python3 -m pip install \
+    packaging mypy pylint==2.4.4
+


This fixes #5412 .

I believe that this change is no longer required

Relevant discussion

I haven't removed these changes as yet. Do we want to ?. As I understand it from my discussion with @gilles-peskine-arm this docker is not being used and will at some point be completely removed. I've been running my trial tests on this docker. So if it doesn't hurt, I would like to keep these changes. It's a stand alone commit, so can be knocked off.

I'm ok with having these changes. But please be aware that Arm will not make any effort to maintain this Dockerfile, and we're likely to remove it soon. We maintain and use https://github.com/ARMmbed/mbedtls-test/tree/master/resources/docker_files .

silabs-ArchanaM · 2022-01-11T11:37:16Z

Changelog and Migration guide will be updated towards the end of review.

.travis.yml

scripts/data_files/driver_jsons/mbedtls_test_opaque_driver.json

scripts/data_files/driver_templates/psa_crypto_driver_wrappers.c.jinja

scripts/data_files/driver_jsons/driver_opaque_schema.json

scripts/generate_driver_wrappers.py

minosgalanakis · 2022-02-03T17:49:18Z

.travis.yml

      language: c
      before_install:
        - choco install python --version=3.5.4
+        - pip install jsonschema==3.2.0 types-jsonschema


Any reason we are not using the scripts/min_requirements.py script from the install section?

install: - $PYTHON scripts/min_requirements.py

If it has be provided in the before_install enviroment could we have a comment as to why so?

I would assume min_requirements.py anyway gets triggered by .travis.yml, but on the Windows server, unless I explicitly request for this install, jsonschema is not installed and the CI cries in pain. @gilles-peskine-arm, I think we saw this in the past as well. Am I missing some thing here ?

I've lost track of all the attempts we made in the early draft of this PR, but I think it should be ok to remove this line. On Windows, first the before_install block installs Python, then the global install block installs the Python packages.

Hmmm, this kind of just fixed itself and am very wary when that happens. @gilles-peskine-arm did we somehow force the Python version on the Windows server to not invoke the alternate system python. Any fixes that went in there ?

I checked past versions and couldn't find attempts to do it differently here. Just this patch vs nothing. Failing attempts must have been due to not yet having the modules listed in a requirements file.

If it was necessary to install Python modules this way on Windows, that would also apply to Jinja2.

On the Travis Windows images, there's no system Python. Confusion between the system python and the Travis python are an issue on Linux.

Thanks for checking ! But it did fail at some point and was hence added. Maybe something else tripped me then.

silabs-ArchanaM · 2022-02-11T10:47:12Z

@minosgalanakis, @gilles-peskine-arm any further comments on this PR ?

minosgalanakis · 2022-02-15T16:54:36Z

.travis.yml

+        - pip install jsonschema==3.2.0 types-jsonschema
      env:
        # Add the directory where the Choco packages go
        - PATH=/c/Python35:/c/Python35/Scripts:$PATH


To my knowledge in latest Windows, Python can also be installed to:

C:\Users\username\AppData\Local\Programs\Python\Python35\

Should that also be included in the path?

Python can be installed in any number of places on Windows. Here it only matters where we're installing it (with choco on the line above).

scripts/generate_driver_wrappers.py

minosgalanakis · 2022-02-15T17:39:04Z

scripts/generate_driver_wrappers.py

+        'scripts/data_files/driver_jsons/driver_transparent_schema.json'), 'r') as file:
+        transparent_driver_schema = json.load(file)
+    with open(os.path.join(mbedtls_root, \
+        'scripts/data_files/driver_jsons/driver_opaque_schema.json'), 'r') as file:


As discussed above

scripts/generate_driver_wrappers.py

minosgalanakis · 2022-02-17T16:01:18Z

I have added few comments, which can be addressed when reworking the changes that @gilles-peskine-arm requested.

gilles-peskine-arm

There's a sentence in the documentation that needs updating. Other than that, looks good to me.

In terms of design, I think there's too much intelligence in the template files, where the meta-logic is mixed with the C code. I'd prefer to have this intelligence in a single place, in the Python script. But this can be improved incrementally in the future.

scripts/generate_driver_wrappers.py

Signed-off-by: Asfandyar Orakzai <[email protected]>

gilles-peskine-arm

Thanks for the update, LGTM

minosgalanakis

Overall it looks good, and is fit for purpose.

Should we have a changelog entry before merging though?

Signed-off-by: Asfandyar Orakzai <[email protected]>

asfand-silabs · 2022-11-02T15:37:12Z

Overall it looks good, and is fit for purpose.

Should we have a changelog entry before merging though?

Updated change log

gilles-peskine-arm · 2022-11-02T16:46:46Z

ChangeLog

     and hmac_demo.c, which use PSA and the md/cipher interfaces side
     by side in order to illustrate how the operation is performed in PSA.
     Addresses #5208.
+   * Brought in PSA code gen driver list JSON,


plz no txtspk in chglog

Also you need to add a file to Changelog.d, don't edit ChangeLog directly (if we edit ChangeLog directly we get conflicts between all the pull requests that do it).

@gilles-peskine-arm Changelog issue is addressed, if there is anything please let me.

Signed-off-by: Asfandyar Orakzai <[email protected]>

gilles-peskine-arm

LGTM

asfand-silabs · 2022-11-04T19:07:45Z

LGTM

Thanks, What is merging strategy?

gilles-peskine-arm · 2022-11-04T20:08:12Z

@asfand-silabs Waiting for Minos to approve the latest version, then one of us will push the merge button. Thanks for the updates!

minosgalanakis

LGTM

gstrauss · 2022-11-12T16:26:59Z

FYI: #5396 or #6577 broke my build environment due to a new dependency for the build.

I had to install the python3-jsonschema package on my system.

gilles-peskine-arm · 2022-11-12T19:20:10Z

@gstrauss Sorry about that. We didn't announce it in the changelog because it only affects people who use the development branch, not people who use a release (the generated file is included in the release). We should probably have a place where we can communicate this. Maybe add a changelog section for people who use development? Or just a small number of entries that are not applicable to people who use releases? (Noting that you need to consult ChangeLog.d/* between releases, not just ChangeLog.)

For future reference, Python dependencies are listed in scripts/*.requirements.txt. If you just want to build the library (and the tests), the official source of truth is basic.requirements.txt (following any included files). We should make this information more visible, any suggestion how?

gilles-peskine-arm · 2022-11-12T19:21:31Z

(Note that this information is in README.md. But for long-time users, there's a bootstrap problem that while users might be expected to read README.md, it's too much to ask for existing users to re-read it every time.)

gstrauss · 2022-11-13T03:22:46Z

@gilles-peskine-arm as you can see, I was able to figure it out. README.md did not change for this added dependency, and it probably should not have changed. As you point out, README.md already points to python -m pip install -r scripts/basic.requirements.txt.

The reason I posted is that the requirements changed underneath me and it was not immediately obvious. I do not frequently rebuild my test environment from scratch.

When things broke for me, I did check for differences in ChangeLog.d/*, so perhaps that might be a place to note development changes which might impact others, though doing so is not necessary for all development changes. Just a suggestion since you asked.

gilles-peskine-arm reviewed Jan 10, 2022

View reviewed changes

scripts/generate_driver_wrappers.py Show resolved Hide resolved

silabs-ArchanaM force-pushed the codegen_1.1 branch 2 times, most recently from 86cc89a to da77e04 Compare January 10, 2022 11:21

yanesca added Community component-psa PSA keystore/dispatch layer (storage, drivers, …) labels Jan 10, 2022

silabs-ArchanaM force-pushed the codegen_1.1 branch 2 times, most recently from 6dc91cf to 2585023 Compare January 11, 2022 11:32

silabs-ArchanaM commented Jan 11, 2022

View reviewed changes

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) labels Jan 12, 2022

silabs-ArchanaM force-pushed the codegen_1.1 branch from 2585023 to 71df609 Compare January 12, 2022 12:59

febdoctor mentioned this pull request Jan 17, 2022

Psa entropy #5437

Draft

3 tasks

minosgalanakis self-requested a review February 3, 2022 09:10

ronald-cron-arm mentioned this pull request Feb 3, 2022

Proposal for driver dispatch code gen #5067

Closed

gilles-peskine-arm requested changes Feb 3, 2022

View reviewed changes

minosgalanakis reviewed Feb 3, 2022

View reviewed changes

gilles-peskine-arm mentioned this pull request Feb 7, 2022

PSA driver description spec: minor tweaks to the JSON format #5512

Merged

minosgalanakis reviewed Feb 15, 2022

View reviewed changes

silabs-ArchanaM force-pushed the codegen_1.1 branch 3 times, most recently from 3dc6d21 to cc1122a Compare February 28, 2022 01:26

silabs-ArchanaM force-pushed the codegen_1.1 branch 2 times, most recently from b489a1a to 2ae9376 Compare March 14, 2022 10:42

gilles-peskine-arm self-requested a review March 14, 2022 10:52

gilles-peskine-arm requested a review from minosgalanakis September 19, 2022 11:42

gilles-peskine-arm requested changes Oct 10, 2022

View reviewed changes

scripts/generate_driver_wrappers.py Outdated Show resolved Hide resolved

gilles-peskine-arm added the needs-work label Oct 10, 2022

fixed docstring sentence

6eaa75a

Signed-off-by: Asfandyar Orakzai <[email protected]>

gilles-peskine-arm previously approved these changes Oct 10, 2022

View reviewed changes

gilles-peskine-arm removed the needs-work label Oct 10, 2022

minosgalanakis previously approved these changes Nov 2, 2022

View reviewed changes

Update changed log

9bd28dc

Signed-off-by: Asfandyar Orakzai <[email protected]>

asfand-silabs dismissed stale reviews from minosgalanakis and gilles-peskine-arm via 9bd28dc November 2, 2022 15:36

gilles-peskine-arm requested changes Nov 2, 2022

View reviewed changes

asfand-silabs added 4 commits November 2, 2022 21:50

Fixed change log issue

ee2b637

Signed-off-by: Asfandyar Orakzai <[email protected]>

fixed changelog formating

4f63ac4

Signed-off-by: Asfandyar Orakzai <[email protected]>

fixed formating issues in psa_crypto_code_gen_1_1.txt

65cd8a4

Signed-off-by: Asfandyar Orakzai <[email protected]>

removed stray whitespaces from change logs

9b656d3

Signed-off-by: Asfandyar Orakzai <[email protected]>

gilles-peskine-arm approved these changes Nov 4, 2022

View reviewed changes

gilles-peskine-arm requested a review from minosgalanakis November 4, 2022 14:38

minosgalanakis approved these changes Nov 7, 2022

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 7, 2022

gilles-peskine-arm merged commit 34c0946 into Mbed-TLS:development Nov 7, 2022

asfand-silabs deleted the codegen_1.1 branch November 9, 2022 09:44

gstrauss mentioned this pull request Nov 12, 2022

Bignum: Add BignumModRaw Operation base classes #6577

Merged

3 tasks

Driver dispatch Codegen 1.1 #5396

Driver dispatch Codegen 1.1 #5396

Uh oh!

Conversation

silabs-ArchanaM commented Jan 9, 2022

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

Uh oh!

silabs-ArchanaM commented Jan 9, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silabs-ArchanaM commented Jan 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silabs-ArchanaM commented Feb 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

minosgalanakis commented Feb 17, 2022

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

asfand-silabs commented Nov 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment