Re: [meta-arm] [PATCH v3 1/2] optee-client: use udev rule and systemd service from upstream

[Mikko Rapeli <mikko.rapeli@linaro.org>]

Hi,

On Tue, Nov 26, 2024 at 09:05:15PM +0000, Gyorgy Szing wrote:
> > 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}/tee-supplicant@.service 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.

Even when "systemd" is the selected init and in DISTRO_FEATURES,
it is not a build dependency and in DEPENDS of optee-client and thus
systemd pkg-config files are not available.

Yocto recipes and open source SW which ships systemd service files
and even notify systemd at startup using sd_notify() are not meant
to have systemd in their build dependencies. This makes too
many things depend on systemd which results in longer build
times.

Thus I don't want to add systemd to DEPENDS of optee-client.
 
> 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 think this patch is complex enough that it needs upstreaming
to optee-client. I don't agree with using pkg-config settings
from systemd to figure out systemd service file install paths
because that requires adding systemd to build dependencies.

> 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.

As with systemd, I don't think udev should be in build time
dependencies of recipes which just ship udev rules.
In the same way, udev is not in the build dependencies by default
and I don't think it's a good idea to add it there.

> 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

Since we disagree on the solution, I'll stop proposing these changes.

It's up to maintainer of optee-client recipe, Jon and Ross,
to decide what to do with newer optee-client releases which do install
a default udev rule and systemd service which use different users and
groups than the meta-arm side optee-client recipe.

I don't think shipping two optee udev rules is good either and thus would
have kept that rule only in optee-client, possibly a separate binary
package.

Cheers,

-Mikko

> /George
> 
> > Cheers,
> >
> > -Mikko
> >
> > > /George
> > >
> > > On 2024-10-29, 18:03, "meta-arm@lists.yoctoproject.org" <meta-arm@lists.yoctoproject.org> 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
> >
>