Re: [PATCH v3 2/2] host/rootfs: Switch to systemd-udevd

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 7221 bytes --]

Every time it's time to review this I feel intimidated, and then I end
up once again being pleasantly surprised by how nice and small it is.

Demi Marie Obenour <demiobenour@gmail.com> writes:

> The dependency on /dev/dri/card0 being present is eliminated, and
> whatever devices the user has are now picked up by the compositor.  New
> dependencies are added to ensure that udev coldplug has finished before
> any non-trivial services are started.  systemd-udev-trigger.service runs
> 'udevadm trigger' and has Before=sysinit.target, so anything that is not
> an early boot service can assume 'udevadm trigger' has run.

Sorry if we already discussed this — I thought I remembered bringing it
up but can't find it now, so maybe I didn't — but I've found in the past
that Weston does need /dev/dri/card0 to exist before it's started, even
with udev, in the release/checks/wayland NixOS test — see commit ab5a00b
("release/checks/wayland: add missing dependency").  Before that, the
test intermittently failed, so to make sure we're not introducing a
similar intermittent fault here, I'd like to understand why that's
different, if it is.  Do NixOS tests start running before coldplug, for
example?  If so, that suggests it might be okay for us to run things
before coldplug too.

> ---
>  host/rootfs/Makefile                               |  36 +++----
>  host/rootfs/default.nix                            | 118 ++++++++-------------
>  host/rootfs/image/etc/mdev.conf                    |   7 --
>  host/rootfs/image/etc/mdev/listen                  |  11 --
>  host/rootfs/image/etc/mdev/wait                    |  14 ---
>  host/rootfs/image/etc/s6-rc/card0/type.license     |   2 -
>  host/rootfs/image/etc/s6-rc/card0/up               |   4 -
>  host/rootfs/image/etc/s6-rc/kvm/timeout-up         |   1 -
>  host/rootfs/image/etc/s6-rc/kvm/timeout-up.license |   2 -
>  host/rootfs/image/etc/s6-rc/kvm/type               |   1 -
>  host/rootfs/image/etc/s6-rc/kvm/type.license       |   2 -
>  host/rootfs/image/etc/s6-rc/kvm/up                 |   4 -
>  host/rootfs/image/etc/s6-rc/mdevd-coldplug/type    |   1 -
>  host/rootfs/image/etc/s6-rc/mdevd-coldplug/up      |   4 -
>  .../image/etc/s6-rc/mdevd/notification-fd.license  |   2 -
>  host/rootfs/image/etc/s6-rc/mdevd/run              |   5 -
>  .../contents.d/systemd-udevd-coldplug}             |   0
>  .../dependencies.d/systemd-udevd}                  |   0
>  .../s6-rc/{card0 => systemd-udevd-coldplug}/type   |   0
>  .../type.license                                   |   0
>  .../image/etc/s6-rc/systemd-udevd-coldplug/up      |   3 +
>  .../kvm => systemd-udevd/flag-essential}           |   0

You said you were going to drop this?

https://spectrum-os.org/lists/archives/spectrum-devel/3bc30fa7-3f40-4373-bb7a-1c9119d8b939@gmail.com

>  .../s6-rc/{mdevd => systemd-udevd}/notification-fd |   0
>  .../s6-rc/systemd-udevd/notification-fd.license    |   2 +
>  host/rootfs/image/etc/s6-rc/systemd-udevd/run      |  13 +++
>  .../image/etc/s6-rc/{mdevd => systemd-udevd}/type  |   0
>  .../s6-rc/{mdevd => systemd-udevd}/type.license    |   0
>  .../contents.d/systemd-udevd-coldplug}             |   0
>  host/rootfs/image/etc/s6-rc/vm-env/type.license    |   2 -

Mistake?

