> From: "Theo de Raadt" <[email protected]> > Date: Tue, 01 Feb 2022 10:48:39 -0700 > > Mark Kettenis <[email protected]> wrote: > > > > From: "Theo de Raadt" <[email protected]> > > > Date: Tue, 01 Feb 2022 09:28:59 -0700 > > > > > > A few comments. > > > > > > I think this approach is increasingly fragile, because a change for one > > > architecture will affect others. > > > > > > > -.elif ${MACHINE} == "armv7" || ${MACHINE} == "arm64" > > > > +.elif ${MACHINE} == "armv7" || ${MACHINE} == "arm64" || ${MACHINE} == > > > > "riscv64" > > > > SRCS += armv7_installboot.c > > > > > > How about copying armv7_installboot.c to a new file, and then it even > > > becomes > > > possible to perform this operation in the C code also: > > > > > > > echo bootriscv64.efi > /mnt/mnt/efi/boot/startup.nsh > > > > > > As a bonus, it will umount the disk... which the current shell function > > > forgets > > > to do. > > > > > > I think a new file is better, if this addition is made, rather than > > > hiding it > > > behind a #ifdef inside a .if file > > > > I disagree. Apart from the name of the bootloader file, the code is > > identical and this is by design! > > > > Maybe we should rename the file to efi_installboot.c and/or rearrange > > the code slightly such that is becomes more obvious that the name of > > the file is indeed the only difference. > > The code cannot be the same, because installboot is supposed to do the > *COMPLETE OPERATION* of making a disk bootable. > > But with the proposal, installboot(8) fails to do the complete job, and > the md_installboot() shell function has to still do: > > echo bootriscv64.efi > /mnt/mnt/efi/boot/startup.nsh > > But look closer. md_intallboot does a mount also. > > But if this isn't changed, the actual sequence really becomes: > > newfs -t msdos > mount > copy file > unmount > mount > echo bootriscv64.efi > /mnt/mnt/efi/boot/startup.nsh > unmount > > Right now, the operation is: > > newfs -t msdos (elsewhere) > mount > copy file > echo bootriscv64.efi > /mnt/mnt/efi/boot/startup.nsh > unmount # it actually forgets to do this > > Really, that is what the operations in md_installboot() script expands to. > Right now, it mounts once. With the proposal, it mounts the filesystem twice. > That is pretty ridiculous. > > The way that gets solved is to put the startup.nsh file creation directly into > installboot(8). Into armv7_installboot.c. But obviously hiding behind an > #ifdef __riscv64__ or something? Gross isn't it.
No it should be done the same way on arm64 and armv7 as well. The startup.nsh file is needed on (some) arm64 machines as well and would be needed on armv7 machines that don't use U-Boot too... > So on the balance, copying the code is a better plan. ...so I disagree. > Most of the *_installboot.c files already have a huge amount of duplication, > mixed with little changes which we don't hide behind #ifdef. Hey, we could > merge all of *_installboot.c into one file, with a pile of #ifdef. BUT > that is what we used to have, and it was difficult to maintain correctness. > > The new instalboot(8) design is such that you run the command and it does > EVERYTHING. Visa's proposal is pointless if the filesystem still has to be > mounted a 2nd time to create a file, because of some purist attitude that > this code should be shared. I agree this is stupid, but that really was just lazyness on my side. The code to create startup.nsh should have been added when I converted the arm64 installer to use installboot(8) in the same way as visa@ is now doing for riscv64. So yes, it would be good if visa@ could add the startup.sh creation code to installboot for arm64, armv7 and riscv64.
