From: "Theo de Raadt" Subject: Re: installer: defer installboot to pick up apple-boot firmware To: tech@openbsd.org, Tobias Heider , Mark Kettenis Date: Mon, 06 May 2024 13:25:35 -0600 I'm pretty happy with the decision. Klemens Nanni wrote: > On Wed, Apr 17, 2024 at 05:14:56AM GMT, Klemens Nanni wrote: > > On Tue, Apr 09, 2024 at 08:19:13PM +0000, Klemens Nanni wrote: > > > On Tue, Apr 09, 2024 at 01:11:55PM -0600, Theo de Raadt wrote: > > > > Stuart Henderson wrote: > > > > > > > > > On 2024/04/09 14:52, Klemens Nanni wrote: > > > > > > On Sun, Apr 07, 2024 at 01:17:16PM -0600, Theo de Raadt wrote: > > > > > > > > If fw_update hangs are a real problem, they should be fixed; this isn't > > > > > > > > the only place we run fw_update. > > > > > > > > > > > > > > fw_update is the first time we actually depend upon internet working. > > > > > > > I've seen it not work, and I've seen it slap against a proxy for a while > > > > > > > thinking it will work. Regardless, let's say it times out. (Meaning, > > > > > > > we must make sure it gives up quickly, and nicely. I think afresh1 designed > > > > > > > that into the new one). > > > > > > > > > > > > > > In that case, do you have the files neccessary to proceed? > > > > > > > > > > > > With install? Sure, installboot works the same, whether fw_update > > > > > > ran or failed before or after. > > > > > > > > > > > > > > > > > > > > If we do, that's OK. > > > > > > > > > > > > > > So it mostly comes down to fw_update must not hangs. If it hangs, people > > > > > > > hit the power button. Now the machine is not bootable. > > > > > > > > > > > > If we do fw_update before installboot (my diff) and people force-reboot > > > > > > their machine, then no, fresh^Wpartial installs won't boot as boot blocks > > > > > > haven't been installed, but what do you expect of manually aborted installs? > > > > > > > > > > > > Upgrades abrupted this way ought to still boot from their root disk, > > > > > > unless there's a bootloader/kernel compat break or filesystem damage from > > > > > > resetting the box or whatever... > > > > > > > > > > Remember that machines booting in this way maybe remote installs > > > > > or repairs where someone has manually put a USB stick in to a machine > > > > > somewhere else in the world and set a temporary boot device as a > > > > > one-off. > > > > > > > > > > It can be quite helpful in those cases if the installer can try to > > > > > leave the machine bootable as early as possible (and definitely if > > > > > network is broken; you might be trying to mend a machine which is > > > > > providing the network and things may be in a state where connections > > > > > hang rather than terminate quickly). > > > > > > > > Yes, I agree strongly. > > > > > > > > Trying to be too clever can hurt us. On a majority or our machines, > > > > installboot is independent of fw_update. We should not allow a fw_update > > > > issue (like any sort of freeze) to result in such machines being broken. > > > > > > Sure, then we keep trying our best to leave users with a usable install. > > > > > > Thanks for the input. > > > > > > > Do not allow the obscure needs of 1 architecture to break other > > > > architectures. The previous fw_update positioning in the script was > > > > PERFECT, meaning, we landed it in that sequence more than a decade ago. > > > > > > > > So there's two choices. > > > > > > > > Either we work harder to make sure the booting firwares are present > > > > ahead of time, via other data flows, and skip this change. > > > > or > > > > > > > > We installboot twice. > > > > > > Let's do that now and not stop improving on it. > > > > > > I'll look at how to best do the second run and send a diff soon. > > > > This reruns just 'installboot -r /mnt $ROOTDISK' only on arm64 apple via > > new md_fw() reusing md_installboot(). > > > > Straight forward, but I have yet to test a fresh ramdisk install. > > Thoughts? > > Works as advertised. > > > (It tests function existence to avoid other arch stubs/diff churn; > > Could also need better names and comments why we do it this way...) > > I added a brief comment, otherwise same diff. > > Feedback? OK? > > Index: distrib/arm64/ramdisk/install.md > =================================================================== > RCS file: /cvs/src/distrib/arm64/ramdisk/install.md,v > diff -u -p -r1.49 install.md > --- distrib/arm64/ramdisk/install.md 17 Apr 2024 04:36:39 -0000 1.49 > +++ distrib/arm64/ramdisk/install.md 17 Apr 2024 04:57:27 -0000 > @@ -39,10 +39,11 @@ MOUNT_ARGS_msdos="-o-l" > KEEP_EFI_SYS=false > > md_installboot() { > - local _disk=$1 _chunks _bootdisk _mdec _plat > + local _disk=$1 _reason=$2 _rerun=false _chunks _bootdisk _mdec _plat > > case ${COMPATIBLE} in > - apple,*) _plat=apple;; > + apple,*) _plat=apple > + [[ $_reason == apple-boot ]] && _rerun=true;; > raspberrypi,*) _plat=rpi;; > esac > > @@ -52,6 +53,8 @@ md_installboot() { > exit > fi > > + $_rerun && return > + > # Apply some final tweaks on selected platforms > _mdec=/usr/mdec/$_plat > > @@ -81,6 +84,10 @@ md_installboot() { > umount /mnt/mnt > ;; > esac > +} > + > +md_fw() { > + md_installboot "$@" > } > > md_prep_fdisk() { > Index: distrib/miniroot/install.sub > =================================================================== > RCS file: /cvs/src/distrib/miniroot/install.sub,v > diff -u -p -r1.1263 install.sub > --- distrib/miniroot/install.sub 15 Mar 2024 17:31:21 -0000 1.1263 > +++ distrib/miniroot/install.sub 6 May 2024 18:46:17 -0000 > @@ -3011,7 +3011,11 @@ if ((${#_KERNV[*]} == 1)); then > fi > __EOT > > - [ -x /mnt/usr/sbin/fw_update ] && DESTDIR=/mnt /mnt/usr/sbin/fw_update > + if [[ -x /mnt/usr/sbin/fw_update ]]; then > + DESTDIR=/mnt /mnt/usr/sbin/fw_update > + # Rerun installboot(8) to pick up just fetched boot firmware. > + typeset -f md_fw >/dev/null && md_fw $ROOTDISK apple-boot > + fi > > if [[ -f $_kernel_dir.tgz ]]; then > echo -n "Relinking to create unique kernel..." > @@ -3584,6 +3588,7 @@ umount -af >/dev/null 2>&1 > # md_consoleinfo() - set CDEV, CTTY, CSPEED, CPROM > # > # The following functions can be provided if required: > +# md_fw() - device specific firmware quirks > # md_prep_fdisk() - put a partition table on the disk > # > # The following variables can be provided if required: >