>  .../vmm-env/contents.d/systemd-udevd-coldplug      |   0
>  .../weston/dependencies.d/systemd-udevd-coldplug   |   0
>  .../image/etc/udev/rules.d/99-spectrum.rules       |  19 ++++
>  host/rootfs/image/usr/bin/run-vmm                  |   3 +
>  host/rootfs/image/usr/bin/systemd-udevd            |   1 +
>  .../{etc/mdev/net/add => usr/libexec/net-add}      |   0
>  35 files changed, 100 insertions(+), 157 deletions(-)
>
> diff --git a/host/rootfs/image/etc/s6-rc/systemd-udevd/run b/host/rootfs/image/etc/s6-rc/systemd-udevd/run
> new file mode 100644
> index 0000000000000000000000000000000000000000..8d0fd046e2d38aef9c0010fa40aa7b9a57c76373
> --- /dev/null
> +++ b/host/rootfs/image/etc/s6-rc/systemd-udevd/run
> @@ -0,0 +1,13 @@
> +#!/bin/execlineb -P
> +# SPDX-License-Identifier: EUPL-1.2+
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +s6-setlock -d 4 /run/sd-notify-wrapper/systemd-udevd.lock

Why do we need to specify a descriptor here?

> +if { rm -f /run/sd-notify-wrapper/systemd-udevd.sock }

"Note that path will be deleted if it already exists at program start time."

https://skarnet.org/software/s6/s6-ipcserver-socketbinder.html

> +background {
> +   s6-ipcserver-socketbinder -b0 -m -a 0600 /run/sd-notify-wrapper/systemd-udevd.sock
> +   fdmove 1 3
> +   sd-notify-adapter
> +}
> +fdclose 3
> +export NOTIFY_SOCKET /run/sd-notify-wrapper/systemd-udevd.sock
> +systemd-udevd

> diff --git a/host/rootfs/image/etc/udev/rules.d/99-spectrum.rules b/host/rootfs/image/etc/udev/rules.d/99-spectrum.rules
> new file mode 100644
> index 0000000000000000000000000000000000000000..f9047d5dabe493f21b5f27d6ef7d44b31f9d741b
> --- /dev/null
> +++ b/host/rootfs/image/etc/udev/rules.d/99-spectrum.rules
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: EUPL-1.2+
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +# systemd-udevd has a built-in module loading feature,
> +# but it seems to not work for some reason or another.
> +# This works.
> +ACTION!="remove", ENV{MODALIAS}!="", RUN+="/usr/bin/modprobe -q $env{MODALIAS}"
> +# systemd-udevd unsets PATH, so fix that.
> +ACTION=="remove", GOTO="spectrum-end"
> +ENV{PCI_CLASS}!="2????", GOTO="spectrum-end"
> +
> +# net-add unbinds and rebinds the driver, so avoid calling
> +# it more than once.
> +IMPORT{db}="SPECTRUM_DRIVER_ASSIGNED"
> +ENV{SPECTRUM_DRIVER_ASSIGNED}=="yes", GOTO="spectrum-end"
> +# systemd-udevd unsets PATH, so fix that.
> +RUN+="/usr/bin/env PATH=/usr/bin /usr/libexec/net-add"
> +ENV{SPECTRUM_DRIVER_ASSIGNED}="yes"
> +LABEL="spectrum-end"

I'd rather this was protected against in net-add, in the way I outlined
before, because that would also protect against it being run from
elsewhere.

> diff --git a/host/rootfs/image/usr/bin/run-vmm b/host/rootfs/image/usr/bin/run-vmm
> index bcb6cdaf6646da6bb4970fe97f5ef03badbd66a6..00d8b0ee75311855f0b2c0686b66616c747b68c0 100755
> --- a/host/rootfs/image/usr/bin/run-vmm
> +++ b/host/rootfs/image/usr/bin/run-vmm
> @@ -53,4 +53,7 @@ unexport !
>  fdmove -c 3 0
>  redirfd -r 0 /dev/null
>  
> +# Do this last so that udev has as much
> +# time to set up KVM as possible.
> +if { udevadm wait /dev/kvm }

Doesn't need the comment IMO.  Lots of this stuff is deliberately
ordered already.

>  cloud-hypervisor --api-socket fd=3
> diff --git a/host/rootfs/image/usr/bin/systemd-udevd b/host/rootfs/image/usr/bin/systemd-udevd
> new file mode 120000
> index 0000000000000000000000000000000000000000..b7887eaf6dd3279116ba61d613bd467598089597
> --- /dev/null
> +++ b/host/rootfs/image/usr/bin/systemd-udevd
> @@ -0,0 +1 @@
> +udevadm
> \ No newline at end of file

Do we want to just put it in usrPackages, so we get both
/usr/bin/udevadm and /usr/lib/systemd/systemd-udevd without having to do
anything else?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]