From ef809e567707359ed133d2573b01bf66d9e68fe5 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Tue, 14 Jan 2025 15:45:20 -0800 Subject: [PATCH 1/7] Add new scripts/README; explain logging subtleties It was not obvious what gets logged where and when, document that. Signed-off-by: Marc Herbert --- scripts/README.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 scripts/README.md diff --git a/scripts/README.md b/scripts/README.md new file mode 100644 index 0000000..b6d4003 --- /dev/null +++ b/scripts/README.md @@ -0,0 +1,21 @@ +Logs and autorun +---------------- + +Some of these scripts are installed in the image and `run_qemu.sh` can +configure systemd to autorun them at boot time. + +By default, systemctl logs the stdout and stderr (including plain set +-x) of what it starts at the journalctl -p priority level 6 +(info). These are NOT in the "kernel" logging facility hence not on QEMU +serial console and not visible outside the VM. You must ssh to see these +logs. + +On the other hand, commands inside these scripts frequently report +status by printing to `/dev/kmsg` which is in the "kernel" facility and +does exit the VM through QEMU's serial port. + +https://www.kernel.org/doc/Documentation/ABI/testing/dev-kmsg + +When the printed line has no , "echo foo > /dev/kmsg" is logged +at the "default kernel priority". On Fedora, this is priority level 4 +(warn). From 9e324e5dd48667e321100f4cb129f594ab5e128d Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Thu, 16 Jan 2025 15:26:14 -0800 Subject: [PATCH 2/7] run_qemu.sh: extract new function generatedfrom_header() This will be re-used by systemd fixes Signed-off-by: Marc Herbert --- run_qemu.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/run_qemu.sh b/run_qemu.sh index d0312e9..cf5b2a4 100755 --- a/run_qemu.sh +++ b/run_qemu.sh @@ -1008,12 +1008,18 @@ setup_gcp_tweaks() chmod go-rw mkosi.extra/etc/ssh/sshd_config } +# "... generated by ... from $1 ..." +generatedfrom_header() +{ + printf '\n### Generated by %s,\n### from %s,\n### on %s\n### for mkosi version %s\n\n' \ + "$0" "$1" "$(date)" "$( "$mkosi_bin" --version )" +} + # $1 -> stdout process_mkosi_template() { local src="$1" - printf '\n### Generated by %s,\n### from %s,\n### on %s\n### for mkosi version %s\n\n' \ - "$0" "$src" "$(date)" "$( "$mkosi_bin" --version )" + generatedfrom_header "$src" sed \ -e "s:@OS_DISTRIBUTION_DEF@:${distribution_def}:" \ From 66fd06185b3da6e32831592b6052eeccae92bcf8 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Thu, 16 Jan 2025 15:29:31 -0800 Subject: [PATCH 3/7] run_qemu.sh: use presets to enable basic OS services Stop hacking symbolic links and use .preset files to make sure ssh, systemd-networkd and systemd-resolved are enabled by default. Use .preset files according to `man systemd.preset` and other official systemd documentation. Generally speaking, symbolic links are created and deleted by systemctl commands based on configuration files. So it's not clear whether the previous symbolic link creation really worked and when (most services were likely already enabled anyway in most configurations). Signed-off-by: Marc Herbert --- run_qemu.sh | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/run_qemu.sh b/run_qemu.sh index cf5b2a4..bd446b1 100755 --- a/run_qemu.sh +++ b/run_qemu.sh @@ -939,18 +939,32 @@ update_existing_rootfs() fi } -enable_systemd_service() +systemd_preset() { - servicename="$1" + state="$1" + servicename="$2" if [ ! -d "mkosi.extra" ]; then fail "couldn't find mkosi.extra, are we in an unexpected CWD?" fi - mkdir -p mkosi.extra/etc/systemd/system/multi-user.target.wants + local preset_dir=mkosi.extra/etc/systemd/system-preset/ + mkdir -p "$preset_dir" - ln -sf "/usr/lib/systemd/system/${servicename}.service" \ - "mkosi.extra/etc/systemd/system/multi-user.target.wants/${servicename}.service" + { + generatedfrom_header "preset_service $servicename" + cat < "$preset_dir/04-run_qemu-$servicename.preset" } setup_network() @@ -1134,11 +1148,11 @@ make_rootfs() fi # enable ssh - enable_systemd_service sshd + systemd_preset enable sshd.service # These are needed for Arch only, but didn't seem to have any adverse effect on Fedora - enable_systemd_service systemd-networkd - enable_systemd_service systemd-resolved + systemd_preset enable systemd-networkd.service + systemd_preset enable systemd-resolved.service setup_network # this is effectively 'daxctl migrate-device-model' From 88c1f956ddd9f92ab35e4a373b4f90b57523d7d4 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Thu, 16 Jan 2025 15:37:07 -0800 Subject: [PATCH 4/7] run_qemu.sh: use .preset instead of symlinks to fix --autorun option - Stop messing with symbolic links and use systemd preset instead. Since Fedora 37, `systemctl preset-all` is run on the first boot (which is every boot unless run_qemu.sh --rw is specified). https://fedoraproject.org/wiki/Changes/Preset_All_Systemd_Units_on_First_Boot This destroys and reconfigures all symbolic links based on .preset files. So, switch to .preset files. Other recent distributions have most likely adopted the same method, see `man systemd-firstboot` etc. Even for the distributions that have not, mkosi v15 commit 84ec58cca8e3c also runs `systemctl preset-all` at build time anyway - whichever the distribution is. - Also drop "AllowIsolate" which is discouraged probably never made sense. - Simplify and fix the target dependency graph Signed-off-by: Marc Herbert --- run_qemu.sh | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/run_qemu.sh b/run_qemu.sh index bd446b1..fc54ed6 100755 --- a/run_qemu.sh +++ b/run_qemu.sh @@ -555,7 +555,6 @@ setup_autorun() local bin_dir="/usr/local/bin" local systemd_dir="/etc/systemd/system/" local systemd_unit="$systemd_dir/rq_autorun.service" - local systemd_linkdir="$systemd_dir/rq-custom.target.wants" if [[ ! $_arg_autorun ]]; then autorun_file="$prefix/$systemd_unit" @@ -565,16 +564,20 @@ setup_autorun() mkdir -p "$prefix/$bin_dir" mkdir -p "$prefix/$systemd_dir" - mkdir -p "$prefix/$systemd_linkdir" + cp -L "$_arg_autorun" "$prefix/$bin_dir" chmod +x "$prefix/$bin_dir/${_arg_autorun##*/}" + + # TODO: is a target really necessary? cat <<- EOF > "$prefix/$systemd_dir/rq-custom.target" [Unit] Description=run_qemu Custom Target - Requires=multi-user.target After=multi-user.target - AllowIsolate=yes + + [Install] + WantedBy=default.target EOF + cat <<- EOF > "$prefix/$systemd_unit" [Unit] Description=run_qemu autorun script @@ -589,10 +592,15 @@ setup_autorun() ExecStart=$bin_dir/${_arg_autorun##*/} [Install] - WantedBy=rq-custom.target + RequiredBy=rq-custom.target + WantedBy=default.target EOF - ln -sfr "$prefix/$systemd_unit" "$prefix/$systemd_linkdir/${systemd_unit##*/}" - ln -sfr "$prefix/$systemd_dir/rq-custom.target" "$prefix/$systemd_dir/default.target" + + # Only when building image, not when updating it. + if [ "$(basename "$prefix")" != 'mnt' ]; then + systemd_preset enable rq-custom.target + systemd_preset enable rq_autorun.service + fi } get_loopdev() @@ -925,7 +933,7 @@ __update_existing_rootfs() fi sudo -E bash $_trace_sh -c "$(declare -f setup_depmod); _arg_nfit_test=$_arg_nfit_test; _arg_cxl_test=$_arg_cxl_test; kver=$kver; setup_depmod $inst_prefix" - sudo -E bash $_trace_sh -c "$(declare -f setup_autorun); _arg_autorun=$_arg_autorun; setup_autorun $inst_prefix" + sudo -E bash -e $_trace_sh -c "$(declare -f setup_autorun); _arg_autorun=$_arg_autorun; setup_autorun $inst_prefix" umount_rootfs 2 } From 9ad89c31f04403c8bb85d0cf72ecb3e67b3c6771 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Tue, 14 Jan 2025 15:40:34 -0800 Subject: [PATCH 5/7] rq_cxl_tests.sh: don't silently timeout when /root/ndctl is missing Fail fast and print an error message instead. There's probably a dependency missing between --options but that's a different question. Signed-off-by: Marc Herbert --- scripts/rq_cxl_tests.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/rq_cxl_tests.sh b/scripts/rq_cxl_tests.sh index 6312407..b88fc89 100755 --- a/scripts/rq_cxl_tests.sh +++ b/scripts/rq_cxl_tests.sh @@ -12,7 +12,10 @@ trap cleanup EXIT sleep 4 echo "======= auto-running $0 ========" > /dev/kmsg -cd /root/ndctl || exit +cd /root/ndctl || { + printf '<0> FATAL: %s: no /root/ndctl directory' "$0" > /dev/kmsg + exit 1 +} rm -rf build meson setup build 2>/dev/kmsg From 03be261b67f138e5b7414a2c5aeedcad357699a8 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Thu, 16 Jan 2025 16:10:07 -0800 Subject: [PATCH 6/7] rq_cxl_tests.sh: remove spurious, trailing 'systemctl poweroff' We already power off in trap EXIT, no need to repeat it. Signed-off-by: Marc Herbert --- scripts/rq_cxl_tests.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/rq_cxl_tests.sh b/scripts/rq_cxl_tests.sh index b88fc89..eb0cbdd 100755 --- a/scripts/rq_cxl_tests.sh +++ b/scripts/rq_cxl_tests.sh @@ -45,4 +45,3 @@ dumpfile /root/ndctl/build/meson-logs/testlog.txt echo "======= meson-test.log ========" > /dev/kmsg dumpfile "$logfile" echo "======= Done $0 ========" > /dev/kmsg -systemctl poweroff From 04cf33c785d2572696ce7b9046b6f2f33b936ce5 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Thu, 16 Jan 2025 18:23:22 -0800 Subject: [PATCH 7/7] rq_cxl_results.sh: stop expecting logs from cxl/core/regs.c According to Dave Jiang, cxl_test skips over the hardware registers. Maybe it did not in 2021 when these lines were added to git? Signed-off-by: Marc Herbert --- scripts/rq_cxl_results.sh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/scripts/rq_cxl_results.sh b/scripts/rq_cxl_results.sh index ede093b..9b020e4 100755 --- a/scripts/rq_cxl_results.sh +++ b/scripts/rq_cxl_results.sh @@ -2,16 +2,14 @@ # SPDX-License-Identifier: CC0-1.0 # Copyright (C) 2021 Intel Corporation. All rights reserved. +# TODO: convert this to a proper Expect script. + logfile="$1" # lines we expect to find in the serial log # if any of these are not found, this is an error find_lines_re=( "auto-running .*rq_cxl_tests.sh" - "Mapped CXL Memory Device resource" - "found Status capability" - "found Mailbox capability" - "found Memory Device capability" "[0-9]+/[0-9]+ ndctl:.*OK.*" "Ok:[ \t]+[0-9]+" "Fail:[ \t]+0"