I'm pretty happy with the decision.

Klemens Nanni <kn@openbsd.org> 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 <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?
> 
> 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:
>