Add support for TF-PSA-Crypto test driver #243

ronald-cron-arm · 2025-12-01T13:51:11Z

Description

Add support for driver dispatch testing in TF-PSA-Crypto

PR checklist

TF-PSA-Crypto PR provided Add support for driver dispatch testing 2/2 TF-PSA-Crypto#586
development PR not required because: TF-PSA-Crypto only work
3.6 PR not required because: TF-PSA-Crypto only work

Update test drivers for TF-PSA-Crypto test driver based on a copy of `drivers/builtin`. We mainly keep using what is already in place for libtestdriver1 in Mbed TLS by still defining MBEDTLS_TEST_LIBTESTDRIVER1. As in TF-PSA-Crypto, the test driver is a derivative of a clone of the built-in driver, not of the whole library, the paths to the driver internal headers are different. The paths are relative to `drivers/libtestdriver1/include`. Otherwise, `psa_key_attributes_t` is prefixed with `libtestdriver1_` in Mbed TLS test driver but not in TF-PSA-Crypto one. Thus in TF-PSA-Crypto case define `libtestdriver1_psa_key_attributes_t` as `psa_key_attributes_t`. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Add get_identifiers_with_prefixes method that returns the list of identifiers that potentially need to be prefixed in the test driver code. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

gilles-peskine-arm

I've fully reviewed 61f15a6, as well as the Python code in Mbed-TLS/TF-PSA-Crypto#586.

I like the methodology, but the Python code sometimes feels hard to follow compared to what it does.

gilles-peskine-arm · 2025-12-03T17:00:20Z

tests/src/drivers/test_driver_aead.c


-#if defined(MBEDTLS_TEST_LIBTESTDRIVER1)
+#if defined(TF_PSA_CRYPTO_TEST_LIBTESTDRIVER1)
+typedef psa_key_attributes_t libtestdriver1_psa_key_attributes_t;


Since this is repeated, it feels like it should be in a header somewhere, probably tests/include/test/drivers/test_driver_common.h.

I agree with @gilles-peskine-arm

What the code is effectively trying to achieve is rewrite the include path and include the appropriate header depending on the variant of the libtestdriver.

That logic alongside the typedef could easily reside in a separate header and exposed thought a getter macro e.g LIBTESTDRIVER1_INCLUDE(psa_crypto_hash)

gilles-peskine-arm · 2025-12-03T17:08:36Z

tests/src/drivers/test_driver_aead.c

-#if defined(MBEDTLS_TEST_LIBTESTDRIVER1)
+#if defined(TF_PSA_CRYPTO_TEST_LIBTESTDRIVER1)
+typedef psa_key_attributes_t libtestdriver1_psa_key_attributes_t;
+#include "../../libtestdriver1/src/psa_crypto_aead.h"


It took me a while to understand this path. It's weird to go up two levels. My first thought was that the include path was relative to the file that this directive is in, but that didn't work, and a path relative to the framework would be fragile since it could be different in in-tree vs out-of-tree builds. Then I noticed the commit message

The paths are relative to drivers/libtestdriver1/include.

Ok, that makes sense, but then why not ../src/psa_crypto_aead.h? But that wouldn't necessarily work since it could pick up the one under /drivers/builtin. Ok, then it does have to be what you wrote, although I would prefer

#include <../../libtestdriver1/src/psa_crypto_aead.h>

since we know this isn't relative to the location of the file, it comes from the include path.

And I would like this explained in a comment.

+1. When people will need to touch the code in the future they likely wont consider looking at the comment history using git log --follow. If its not intuitive it should be a comment

gilles-peskine-arm · 2025-12-03T17:12:23Z

scripts/mbedtls_framework/test_driver.py

+    parser = argparse.ArgumentParser(description= \
+        "Clone partially builtin tree, rewrite header inclusions and prefix"
+        "exposed C identifiers.")


The string contains prefixexposed without a space. Use triple-quotes for such long strings.

Suggested change

parser = argparse.ArgumentParser(description= \

"Clone partially builtin tree, rewrite header inclusions and prefix"

"exposed C identifiers.")

parser = argparse.ArgumentParser(description="""\

Partially clone builtin tree, rewrite header inclusions and prefix exposed C

identifiers.

""")

