Cortex M33: set DOMAIN_NS to 0 when TZ is not used

[jeromecoutant]

Description

This could fix #9460 ?

If TFM is not used for a V8M target:

• cmse compilation option is not set
• DOMAIN_NS is explitely defined for non-NS targets

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-psa
@LMESTM
@kjbracey-arm

[jeromecoutant]

@MarceloSalazar

[kjbracey]

This looks plausible, and might be sufficient, but I'm not familiar enough with where else assumptions occur in the tools. I guess the "TFM" check will do - "TrustZone iff TFM" seems a sounder assumption than "TrustZone iff ARMv8" at least.

Does DOMAIN_NS actually need to be explicitly defined to 0? All the code I can see seems to assume 0 or set it to 0 if not defined. (I have a feeling this came up before, but can't find it).

[kjbracey]

If testing TFM, I would take out the CORE_ARCH test

[alzix]

please consider following approach:

Suggested change

	if CORE_ARCH[target.core] == 8 and "TFM" in target.labels:
	if CORE_ARCH[target.core] == 8 and target.is_PSA_secure_target:

We are only injecting this define to a linker script. Please consider adding this macro also for compile flags for consistency.

[kjbracey]

What I'd like to see is a core-independent "we're making a TrustZone/CMSE-type split image" test here.

If I understand correctly, is_PSA_secure_target covers TrustZone and dual-core solutions, and there's no fundamental reason why someone might not end up making a dual-M33 device.

So "TFM" is closer to the meaning we want. In #9460 I suggested adding Target.is_TZ_secure_target and Target.is_TZ_nonsecure_target for this bit of code.

If not changing anything else, those could be defined as 'TFM' in self.labels and [not] self.core.ends_with('-NS')

Although for all of the three possibilities suggested here, we're not covering "NUMAKER_PFM_M2351", which doesn't have TFM/PSA.

But then that's why @bridadan was looking at adding a separate explicit tz flag.

[jeromecoutant]

@alzix proposition is fine for me

[jeromecoutant]

is

if CORE_ARCH[target.core] == 8 and (target.is_PSA_secure_target or "MBED_TZ_DEFAULT_ACCESS=1" in target.macros):

would be fine for all ?

[kjbracey]

I really don't want CORE_ARCH in there, on principle. The tying of security to core version is the whole problem we're trying to fix, so even if this is a temporary "heuristic" until a more explicit flag is added for targets.json, I want that to be an architecture-free heuristic if at all possible.

We can cover M2351's use of NS by just detecting the "-NS" suffix on the core, without caring about any other settings.

I believe it should be flattened out, a bit like my initial comment on #9460, so it's something like

if `TFM` in target.labels:
    if not target.core.endswith("-NS"):
        # XXX Why is this conditional? Don't we always create secure library
        # if setting secure build flag?
        if (kwargs.get('build_dir', False)):  
            # Create Secure library

        # Add secure build flag
    # No else? No special handling for TFM NS images, I think?

if target.core.ends_with('-NS'):
    # Add linking time preprocessor macro DOMAIN_NS

I didn't quite get @alzix's objection to looking at label TFM rather than SPE_Target or NSPE_Target.

I think the above makes sense as a heuristic for now, as the only secure image we support building is a TFM one, but we don't really care why anyone might choose to make a NS image. They're free to use whatever secure library they want, I think.

[kjbracey]

Oh, and I still don't think there's any need to be explicitly setting DOMAIN_NS to 0, but let me know if I'm wrong.

[ciarmcom]

@jeromecoutant, thank you for your changes.
@kjbracey-arm @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

[jeromecoutant]

Let's use this PR to have a "quick" workaround,
and let's keep #9460 to get a clean build architecture
Thx

[kjbracey]

"Quick" certainly implies limiting it to just modifying the heuristic in this code block.

But given that I'm not sure when any progress is going to be made on wider tool changes, I want to get whatever heuristic is placed here as solid as possible, so we hopefully don't have to mess with it again (and maybe cope without any further tool changes for a prolonged time).

[alzix]

This PR is all about injecting a define to a linker script, right? regardless of SPM implementation this can be useful piece of configuration for the SW developer.
So we have:

• DOMAIN_NS=0 for cases when target.is_PSA_secure_target
• DOMAIN_NS=1 for the opposite cases

Am i missing something? This discussion got too lengthy :)

So i'm suggesting:

self.flags["ld"].append(self.make_ld_define("DOMAIN_NS", "0x0" if target.is_PSA_secure_target else "0x1"))

[kjbracey]

No, DOMAIN_NS is the indicator of which side of the ARM TrustZone boundary we're on, specifically, not the PSA security boundary.

So when using TFM it's 0/unset for secure side and 1 for non-secure side.

If this was PSOC6 PSA, it's 0/unset on both chips.

The DOMAIN_NS option is actually being set for the assembler and compiler elsewhere, based on the core's -NS suffix - this code fragment is just doing the same for the linker. Code is a bit out-of-place.

The PR is really about changing the CMSE options - it's not actually changing DOMAIN_NS behaviour. I'm making a replacement PR myself as it's going to be easier than continuing to discuss changing this one.