Re: [meta-arm] [PATCH v3 1/2] optee-client: use udev rule and systemd service from upstream
Gyorgy Szing <[email protected]>
| Newsgroups | org.yoctoproject.lists.meta-arm |
|---|---|
| Message-ID | <AS8PR08MB58955003C2B8CF01CDE7EA54912F2@AS8PR08MB5895.eurprd08.prod.outlook.com> |
> Hi,
>
> On Wed, Oct 30, 2024 at 08:02:19AM +0000, Gyorgy Szing wrote:
> > Hi,
> >
> > (Sorry for the delayed reply.)
> >
> > Another way to address this would be to use PkgConfig from CMakeLists.txt (see [1]). This might be considered a nicer solution as the yocto recipe would not have to correct the install location, and thus there would be a single source of truth. (Kudos for Ross for the suggestion.)
> > Something like (code is not tested):
> >
> > Include(FindPkgConfig)
> > If (PKG_CONFIG_FOUND)
> > # TODO: there are other variables. Doublecheck if systemdsystemconfdir is the right one to be used. On X86 Ubuntu this returns /etc/systemd/system. Yocto might define distro specific values.
> > pkg_get_variable(UNIT_DIR systemd systemdsystemconfdir)
> > # TODO: check the value to see if pkgconfig successfully found the value. Behavior is not documented, probably if (UNIT_DIR) would work.
> > # TODO: UNIT_DIR is a string list, use only the first or last value? Rewrite the line below accordingly
> > set(CMAKE_INSTALL_SYSTEMDSYSCONFDIR “${ UNIT_DIR }” CACHE PATH “Target directory for system config files”)
> > endif()
> > # By default, use LIBDIR
> > # TODO: CMAKE_INSTALL_SYSCONFDIR might be a better default base dir.
> > set(CMAKE_INSTALL_ SYSTEMDSYSCONFDIR “${ CMAKE_INSTALL_LIBDIR}}/systemd/system” CACHE PATH “Target directory for system config files”)
> >
> > install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/[email protected] DESTINATION ${ CMAKE_INSTALL_SYSTEMDSYSCONFDIR})
> >
> >
> > The above code would default to the current install location under LIBDIR, and would use PkgConfig if available. Setting CMAKE_INSTALL_ SYSTEMDSYSCONFDIR from the command line allows manual override.
> >
> > 1: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html
>
> This would only work if optee-client has "systemd" build time dependency in DEPENDS.
> I don't think we should add this dependency. The sd_notify() support for example
> dlopen()'s systemd libs at runtime to avoid making systemd a hard build time
> dependency. I don't think we should add this dependency due to systemd
> service file install paths.
DISTRO_FEATURES seem to reflect if systemd is deployed and this allows the recipe to behave accordingly. Not sure if this is the case for all distros, but poky and meta-arm already depend on DISTRO_FEATURES based systemd configuration and thus might be an acceptable risk.
AFAIK systemd was added part of uefi-secure boot enablement. If this feature is enabled the native sysroot of the optee-client recipe has systemd already because of some uefi-secureboot specific transitive dependency. Might not be bulletproof but if systemd is needed, things seem to be ok as is. On the other hand, adding systemd conditionally to DEPENDS might not make things much worse since it is included already.
Please check the attached patch file. I cherry-picked you commit “optee-client: use udev rule and systemd service from upstream“ to my topic branch and added the attached patch on top (among other patches which are hopefully have no effect on optee-client).
I did some manual testing by hacking DISTRO_FEATURES and running bitbake-getvar, plus testing fvp_base and quemuarm64-secureboot configurations. It’s not 100% tested as e.g. I did not check how the cmake modification behaves if pkg-config is enabled, but the executable is not available or is available but either system or udev package is missing. The cmake code is there to handle these cases though. Anyways these scenarios seem to be out of scope for yocto.
I added a dependency on udev to the optee-client recipe, to make udevs pkg config data available. This might be considered similar “unwanted bloat” you mentioned related to systemd. I tried to make this conditional but could not find a safe way. According to [1] there is a variable called USE_DEVFS which tells if a device manager is available, but the variable is not visible to the recipe. AFAIK the image class would need to be inherited to get access to it, but that does not seem to be a good idea.
The op-tee client patch:
- adds build options to control if the system service file shall be installed or not. The default value is on, to keep backwards compatibility.
- another option is added to enable using pkg-config. This is by default off, and the op-tee client recipe enables it for yocto. If enabled both system and/or udev specific directories are discovered with pkg-config.
- the intention was to implement option and pkg-config processing in a flexible way. There are hard-coded default values which keep existing behavior and will be used if pkg-config is disabled or failing.
Note: I missed the systemd dependency issue and conditional appending to DEPENDS is not implemented.
1: https://docs.yoctoproject.org/dev/dev-manual/device-manager.html
/George
> Cheers,
>
> -Mikko
>
> > /George
> >
> > On 2024-10-29, 18:03, "[email protected]" <[email protected]> wrote:
> >
> > On 10/23/2024 9:54 AM, Mikko Rapeli wrote:
> > > Ok got it now.
> > >
> > > ${nonarch_base_libdir} isn't in the CMake toolchain file. We could
> > > move ${libdir}/systemd unconditionlly to ${nonarch_base_libdir}/systemd
> > > in the do_install task. Could you test and submit a patch like below?
> > >
> > > --- a/meta-arm/recipes-security/optee/optee-client.inc
> > > +++ b/meta-arm/recipes-security/optee/optee-client.inc
> > > @@ -23,8 +23,9 @@ EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0"
> > >
> > > do_install:append() {
> > > # installed by default
> > > + mv ${D}${libdir}/systemd ${D}${noarch_base_libdir}/systemd
> > > if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then
> > > - rm -rf ${D}${libdir}/systemd
> > > + rm -rf ${D}${noarch_base_libdir}/systemd
> > > fi
> > > if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then
> > > install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant
> > >
> > > Cheers,
> > >
> > > -Mikko
> >
> > This works fine in our testing. Sorry for the late response.
> >
> > Thanks again!
> >
> > Tom
>
0001-optee-client-fix-udev-and-systemd-handling.patch
(application/octet-stream, 7.1 KB)
From 9509a6e37f17db10d131cf4c333487106d6f9b8d Mon Sep 17 00:00:00 2001 From: Gyorgy Szing <[email protected]> Date: Mon, 28 Oct 2024 23:48:21 +0100 Subject: [PATCH 1/1] optee-client: fix udev and systemd handling Eliminate the systemd specific install content fix-up commands appended to do_install. For this patch tee-suplicant to use pkg-config for discovering udev and systemd specific install locations. Signed-off-by: Gyorgy Szing <[email protected]> --- .../recipes-security/optee/optee-client.inc | 11 +-- ...pdate-udev-rule-and-systemd-service-.patch | 71 +++++++++++++++++++ .../optee/optee-client_4.3.0.bb | 3 +- .../optee/optee-client_4.4.0.bb | 1 + 4 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-update-udev-rule-and-systemd-service-.patch diff --git a/meta-arm/recipes-security/optee/optee-client.inc b/meta-arm/recipes-security/optee/optee-client.inc index fc48c302..0f8665ad 100644 --- a/meta-arm/recipes-security/optee/optee-client.inc +++ b/meta-arm/recipes-security/optee/optee-client.inc @@ -16,16 +16,19 @@ UPSTREAM_CHECK_GITTAGREGEX = "^(?P<pver>\d+(\.\d+)+)$" S = "${WORKDIR}/git" +DEPENDS += "udev" + EXTRA_OECMAKE = " \ -DBUILD_SHARED_LIBS=ON \ + -DCFG_USE_PKGCONFIG=On \ " + +EXTRA_OECMAKE += "${@bb.utils.contains('DISTRO_FEATURES', 'systemd', \ + '-DCFG_ENABLE_SYSTEMD=On', ' -DCFG_ENABLE_SYSTEMD=Off' , d)}" + EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0" do_install:append() { - # installed by default - if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then - rm -rf ${D}${libdir}/systemd - fi if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant sed -i -e s:@sysconfdir@:${sysconfdir}:g \ diff --git a/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-update-udev-rule-and-systemd-service-.patch b/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-update-udev-rule-and-systemd-service-.patch new file mode 100644 index 00000000..9e8099bf --- /dev/null +++ b/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-update-udev-rule-and-systemd-service-.patch @@ -0,0 +1,71 @@ +From ed7d18ae174f648db5ccdc20f07aa86ebf1410ce Mon Sep 17 00:00:00 2001 +From: Gyorgy Szing <[email protected]> +Date: Thu, 14 Nov 2024 23:08:35 +0100 +Subject: [PATCH 1/1] tee-supplicant: update udev rule and systemd service file + handling + +Allow optionally using pkg-config to discover install location of +systemd service and udev rule files. +Make systemd service file generation and installation optional. + +Upstream-Status: Pending + +Signed-off-by: Gyorgy Szing <[email protected]> +--- + tee-supplicant/CMakeLists.txt | 34 +++++++++++++++++++++++++++++++--- + 1 file changed, 31 insertions(+), 3 deletions(-) + +diff --git a/tee-supplicant/CMakeLists.txt b/tee-supplicant/CMakeLists.txt +index 8df9bef..a877171 100644 +--- a/tee-supplicant/CMakeLists.txt ++++ b/tee-supplicant/CMakeLists.txt +@@ -7,6 +7,8 @@ option(RPMB_EMU "Enable tee-supplicant to emulate RPMB" ON) + option(CFG_TA_GPROF_SUPPORT "Enable tee-supplicant support for TAs instrumented with gprof" ON) + option(CFG_FTRACE_SUPPORT "Enable tee-supplicant support for TAs instrumented with ftrace" ON) + option(CFG_TEE_SUPP_PLUGINS "Enable tee-supplicant plugin support" ON) ++option(CFG_ENABLE_SYSTEMD "Enable systemd service unit file generation." ON) ++option(CFG_USE_PKGCONFIG "Use pkg-config for discovering install target directory for systemd and udev files." OFF) + + set(CFG_TEE_SUPP_LOG_LEVEL "1" CACHE STRING "tee-supplicant log level") + # FIXME: Question is, is this really needed? Should just use defaults from # GNUInstallDirs? +@@ -117,8 +119,34 @@ endif() + ################################################################################ + # Install targets + ################################################################################ ++# Discover target install location of the systemd and udev files using pkg-config ++if (CFG_USE_PKGCONFIG) ++ # Note: pkg-config should return setting valid for the target platform and not the host. ++ include(FindPkgConfig) ++ if (PKG_CONFIG_FOUND) ++ pkg_search_module(SYSTEMD systemd) ++ if (SYSTEMD_FOUND AND CFG_ENABLE_SYSTEMD) ++ pkg_get_variable(UNIT_DIR systemd systemd_system_unit_dir) ++ set(SYSTEMD_UNIT_DIR "${UNIT_DIR}" CACHE PATH "Location of systemd unit files.") ++ unset(UNIT_DIR) ++ endif() ++ pkg_search_module(UDEV udev) ++ if (UDEV_FOUND) ++ pkg_get_variable(UDEV_DIR udev udev_dir) ++ set(UDEV_UDEV_DIR "${UDEV_DIR}" CACHE PATH "Location of udev files.") ++ unset(UDEV_DIR) ++ endif() ++ endif() ++endif() ++ ++# Some sane defaults is discovering through pkgconfig fails or is disabled. ++set(SYSTEMD_UNIT_DIR "${CMAKE_INSTALL_LIBDIR}/systemd/system" CACHE PATH "Location of systemd unit files.") ++set(UDEV_UDEV_DIR "${CMAKE_INSTALL_SYSCONFDIR}/udev" CACHE PATH "Location of udev files.") ++ + install(TARGETS ${PROJECT_NAME} RUNTIME DESTINATION ${CMAKE_INSTALL_SBINDIR}) +-configure_file([email protected] [email protected] @ONLY) +-install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/[email protected] DESTINATION ${CMAKE_INSTALL_LIBDIR}/systemd/system) ++if (CFG_ENABLE_SYSTEMD) ++ configure_file([email protected] [email protected] @ONLY) ++ install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/[email protected] DESTINATION ${SYSTEMD_UNIT_DIR}) ++endif() + configure_file(optee-udev.rules.in optee-udev.rules @ONLY) +-install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/optee-udev.rules DESTINATION ${CMAKE_INSTALL_SYSCONFDIR}/udev/rules.d) ++install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/optee-udev.rules DESTINATION ${UDEV_UDEV_DIR}/rules.d) +-- +2.43.0 + diff --git a/meta-arm/recipes-security/optee/optee-client_4.3.0.bb b/meta-arm/recipes-security/optee/optee-client_4.3.0.bb index edab4583..9dc301f4 100644 --- a/meta-arm/recipes-security/optee/optee-client_4.3.0.bb +++ b/meta-arm/recipes-security/optee/optee-client_4.3.0.bb @@ -2,7 +2,8 @@ require recipes-security/optee/optee-client.inc SRCREV = "a5b1ffcd26e328af0bbf18ab448a38ecd558e05c" -SRC_URI += "file://0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch" +SRC_URI += "file://0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch \ + file://0001-tee-supplicant-update-udev-rule-and-systemd-service-.patch" inherit pkgconfig DEPENDS += "util-linux" diff --git a/meta-arm/recipes-security/optee/optee-client_4.4.0.bb b/meta-arm/recipes-security/optee/optee-client_4.4.0.bb index fa7b9564..fa649143 100644 --- a/meta-arm/recipes-security/optee/optee-client_4.4.0.bb +++ b/meta-arm/recipes-security/optee/optee-client_4.4.0.bb @@ -2,6 +2,7 @@ require recipes-security/optee/optee-client.inc # v4.4.0 SRCREV = "d221676a58b305bddbf97db00395205b3038de8e" +SRC_URI += "file://0001-tee-supplicant-update-udev-rule-and-systemd-service-.patch" inherit pkgconfig DEPENDS += "util-linux" -- 2.43.0