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-bb7...
.../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?