(I also fixed the grammar, since AFAICT this isn't about cloning trees that are partially built-in, but about partially cloning the builtin tree.)

gilles-peskine-arm · 2025-12-03T17:14:45Z

scripts/mbedtls_framework/test_driver.py

+                        help="Destination directory.\n"
+                        " - If absolute, used as-is.\n"
+                        " - If relative, interpreted relative to the repository root.\n")


argparse doesn't preserve line breaks in help text, so you can't format with bullet points. I think it could be shorter anyway.

Suggested change

help="Destination directory.\n"

" - If absolute, used as-is.\n"

" - If relative, interpreted relative to the repository root.\n")

help="Destination directory (relative to repository root)"

gilles-peskine-arm · 2025-12-03T17:23:19Z

scripts/mbedtls_framework/test_driver.py

+    for directory in ("include", "src"):
+        directory_path = root / directory
+        for ext in (".c", ".h"):
+            yield from directory_path.rglob(f"*{ext}")


rglob doesn't have deterministic ordering, which can make debugging annoying. On the other hand, you take care to emit .c files before .h files, is there a reason for that?

How about something like

return sorted(path for directory in ('include', 'src') for path in (root / directory).rglob('*.[hc]')

No particular reason to use "for ext in (".c", ".h"):" over "*.[hc]".

gilles-peskine-arm · 2025-12-03T18:01:06Z

scripts/mbedtls_framework/test_driver.py

+            #include "libtestdriver1/private/aes.h"
+        """
+        include_line_re = re.compile(
+            fr'^\s*#\s*include\s*([<"])\s*{src_include_dir}/([^>"]+)\s*([>"])',


You can't have spaces inside the quotes/brackets.

Suggested change

fr'^\s*#\s*include\s*([<"])\s*{src_include_dir}/([^>"]+)\s*([>"])',

fr'^\s*#\s*include\s*([<"]){src_include_dir}/([^>"]+)([>"])',

gilles-peskine-arm · 2025-12-03T18:06:56Z

scripts/mbedtls_framework/test_driver.py

+                return f'#include {m.group(1)}{driver}/{header}{m.group(3)}'
+            return m.group(0)
+
+        new_text = include_line_re.sub(repl, text)


This could be simplified to something like (untested):

Suggested change

new_text = include_line_re.sub(repl, text)

renamed_headers = '|'.join(re.escape(header) for header in headers)

include_line_re = re.compile(fr'^(\s*#\s*include\s*["<])((?:{renamed_headers})[">])', re.M)

new_text, changed = include_line_re.subn(repl, fr'\1{driver}-\2')

I tried something similar but this is significantly slower. I guess because of the huge regular expressions.

gilles-peskine-arm · 2025-12-03T18:12:23Z

scripts/mbedtls_framework/test_driver.py

+        c_identifier_re = re.compile(r"\b[A-Za-z_][A-Za-z0-9_]*\b")
+        text = file.read_text(encoding="utf-8")
+        prefix_uppercased = prefix.upper()
+        prefix_lowercased = prefix.lower()
+
+        def repl(m: Match) -> str:
+            identifier = m.group(0)
+            if identifier in identifiers:
+                if identifier[0].isupper():
+                    return f"{prefix_uppercased}_{identifier}"
+                else:
+                    return f"{prefix_lowercased}_{identifier}"
+            return identifier
+
+        new_text = c_identifier_re.sub(repl, text)


You can make regexps do the work:

Suggested change

c_identifier_re = re.compile(r"\b[A-Za-z_][A-Za-z0-9_]*\b")

text = file.read_text(encoding="utf-8")

prefix_uppercased = prefix.upper()

prefix_lowercased = prefix.lower()

def repl(m: Match) -> str:

identifier = m.group(0)

if identifier in identifiers:

if identifier[0].isupper():

return f"{prefix_uppercased}_{identifier}"

else:

return f"{prefix_lowercased}_{identifier}"

return identifier

new_text = c_identifier_re.sub(repl, text)

lower_identifier_re = re.compile(r'\b(' + '|'.join(ident for ident in identifiers if ident[0].islower()) + r')\b')

upper_identifier_re = re.compile(r'\b(' + '|'.join(ident for ident in identifiers if ident[0].isupper()) + r')\b')

text = file.read_text(encoding="utf-8")

prefix_uppercased = prefix.upper()

prefix_lowercased = prefix.lower()

new_text = lower_identifier_re.sub(prefix.lower() + r'_\1',

upper_identifier_re.sub(prefix.upper() + r'_\1', text))

I tried something similar but this is significantly slower. I guess because of the huge regular expressions.

gilles-peskine-arm · 2025-12-03T18:23:23Z

scripts/mbedtls_framework/test_driver.py

+        Only the `include` and `src` directories from `src_dir` are used to build
+        the test driver tree, and their directory structure is preserved.
+
+        Only "*.h" and "*.c" files are copied. Files whose names match any of the


“file name” either refers to the base name or to the full path (not necessarily absolute), depending on who you ask. Please clarify.

Suggested change

Only "*.h" and "*.c" files are copied. Files whose names match any of the

Only "*.h" and "*.c" files are copied. Files whose basenames match any of the

gilles-peskine-arm · 2025-12-03T18:43:33Z

scripts/mbedtls_framework/test_driver.py

+        for ext in (".c", ".h"):
+            yield from directory_path.rglob(f"*{ext}")
+
+def run_ctags(file: Path) -> Set[str]:


“Run ctags” is how the function works, not its purpose. How about something like

Suggested change

def run_ctags(file: Path) -> Set[str]:

def get_c_identifiers(file: Path) -> Set[str]:

minosgalanakis

Functionally works, but there is some work required for the design

minosgalanakis · 2025-12-04T14:54:56Z

tests/src/drivers/test_driver_aead.c


-#if defined(MBEDTLS_TEST_LIBTESTDRIVER1)
+#if defined(TF_PSA_CRYPTO_TEST_LIBTESTDRIVER1)
+typedef psa_key_attributes_t libtestdriver1_psa_key_attributes_t;


I agree with @gilles-peskine-arm

What the code is effectively trying to achieve is rewrite the include path and include the appropriate header depending on the variant of the libtestdriver.

That logic alongside the typedef could easily reside in a separate header and exposed thought a getter macro e.g LIBTESTDRIVER1_INCLUDE(psa_crypto_hash)

minosgalanakis · 2025-12-04T14:55:48Z

tests/src/drivers/test_driver_aead.c

-#if defined(MBEDTLS_TEST_LIBTESTDRIVER1)
+#if defined(TF_PSA_CRYPTO_TEST_LIBTESTDRIVER1)
+typedef psa_key_attributes_t libtestdriver1_psa_key_attributes_t;
+#include "../../libtestdriver1/src/psa_crypto_aead.h"


+1. When people will need to touch the code in the future they likely wont consider looking at the comment history using git log --follow. If its not intuitive it should be a comment

minosgalanakis · 2025-12-04T14:57:37Z

tests/src/drivers/test_driver_key_management.c


-#if defined(MBEDTLS_TEST_LIBTESTDRIVER1)
+#if !defined(TF_PSA_CRYPTO_TEST_LIBTESTDRIVER1) && \
+    defined(MBEDTLS_TEST_LIBTESTDRIVER1)


If we end up using a helper header file, this logic and other variants can also be simplified by a macro or symbol defined there.

minosgalanakis · 2025-12-04T14:59:03Z

tests/src/drivers/test_driver_signature.c

    mbedtls_test_driver_signature_verify_hooks = MBEDTLS_TEST_DRIVER_SIGNATURE_INIT;

-psa_status_t sign_hash(
+static psa_status_t sign_hash(


Is that a minor bugfix unreleated to the previous changes? Or required because multiple references of the symbol were included?

minosgalanakis · 2025-12-04T15:55:13Z

scripts/mbedtls_framework/test_driver.py

+        The source directory `src_dir` is expected to have the following structure:
+        - an `include` directory
+        - an `src` directory
+        - the `include` directory contains exactly one subdirectory


I think this is needed because of the line:

(self.dst_dir / "include" / src_include_dir_name).rename( \ self.test_driver_include_dir)

which will rename the drivers/builtin/include/mbedtls -> drivers/builtin/include/libtestdrive1

minosgalanakis · 2025-12-04T16:13:17Z

scripts/mbedtls_framework/test_driver.py

+        if exclude_files is None:
+            exclude_files = set()
+
+        for file in iter_code_files(src_dir):


+1 Currently the logic required to create this tree is split across two repositories, with tf-psa-crypto/tests/scripts/generate_test_driver.py responsbile to call generator.prefix_identifiers(identifiers). We have historically been having troubles maintaing split logic tools.

The way the data is linked is that build_tree stores data to 'self.dst_dir' which is parsed by get_identifiers_with_prefixes and get_external_identifiers to extract the contents of that to generate_test_driver, allow it to append some identifiers to the list, and call the prefix_identifiers to write them in the file.

I propose we give the TestDriveGeneration information ahead of time, and let it do its thing without having to know how it works from the caller side.

As such a simplistic approach on the api should be:

create the instance: generator = test_driver.TestDriverGenerator(dst_dir, args.driver)

Configure it with the infomation it needs. Provide the EXCLUDE_FILES, extra identifies that need to be added like MBEDTLS_ECP_LIGHT, set the rename rules e.g PSA_WANT_ -> MBEDTLS_PSA_ACCEL_. This will need a new .config method.

Invoke generator.build_tree or something equivalent that will do all the steps, without exposing the logic to the calling script.

minosgalanakis · 2025-12-04T16:15:19Z

scripts/mbedtls_framework/test_driver.py

+                continue
+            dst = self.dst_dir / file.relative_to(src_dir)
+            dst.parent.mkdir(parents=True, exist_ok=True)
+            shutil.copy2(file, dst)


I would keep and return a list of all the files that have been created

We have "--list-vars-for-cmake" for that I'd say.

ronald-cron-arm added 3 commits December 1, 2025 11:53

Introduce test_driver.py

75540ee

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

test_driver.py: Add method to build the test driver tree

4eeffe8

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm mentioned this pull request Dec 1, 2025

Add support for driver dispatch testing 2/2 Mbed-TLS/TF-PSA-Crypto#586

Open

4 tasks

ronald-cron-arm added enhancement New feature or request needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Dec 1, 2025

ronald-cron-arm force-pushed the test-driver branch from c01d250 to 61f15a6 Compare December 1, 2025 15:53

ronald-cron-arm added 2 commits December 1, 2025 23:15

test_driver.py: Add get_identifiers_with_prefixes method

b14ccde

Add get_identifiers_with_prefixes method that returns the list of identifiers that potentially need to be prefixed in the test driver code. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

test_driver.py: Add method to prefix test driver exposed identifiers

6ffca2e

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm force-pushed the test-driver branch from 61f15a6 to 6ffca2e Compare December 1, 2025 22:28

ronald-cron-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 and removed needs-ci Needs to pass CI tests labels Dec 2, 2025

ronald-cron-arm mentioned this pull request Dec 2, 2025

Add support for driver dispatch testing 1/2 Mbed-TLS/TF-PSA-Crypto#587

Open

4 tasks

ronald-cron-arm added this to Roadmap pull requests (new board) Dec 2, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Dec 2, 2025

ronald-cron-arm requested review from gilles-peskine-arm and minosgalanakis December 2, 2025 11:39

ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Dec 2, 2025

gilles-peskine-arm requested changes Dec 3, 2025

View reviewed changes

minosgalanakis reviewed Dec 4, 2025

View reviewed changes

ronald-cron-arm added the needs-work label Dec 5, 2025

	fr'^\s#\sinclude\s([<"])\s{src_include_dir}/([^>"]+)\s*([>"])',
	fr'^\s#\sinclude\s*([<"]){src_include_dir}/([^>"]+)([>"])',

-        new_text = include_line_re.sub(repl, text)
+        renamed_headers = '|'.join(re.escape(header) for header in headers)
+        include_line_re = re.compile(fr'^(\s*#\s*include\s*["<])((?:{renamed_headers})[">])', re.M)
+        new_text, changed = include_line_re.subn(repl, fr'\1{driver}-\2')

	Only ".h" and ".c" files are copied. Files whose names match any of the
	Only ".h" and ".c" files are copied. Files whose basenames match any of the

	def run_ctags(file: Path) -> Set[str]:
	def get_c_identifiers(file: Path) -> Set[str]:

Add support for TF-PSA-Crypto test driver #243

Are you sure you want to change the base?

Add support for TF-PSA-Crypto test driver #243

Uh oh!

Conversation

ronald-cron-arm commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

Uh oh!

gilles-peskine-arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

minosgalanakis left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ronald-cron-arm commented Dec 1, 2025 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading