TFM: Remove `TFM_MCUBOOT_PATH_DOWNLOAD` Kconfig which downloads other things? In builds

[nordicjm]

Describe the bug

FAO @ceolin
It's been mentioned before that this option should be removed, strangely this option still seems present? The build system should not be letting a random module download random things for a build which firstly doesn't support offline building, secondly isn't related in any way to the west manifest (which should be used) and thirdly has absolutely no guarantee it isn't spoofed or manipulated.

If a user wants to use a custom MCUboot repo, they can (and always) have been able to do that with a custom west manifest which is the standard zephyr way for getting repositories, TFM should be no exception to this

Regression

• This is a regression.

Steps to reproduce

No response

Relevant log output

Impact

Functional Limitation – Some features not working as expected, but system usable.

Environment

No response

Additional Context

No response

[nordicjm]

@Vge0rge @wearyzen @valeriosetti @tomi-font @ithinuel

[tomi-font]

The Kconfig option is unused? can just be removed

[nordicjm]

The Kconfig option is unused? can just be removed

Ah great, will submit a PR: #98064

[nordicjm]

though actually looking here it seems there are more things that are just downloaded automatically?

config TFM_ETHOS_DRIVER_PATH_LOCAL
        string "Path to a locally available Ethos-U driver or an empty string"
        depends on SOC_SERIES_MPS3 || SOC_SERIES_MPS4
        default "$(ZEPHYR_HAL_ETHOS_U_MODULE_DIR)"
        help
          Path to a locally available Ethos-U driver to be used for TF-M builds or
          an empty string to allow TF-M to automatically fetch the Ethos-U
          driver from an external repository at build time.
          By default Zephyr's Ethos-U driver will be used. It is present in
          the hal_ethos_u module.
          Alternatively, applications can point to their own paths for Ethos-U driver.

config TFM_QCBOR_PATH
        string
        prompt "Path to QCBOR or DOWNLOAD to fetch automatically"
        default ""
        help
          Path to QCBOR for TF-M builds. Due to a license issue with this
          library Zephyr does not ship with this library.
          If the application wishes to still use this library they can point
          to their own checkout of this library, or set to DOWNLOAD to allow
          TF-M build system to automatically download this.

[tomi-font]

Yeah, the TFM_QCBOR_PATH one was to solve a license issue, I'm still a bit confused about it (and its usefulness), the other one I don't know.

[wearyzen]

IIRC, TFM_ETHOS_DRIVER_PATH_LOCAL was added to stop TF-M from fetching Ethos-U driver from external repository in Zephyr CI. We cannot append -DETHOS_DRIVER_PATH= unconditionally because it is not applicable to all boards and will result in build failure and so CONFIG_TFM_ETHOS_DRIVER_PATH_LOCAL was introduced. Back then TFM_QCBOR_PATH was taken as reference to add this config and so its a string but revisiting this now, I think it should have just been a simple yes/no config to indicate presence of Ethos-U driver to append the ETHOS_DRIVER_PATH.

[tomi-font]

Do we know what boards or so need TFM_ETHOS_DRIVER_PATH_LOCAL? If so we might be able to get away with just using some other (e.g. board) Kconfig options.

[wearyzen]

Do we know what boards or so need TFM_ETHOS_DRIVER_PATH_LOCAL? If so we might be able to get away with just using some other (e.g. board) Kconfig options.

yes, I see its only enabled for SOC_MPS3 and SOC_MPS4 so we can remove TFM_ETHOS_DRIVER_PATH_LOCAL and use these SOC configs to add the ETHOS_DRIVER_PATH.

[nordicjm]

IIRC, TFM_ETHOS_DRIVER_PATH_LOCAL was added to stop TF-M from fetching Ethos-U driver from external repository in Zephyr CI. We cannot append -DETHOS_DRIVER_PATH= unconditionally because it is not applicable to all boards and will result in build failure and so CONFIG_TFM_ETHOS_DRIVER_PATH_LOCAL was introduced. Back then TFM_QCBOR_PATH was taken as reference to add this config and so its a string but revisiting this now, I think it should have just been a simple yes/no config to indicate presence of Ethos-U driver to append the ETHOS_DRIVER_PATH.

Hmm I don't agree with this, I only just saw this comment now after having made the changes to remove the ethos Kconfig and can confirm with the variable always set, and it being set to empty (because I excluded the module) nrf5340dk/nrf5340/cpuapp/ns builds fine, though you do get this which is a bit annoying:

CMake Warning:
  Manually-specified variables were not used by the project:

    ETHOS_DRIVER_PATH

Do we know what boards or so need TFM_ETHOS_DRIVER_PATH_LOCAL? If so we might be able to get away with just using some other (e.g. board) Kconfig options.

yes, I see its only enabled for SOC_MPS3 and SOC_MPS4 so we can remove TFM_ETHOS_DRIVER_PATH_LOCAL and use these SOC configs to add the ETHOS_DRIVER_PATH.

will update for that

[wearyzen]

IIRC, TFM_ETHOS_DRIVER_PATH_LOCAL was added to stop TF-M from fetching Ethos-U driver from external repository in Zephyr CI. We cannot append -DETHOS_DRIVER_PATH= unconditionally because it is not applicable to all boards and will result in build failure and so CONFIG_TFM_ETHOS_DRIVER_PATH_LOCAL was introduced. Back then TFM_QCBOR_PATH was taken as reference to add this config and so its a string but revisiting this now, I think it should have just been a simple yes/no config to indicate presence of Ethos-U driver to append the ETHOS_DRIVER_PATH.

Hmm I don't agree with this, I only just saw this comment now after having made the changes to remove the ethos Kconfig and can confirm with the variable always set, and it being set to empty (because I excluded the module) nrf5340dk/nrf5340/cpuapp/ns builds fine, though you do get this which is a bit annoying:

CMake Warning:
  Manually-specified variables were not used by the project:

    ETHOS_DRIVER_PATH

Right, it doesn't result in build failure. I remembered half of it then 😄.
Going back to the PR it looks like it was because of this cmake warning that I added the MPS3/MPS4 dependency to the config (which was not needed as you guys mentioned above).