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 <stu@spacehopper.org> 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?


(It tests function existence to avoid other arch stubs/diff churn;
 Could also need better names and comments why we do it this way...)

Index: arm64/ramdisk/install.md
===================================================================
RCS file: /cvs/src/distrib/arm64/ramdisk/install.md,v
diff -u -p -r1.49 install.md
--- arm64/ramdisk/install.md	17 Apr 2024 04:36:39 -0000	1.49
+++ arm64/ramdisk/install.md	17 Apr 2024 04:57:51 -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: miniroot/install.sub
===================================================================
RCS file: /cvs/src/distrib/miniroot/install.sub,v
diff -u -p -r1.1263 install.sub
--- miniroot/install.sub	15 Mar 2024 17:31:21 -0000	1.1263
+++ miniroot/install.sub	17 Apr 2024 04:57:52 -0000
@@ -3011,7 +3011,10 @@ 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
+		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 +3587,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: