Re: installboot(8): copy apple-boot to ESP
On Mon, Nov 21, 2022 at 07:01:44PM +0100, Tobias Heider wrote:
> + if (verbose)
This block could use braces.
> + fprintf(stderr, "%s %s to %s\n",
> + (nowrite ? "would copy" : "copying"),
> + src, dst);
> + if (!nowrite)
Here you forgot them but got lucky that it still works out.
Missing {
> + rslt = filecopy(src, dst);
> + if (rslt == -1)
> + goto cleanup;
Missing }
> + }
> + rslt = 0;
> +
> + cleanup:
> + free(src);
> + return rslt;
> }
>
> /*
>
Re: installboot(8): copy apple-boot to ESP
On Mon, Nov 21, 2022 at 07:01:44PM +0100, Tobias Heider wrote:
> On Mon, Nov 21, 2022 at 03:09:25PM +, Klemens Nanni wrote:
> > On Mon, Nov 21, 2022 at 03:42:37PM +0100, Tobias Heider wrote:
> > > Here is a more cleaned up version of the previous diff. I moved all the
> > > firmware logic to a new write_firmware() function. This should be easy
> > > to extend if we decide to ship more firmware this way.
> >
> > This seems more tidy.
> >
> > >
> > > The diff passes regress and manual tests with and without $ESP/m1n1/,
> > > /etc/firmware and /etc/firmware/apple-boot.bin.
> >
> > Reads good, but I haven't compile- or run-tested it.
> >
> > Comments inline.
>
> One more version with your comments addressed.
> better?
One unaddressed nit remains.
OK kn, assuming that you've sufficiently tested both apple and non-apple
use cases alike.
>
> Index: efi_installboot.c
> ===
> RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 efi_installboot.c
> --- efi_installboot.c 6 Nov 2022 12:33:41 - 1.7
> +++ efi_installboot.c 21 Nov 2022 17:58:19 -
> @@ -70,6 +70,7 @@
>
> static int create_filesystem(struct disklabel *, char);
> static void write_filesystem(struct disklabel *, char);
> +static int write_firmware(const char *, const char *);
> static int findgptefisys(int, struct disklabel *);
> static int findmbrfat(int, struct disklabel *);
>
> @@ -308,6 +309,13 @@ write_filesystem(struct disklabel *dl, c
> goto umount;
> }
>
> + dst[mntlen] = '\0';
> + rslt = write_firmware(root, dst);
> + if (rslt == -1) {
> + warnx("unable to write firmware");
> + goto umount;
> + }
> +
> rslt = 0;
This line is still redundant as pointed out earlier.
rslt is zero if write_firmware() worked and -1 if not, in which case
the code jumps to unmount right away... which is the next line, anyway.
With 'rslt = 0;' gone, you can leave the goto to keep the usual idiom in
tact, or you can simply do
dst[mntlen] = '\0';
rslt = write_firmware(root, dst);
if (rslt == -1) {
warnx("unable to write firmware");
unmount:
...
which is what the code will do either way.
>
> umount:
> @@ -325,6 +333,61 @@ rmdir:
>
> if (rslt == -1)
> exit(1);
> +}
> +
> +static int
> +write_firmware(const char *root, const char *mnt)
> +{
> + char dst[PATH_MAX];
> + char fw[PATH_MAX];
> + char *src;
> + struct stat st;
> + int rslt;
> +
> + strlcpy(dst, mnt, sizeof(dst));
> +
> + /* Skip if no /etc/firmware exists */
> + rslt = snprintf(fw, sizeof(fw), "%s/%s", root, "etc/firmware");
> + if (rslt < 0 || rslt >= PATH_MAX) {
> + warnx("unable to build /etc/firmware path");
> + return -1;
> + }
> + if ((stat(fw, ) != 0) || !S_ISDIR(st.st_mode))
> + return 0;
> +
> + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> + src = fileprefix(fw, "/apple-boot.bin");
> + if (src == NULL)
> + return -1;
> + if (access(src, R_OK) == 0) {
> + if (strlcat(dst, "/m1n1", sizeof(dst)) >= sizeof(dst)) {
> + rslt = -1;
> + warnx("unable to build /m1n1 path");
> + goto cleanup;
> + }
> + if ((stat(dst, ) != 0) || !S_ISDIR(st.st_mode)) {
> + rslt = 0;
> + goto cleanup;
> + }
> + if (strlcat(dst, "/boot.bin", sizeof(dst)) >= sizeof(dst)) {
> + rslt = -1;
> + warnx("unable to build /m1n1/boot.bin path");
> + goto cleanup;
> + }
> + if (verbose)
> + fprintf(stderr, "%s %s to %s\n",
> + (nowrite ? "would copy" : "copying"),
> + src, dst);
> + if (!nowrite)
> + rslt = filecopy(src, dst);
> + if (rslt == -1)
> + goto cleanup;
> + }
> + rslt = 0;
> +
> + cleanup:
> + free(src);
> + return rslt;
> }
>
> /*
>
Re: installboot(8): copy apple-boot to ESP
On Mon, Nov 21, 2022 at 03:09:25PM +, Klemens Nanni wrote:
> On Mon, Nov 21, 2022 at 03:42:37PM +0100, Tobias Heider wrote:
> > Here is a more cleaned up version of the previous diff. I moved all the
> > firmware logic to a new write_firmware() function. This should be easy
> > to extend if we decide to ship more firmware this way.
>
> This seems more tidy.
>
> >
> > The diff passes regress and manual tests with and without $ESP/m1n1/,
> > /etc/firmware and /etc/firmware/apple-boot.bin.
>
> Reads good, but I haven't compile- or run-tested it.
>
> Comments inline.
One more version with your comments addressed.
better?
Index: efi_installboot.c
===
RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
retrieving revision 1.7
diff -u -p -r1.7 efi_installboot.c
--- efi_installboot.c 6 Nov 2022 12:33:41 - 1.7
+++ efi_installboot.c 21 Nov 2022 17:58:19 -
@@ -70,6 +70,7 @@
static int create_filesystem(struct disklabel *, char);
static voidwrite_filesystem(struct disklabel *, char);
+static int write_firmware(const char *, const char *);
static int findgptefisys(int, struct disklabel *);
static int findmbrfat(int, struct disklabel *);
@@ -308,6 +309,13 @@ write_filesystem(struct disklabel *dl, c
goto umount;
}
+ dst[mntlen] = '\0';
+ rslt = write_firmware(root, dst);
+ if (rslt == -1) {
+ warnx("unable to write firmware");
+ goto umount;
+ }
+
rslt = 0;
umount:
@@ -325,6 +333,61 @@ rmdir:
if (rslt == -1)
exit(1);
+}
+
+static int
+write_firmware(const char *root, const char *mnt)
+{
+ char dst[PATH_MAX];
+ char fw[PATH_MAX];
+ char *src;
+ struct stat st;
+ int rslt;
+
+ strlcpy(dst, mnt, sizeof(dst));
+
+ /* Skip if no /etc/firmware exists */
+ rslt = snprintf(fw, sizeof(fw), "%s/%s", root, "etc/firmware");
+ if (rslt < 0 || rslt >= PATH_MAX) {
+ warnx("unable to build /etc/firmware path");
+ return -1;
+ }
+ if ((stat(fw, ) != 0) || !S_ISDIR(st.st_mode))
+ return 0;
+
+ /* Copy apple-boot firmware to /m1n1/boot.bin if available */
+ src = fileprefix(fw, "/apple-boot.bin");
+ if (src == NULL)
+ return -1;
+ if (access(src, R_OK) == 0) {
+ if (strlcat(dst, "/m1n1", sizeof(dst)) >= sizeof(dst)) {
+ rslt = -1;
+ warnx("unable to build /m1n1 path");
+ goto cleanup;
+ }
+ if ((stat(dst, ) != 0) || !S_ISDIR(st.st_mode)) {
+ rslt = 0;
+ goto cleanup;
+ }
+ if (strlcat(dst, "/boot.bin", sizeof(dst)) >= sizeof(dst)) {
+ rslt = -1;
+ warnx("unable to build /m1n1/boot.bin path");
+ goto cleanup;
+ }
+ if (verbose)
+ fprintf(stderr, "%s %s to %s\n",
+ (nowrite ? "would copy" : "copying"),
+ src, dst);
+ if (!nowrite)
+ rslt = filecopy(src, dst);
+ if (rslt == -1)
+ goto cleanup;
+ }
+ rslt = 0;
+
+ cleanup:
+ free(src);
+ return rslt;
}
/*
Re: installboot(8): copy apple-boot to ESP
On Mon, Nov 21, 2022 at 03:42:37PM +0100, Tobias Heider wrote:
> Here is a more cleaned up version of the previous diff. I moved all the
> firmware logic to a new write_firmware() function. This should be easy
> to extend if we decide to ship more firmware this way.
This seems more tidy.
>
> The diff passes regress and manual tests with and without $ESP/m1n1/,
> /etc/firmware and /etc/firmware/apple-boot.bin.
Reads good, but I haven't compile- or run-tested it.
Comments inline.
>
> ok?
>
> Index: efi_installboot.c
> ===
> RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 efi_installboot.c
> --- efi_installboot.c 6 Nov 2022 12:33:41 - 1.7
> +++ efi_installboot.c 21 Nov 2022 14:21:29 -
> @@ -70,6 +70,7 @@
>
> static int create_filesystem(struct disklabel *, char);
> static void write_filesystem(struct disklabel *, char);
> +static int write_firmware(char *, char *);
> static int findgptefisys(int, struct disklabel *);
> static int findmbrfat(int, struct disklabel *);
>
> @@ -308,6 +309,13 @@ write_filesystem(struct disklabel *dl, c
> goto umount;
> }
>
> + dst[mntlen] = '\0';
> + if (write_firmware(root, dst) == -1) {
> + warn("unable to write firmware");
> + rslt = -1;
> + goto umount;
> + }
> +
write_firmware() follows the 0/-1 idiom and the following would make
that immediately obvious:
rslt = write_firmware();
if (rslt == -1) {
warnx();
goto unmount;
}
> rslt = 0;
Then this line can also go.
>
> umount:
> @@ -325,6 +333,61 @@ rmdir:
>
> if (rslt == -1)
> exit(1);
> +}
> +
> +static int
> +write_firmware(char *root, char *mnt)
Both arguments are each only read once, so they can be const.
> +{
> + char dst[PATH_MAX];
> + char fw[PATH_MAX];
> + char *src;
> + struct stat st;
> + int rslt;
> +
> + src = NULL;
> + strlcpy(dst, mnt, sizeof(dst));
> +
> + /* Skip if no /etc/firmware exists */
> + rslt = snprintf(fw, sizeof(fw), "%s/%s", root, "etc/firmware");
> + if (rslt < 0 || rslt >= PATH_MAX) {
> + warn("unable to build /etc/firmware path");
I don't think this should print errno.
Most of these "unable to X" warnings already say everything the user
needs to know and thus use warnx(); it seems that efi_installboot.c has
a few warn() by mistake, but I'd have to double check on this.
> + return -1;
> + }
> + if ((stat(fw, ) != 0) || !S_ISDIR(st.st_mode))
> + return 0;
> +
> + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> + src = fileprefix(fw, "/apple-boot.bin");
> + if (src == NULL)
> + return -1;
> + if (access(src, R_OK) == 0) {
> + if (strlcat(dst, "/m1n1", sizeof(dst)) >= sizeof(dst)) {
> + rslt = -1;
> + warn("unable to build /m1n1 path");
> + goto cleanup;
Nit: the order is inconsistent with the rest of installboot, which does
warnx();
rslt = -1;
goto somewhere;
> + }
> + if ((stat(dst, ) != 0) || !S_ISDIR(st.st_mode)) {
> + rslt = 0;
> + goto cleanup;
> + }
> + if (strlcat(dst, "/boot.bin", sizeof(dst)) >= sizeof(dst)) {
> + rslt = -1;
> + warn("unable to build /m1n1/boot.bin path");
> + goto cleanup;
> + }
> + if (verbose)
> + fprintf(stderr, "%s %s to %s\n",
> + (nowrite ? "would copy" : "copying"),
> + src, dst);
> + if (!nowrite)
> + rslt = filecopy(src, dst);
> + if (rslt == -1)
> + goto cleanup;
> + }
> + rslt = 0;
> + cleanup:
> + free(src);
> + return rslt;
> }
>
> /*
>
Re: installboot(8): copy apple-boot to ESP
> Date: Mon, 21 Nov 2022 15:42:37 +0100
> From: Tobias Heider
>
> On Sat, Nov 19, 2022 at 08:27:18PM +0100, Tobias Heider wrote:
> > On Sat, Nov 19, 2022 at 07:25:52PM +0100, Mark Kettenis wrote:
> > > > Date: Sat, 19 Nov 2022 18:44:19 +0100
> > > > From: Tobias Heider
> > > >
> > > > On Sat, Nov 19, 2022 at 06:33:51PM +0100, Mark Kettenis wrote:
> > > > > > Date: Sat, 19 Nov 2022 18:26:36 +0100
> > > > > > From: Tobias Heider
> > > > > >
> > > > > > Here is the promised last diff we need to enable Apple M*
> > > > > > bootloader updates.
> > > > > >
> > > > > > With this, installboot(8) will pick up apple-boot.bin from the
> > > > > > firmware
> > > > > > directory and writes it to $ESP/m1n1/boot.bin if both file and
> > > > > > target
> > > > > > directory exist.
> > > > > > Creation of the m1n1/ directory is expected to happen during the
> > > > > > initial
> > > > > > Asahi Linux EFI environment installation so it is safe to assume it
> > > > > > is
> > > > > > already there when we need it.
> > > > > >
> > > > > > Running this on my M2 gives me:
> > > > > >
> > > > > > kischt# installboot -v sd0
> > > > > > Using / as root
> > > > > > installing bootstrap on /dev/rsd0c
> > > > > > using first-stage /usr/mdec/BOOTAA64.EFI
> > > > > > copying /etc/firmware/apple-boot.bin to
> > > > > > /tmp/installboot.0JSQ0XrWRB/m1n1/boot.bin
> > > > > > copying /usr/mdec/BOOTAA64.EFI to
> > > > > > /tmp/installboot.0JSQ0XrWRB/efi/boot/bootaa64.efi
> > > > > > writing /tmp/installboot.0JSQ0XrWRB/efi/boot/startup.nsh
> > > > >
> > > > > Hmm...
> > > > >
> > > > > > Index: efi_installboot.c
> > > > > > ===
> > > > > > RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> > > > > > retrieving revision 1.6
> > > > > > diff -u -p -r1.6 efi_installboot.c
> > > > > > --- efi_installboot.c 14 Sep 2022 16:43:00 - 1.6
> > > > > > +++ efi_installboot.c 19 Nov 2022 16:45:36 -
> > > > > > @@ -191,6 +191,7 @@ write_filesystem(struct disklabel *dl, c
> > > > > > struct msdosfs_args args;
> > > > > > char cmd[60];
> > > > > > char dst[PATH_MAX];
> > > > > > + struct stat st;
> > > > > > char *src;
> > > > > > size_t mntlen, pathlen, srclen;
> > > > > > int rslt;
> > > > > > @@ -245,7 +246,36 @@ write_filesystem(struct disklabel *dl, c
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> > > > > > + src = fileprefix(root, "/etc/firmware/apple-boot.bin");
> > > > > > + if (src == NULL) {
> > > > > > + rslt = -1;
> > > > > > + goto umount;
> > > > >
> > > > > Doesn't this mean that if /etc/firmware/apple-boot.bin doesn't exist,
> > > > > we won't write the OpenBSD bootloader to the filesystem? That would
> > > > > be wrong.
> > > > >
> > > > > So I think the Apple "magic" needs to be done at the end of the
> > > > > function.
> > > >
> > > > That is what I also first thought, but fileprefix() just concats the
> > > > strings
> > > > and doesn't actually touch the file system. This is also why I added
> > > > my own
> > > > stat() && S_ISDIR() check below to make sure the directory exists.
> > > >
> > > > Wit apple-boot manually deleted I get:
> > > >
> > > > kischt# installboot -v sd0
> > > > Using / as root
> > > > installing bootstrap on /dev/rsd0c
> > > > using first-stage /usr/mdec/BOOTAA64.EFI
> > > > copying /usr/mdec/BOOTAA64.EFI to
> > > > /tmp/installboot.Tm5s4GRaRT/efi/boot/bootaa64.efi
> > > > writing /tmp/installboot.Tm5s4GRaRT/efi/boot/startup.nsh
> > >
> > > Hmm, but it will still fail if the /etc/firmware directory doesn't
> > > exist under the "root". Which may be the case if you are using the -r
> > > option?
> > >
> >
> > Right, regress also trips over this.
> >
> > Moving the whole block inside fileprefix() != NULL is not enough since
> > that would still print the error message. We might have to add our own
> > check fore $root/etc/firmware, although that duplicates some of the
> > code in fileprefix().
> >
>
> Here is a more cleaned up version of the previous diff. I moved all the
> firmware logic to a new write_firmware() function. This should be easy
> to extend if we decide to ship more firmware this way.
>
> The diff passes regress and manual tests with and without $ESP/m1n1/,
> /etc/firmware and /etc/firmware/apple-boot.bin.
>
> ok?
I like this a lot better. Two nits below, but either way, ok kettenis@
> Index: efi_installboot.c
> ===
> RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 efi_installboot.c
> --- efi_installboot.c 6 Nov 2022 12:33:41 - 1.7
> +++ efi_installboot.c 21 Nov 2022 14:21:29 -
> @@ -70,6 +70,7 @@
>
> static int create_filesystem(struct disklabel *, char);
> static void
Re: installboot(8): copy apple-boot to ESP
On Sat, Nov 19, 2022 at 08:27:18PM +0100, Tobias Heider wrote:
> On Sat, Nov 19, 2022 at 07:25:52PM +0100, Mark Kettenis wrote:
> > > Date: Sat, 19 Nov 2022 18:44:19 +0100
> > > From: Tobias Heider
> > >
> > > On Sat, Nov 19, 2022 at 06:33:51PM +0100, Mark Kettenis wrote:
> > > > > Date: Sat, 19 Nov 2022 18:26:36 +0100
> > > > > From: Tobias Heider
> > > > >
> > > > > Here is the promised last diff we need to enable Apple M* bootloader
> > > > > updates.
> > > > >
> > > > > With this, installboot(8) will pick up apple-boot.bin from the
> > > > > firmware
> > > > > directory and writes it to $ESP/m1n1/boot.bin if both file and target
> > > > > directory exist.
> > > > > Creation of the m1n1/ directory is expected to happen during the
> > > > > initial
> > > > > Asahi Linux EFI environment installation so it is safe to assume it is
> > > > > already there when we need it.
> > > > >
> > > > > Running this on my M2 gives me:
> > > > >
> > > > > kischt# installboot -v sd0
> > > > > Using / as root
> > > > > installing bootstrap on /dev/rsd0c
> > > > > using first-stage /usr/mdec/BOOTAA64.EFI
> > > > > copying /etc/firmware/apple-boot.bin to
> > > > > /tmp/installboot.0JSQ0XrWRB/m1n1/boot.bin
> > > > > copying /usr/mdec/BOOTAA64.EFI to
> > > > > /tmp/installboot.0JSQ0XrWRB/efi/boot/bootaa64.efi
> > > > > writing /tmp/installboot.0JSQ0XrWRB/efi/boot/startup.nsh
> > > >
> > > > Hmm...
> > > >
> > > > > Index: efi_installboot.c
> > > > > ===
> > > > > RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> > > > > retrieving revision 1.6
> > > > > diff -u -p -r1.6 efi_installboot.c
> > > > > --- efi_installboot.c 14 Sep 2022 16:43:00 - 1.6
> > > > > +++ efi_installboot.c 19 Nov 2022 16:45:36 -
> > > > > @@ -191,6 +191,7 @@ write_filesystem(struct disklabel *dl, c
> > > > > struct msdosfs_args args;
> > > > > char cmd[60];
> > > > > char dst[PATH_MAX];
> > > > > + struct stat st;
> > > > > char *src;
> > > > > size_t mntlen, pathlen, srclen;
> > > > > int rslt;
> > > > > @@ -245,7 +246,36 @@ write_filesystem(struct disklabel *dl, c
> > > > > }
> > > > > }
> > > > >
> > > > > + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> > > > > + src = fileprefix(root, "/etc/firmware/apple-boot.bin");
> > > > > + if (src == NULL) {
> > > > > + rslt = -1;
> > > > > + goto umount;
> > > >
> > > > Doesn't this mean that if /etc/firmware/apple-boot.bin doesn't exist,
> > > > we won't write the OpenBSD bootloader to the filesystem? That would
> > > > be wrong.
> > > >
> > > > So I think the Apple "magic" needs to be done at the end of the
> > > > function.
> > >
> > > That is what I also first thought, but fileprefix() just concats the
> > > strings
> > > and doesn't actually touch the file system. This is also why I added my
> > > own
> > > stat() && S_ISDIR() check below to make sure the directory exists.
> > >
> > > Wit apple-boot manually deleted I get:
> > >
> > > kischt# installboot -v sd0
> > > Using / as root
> > > installing bootstrap on /dev/rsd0c
> > > using first-stage /usr/mdec/BOOTAA64.EFI
> > > copying /usr/mdec/BOOTAA64.EFI to
> > > /tmp/installboot.Tm5s4GRaRT/efi/boot/bootaa64.efi
> > > writing /tmp/installboot.Tm5s4GRaRT/efi/boot/startup.nsh
> >
> > Hmm, but it will still fail if the /etc/firmware directory doesn't
> > exist under the "root". Which may be the case if you are using the -r
> > option?
> >
>
> Right, regress also trips over this.
>
> Moving the whole block inside fileprefix() != NULL is not enough since
> that would still print the error message. We might have to add our own
> check fore $root/etc/firmware, although that duplicates some of the
> code in fileprefix().
>
Here is a more cleaned up version of the previous diff. I moved all the
firmware logic to a new write_firmware() function. This should be easy
to extend if we decide to ship more firmware this way.
The diff passes regress and manual tests with and without $ESP/m1n1/,
/etc/firmware and /etc/firmware/apple-boot.bin.
ok?
Index: efi_installboot.c
===
RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
retrieving revision 1.7
diff -u -p -r1.7 efi_installboot.c
--- efi_installboot.c 6 Nov 2022 12:33:41 - 1.7
+++ efi_installboot.c 21 Nov 2022 14:21:29 -
@@ -70,6 +70,7 @@
static int create_filesystem(struct disklabel *, char);
static voidwrite_filesystem(struct disklabel *, char);
+static int write_firmware(char *, char *);
static int findgptefisys(int, struct disklabel *);
static int findmbrfat(int, struct disklabel *);
@@ -308,6 +309,13 @@ write_filesystem(struct disklabel *dl, c
goto umount;
}
+ dst[mntlen] = '\0';
+ if
Re: installboot(8): copy apple-boot to ESP
On Sat, Nov 19, 2022 at 07:25:52PM +0100, Mark Kettenis wrote:
> > Date: Sat, 19 Nov 2022 18:44:19 +0100
> > From: Tobias Heider
> >
> > On Sat, Nov 19, 2022 at 06:33:51PM +0100, Mark Kettenis wrote:
> > > > Date: Sat, 19 Nov 2022 18:26:36 +0100
> > > > From: Tobias Heider
> > > >
> > > > Here is the promised last diff we need to enable Apple M* bootloader
> > > > updates.
> > > >
> > > > With this, installboot(8) will pick up apple-boot.bin from the firmware
> > > > directory and writes it to $ESP/m1n1/boot.bin if both file and target
> > > > directory exist.
> > > > Creation of the m1n1/ directory is expected to happen during the initial
> > > > Asahi Linux EFI environment installation so it is safe to assume it is
> > > > already there when we need it.
> > > >
> > > > Running this on my M2 gives me:
> > > >
> > > > kischt# installboot -v sd0
> > > > Using / as root
> > > > installing bootstrap on /dev/rsd0c
> > > > using first-stage /usr/mdec/BOOTAA64.EFI
> > > > copying /etc/firmware/apple-boot.bin to
> > > > /tmp/installboot.0JSQ0XrWRB/m1n1/boot.bin
> > > > copying /usr/mdec/BOOTAA64.EFI to
> > > > /tmp/installboot.0JSQ0XrWRB/efi/boot/bootaa64.efi
> > > > writing /tmp/installboot.0JSQ0XrWRB/efi/boot/startup.nsh
> > >
> > > Hmm...
> > >
> > > > Index: efi_installboot.c
> > > > ===
> > > > RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> > > > retrieving revision 1.6
> > > > diff -u -p -r1.6 efi_installboot.c
> > > > --- efi_installboot.c 14 Sep 2022 16:43:00 - 1.6
> > > > +++ efi_installboot.c 19 Nov 2022 16:45:36 -
> > > > @@ -191,6 +191,7 @@ write_filesystem(struct disklabel *dl, c
> > > > struct msdosfs_args args;
> > > > char cmd[60];
> > > > char dst[PATH_MAX];
> > > > + struct stat st;
> > > > char *src;
> > > > size_t mntlen, pathlen, srclen;
> > > > int rslt;
> > > > @@ -245,7 +246,36 @@ write_filesystem(struct disklabel *dl, c
> > > > }
> > > > }
> > > >
> > > > + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> > > > + src = fileprefix(root, "/etc/firmware/apple-boot.bin");
> > > > + if (src == NULL) {
> > > > + rslt = -1;
> > > > + goto umount;
> > >
> > > Doesn't this mean that if /etc/firmware/apple-boot.bin doesn't exist,
> > > we won't write the OpenBSD bootloader to the filesystem? That would
> > > be wrong.
> > >
> > > So I think the Apple "magic" needs to be done at the end of the
> > > function.
> >
> > That is what I also first thought, but fileprefix() just concats the strings
> > and doesn't actually touch the file system. This is also why I added my own
> > stat() && S_ISDIR() check below to make sure the directory exists.
> >
> > Wit apple-boot manually deleted I get:
> >
> > kischt# installboot -v sd0
> > Using / as root
> > installing bootstrap on /dev/rsd0c
> > using first-stage /usr/mdec/BOOTAA64.EFI
> > copying /usr/mdec/BOOTAA64.EFI to
> > /tmp/installboot.Tm5s4GRaRT/efi/boot/bootaa64.efi
> > writing /tmp/installboot.Tm5s4GRaRT/efi/boot/startup.nsh
>
> Hmm, but it will still fail if the /etc/firmware directory doesn't
> exist under the "root". Which may be the case if you are using the -r
> option?
>
Right, regress also trips over this.
Moving the whole block inside fileprefix() != NULL is not enough since
that would still print the error message. We might have to add our own
check fore $root/etc/firmware, although that duplicates some of the
code in fileprefix().
Index: efi_installboot.c
===
RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
retrieving revision 1.7
diff -u -p -r1.7 efi_installboot.c
--- efi_installboot.c 6 Nov 2022 12:33:41 - 1.7
+++ efi_installboot.c 19 Nov 2022 19:19:28 -
@@ -194,6 +194,8 @@ write_filesystem(struct disklabel *dl, c
struct msdosfs_args args;
char cmd[60];
char dst[PATH_MAX];
+ char fw[PATH_MAX];
+ struct stat st;
char *src;
size_t mntlen, pathlen, srclen;
int rslt;
@@ -248,7 +250,46 @@ write_filesystem(struct disklabel *dl, c
}
}
+ /* Copy apple-boot firmware to /m1n1/boot.bin if available */
+ pathlen = strlen(dst);
+ if (strlcat(dst, "/m1n1", sizeof(dst)) >= sizeof(dst)) {
+ rslt = -1;
+ warn("unable to build /m1n1 path");
+ goto umount;
+ }
+ rslt = snprintf(fw, PATH_MAX, "%s/%s", root, "/etc/firmware");
+ if (rslt < 0 || rslt >= PATH_MAX) {
+ rslt = -1;
+ warn("unable to build /etc/firmware path");
+ goto umount;
+ }
+ if ((stat(fw, ) == 0) && S_ISDIR(st.st_mode) &&
+ (stat(dst, ) == 0) && S_ISDIR(st.st_mode)) {
+ src = fileprefix(fw,
Re: installboot(8): copy apple-boot to ESP
> Date: Sat, 19 Nov 2022 18:44:19 +0100
> From: Tobias Heider
>
> On Sat, Nov 19, 2022 at 06:33:51PM +0100, Mark Kettenis wrote:
> > > Date: Sat, 19 Nov 2022 18:26:36 +0100
> > > From: Tobias Heider
> > >
> > > Here is the promised last diff we need to enable Apple M* bootloader
> > > updates.
> > >
> > > With this, installboot(8) will pick up apple-boot.bin from the firmware
> > > directory and writes it to $ESP/m1n1/boot.bin if both file and target
> > > directory exist.
> > > Creation of the m1n1/ directory is expected to happen during the initial
> > > Asahi Linux EFI environment installation so it is safe to assume it is
> > > already there when we need it.
> > >
> > > Running this on my M2 gives me:
> > >
> > > kischt# installboot -v sd0
> > > Using / as root
> > > installing bootstrap on /dev/rsd0c
> > > using first-stage /usr/mdec/BOOTAA64.EFI
> > > copying /etc/firmware/apple-boot.bin to
> > > /tmp/installboot.0JSQ0XrWRB/m1n1/boot.bin
> > > copying /usr/mdec/BOOTAA64.EFI to
> > > /tmp/installboot.0JSQ0XrWRB/efi/boot/bootaa64.efi
> > > writing /tmp/installboot.0JSQ0XrWRB/efi/boot/startup.nsh
> >
> > Hmm...
> >
> > > Index: efi_installboot.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> > > retrieving revision 1.6
> > > diff -u -p -r1.6 efi_installboot.c
> > > --- efi_installboot.c 14 Sep 2022 16:43:00 - 1.6
> > > +++ efi_installboot.c 19 Nov 2022 16:45:36 -
> > > @@ -191,6 +191,7 @@ write_filesystem(struct disklabel *dl, c
> > > struct msdosfs_args args;
> > > char cmd[60];
> > > char dst[PATH_MAX];
> > > + struct stat st;
> > > char *src;
> > > size_t mntlen, pathlen, srclen;
> > > int rslt;
> > > @@ -245,7 +246,36 @@ write_filesystem(struct disklabel *dl, c
> > > }
> > > }
> > >
> > > + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> > > + src = fileprefix(root, "/etc/firmware/apple-boot.bin");
> > > + if (src == NULL) {
> > > + rslt = -1;
> > > + goto umount;
> >
> > Doesn't this mean that if /etc/firmware/apple-boot.bin doesn't exist,
> > we won't write the OpenBSD bootloader to the filesystem? That would
> > be wrong.
> >
> > So I think the Apple "magic" needs to be done at the end of the
> > function.
>
> That is what I also first thought, but fileprefix() just concats the strings
> and doesn't actually touch the file system. This is also why I added my own
> stat() && S_ISDIR() check below to make sure the directory exists.
>
> Wit apple-boot manually deleted I get:
>
> kischt# installboot -v sd0
> Using / as root
> installing bootstrap on /dev/rsd0c
> using first-stage /usr/mdec/BOOTAA64.EFI
> copying /usr/mdec/BOOTAA64.EFI to
> /tmp/installboot.Tm5s4GRaRT/efi/boot/bootaa64.efi
> writing /tmp/installboot.Tm5s4GRaRT/efi/boot/startup.nsh
Hmm, but it will still fail if the /etc/firmware directory doesn't
exist under the "root". Which may be the case if you are using the -r
option?
> > > + }
> > > + pathlen = strlen(dst);
> > > + if (strlcat(dst, "/m1n1", sizeof(dst)) >= sizeof(dst)) {
> > > + rslt = -1;
> > > + warn("unable to build /m1n1 path");
> > > + goto umount;
> > > + }
> > > + if ((access(src, R_OK) == 0) &&
> > > + (stat(dst, ) == 0) && S_ISDIR(st.st_mode)) {
> > > + if (strlcat(dst, "/boot.bin", sizeof(dst)) >= sizeof(dst)) {
> > > + rslt = -1;
> > > + warn("unable to build /m1n1/boot.bin path");
> > > + goto umount;
> > > + }
> > > + if (verbose)
> > > + fprintf(stderr, "%s %s to %s\n",
> > > + (nowrite ? "would copy" : "copying"), src, dst);
> > > + if (!nowrite)
> > > + rslt = filecopy(src, dst);
> > > + if (rslt == -1)
> > > + goto umount;
> > > + }
> > > +
> > > /* Create "/efi/boot" directory in .. */
> > > + dst[pathlen] = '\0';
> > > if (strlcat(dst, "/efi", sizeof(dst)) >= sizeof(dst)) {
> > > rslt = -1;
> > > warn("unable to build /efi directory");
> > >
> > >
>
Re: installboot(8): copy apple-boot to ESP
On Sat, Nov 19, 2022 at 06:33:51PM +0100, Mark Kettenis wrote:
> > Date: Sat, 19 Nov 2022 18:26:36 +0100
> > From: Tobias Heider
> >
> > Here is the promised last diff we need to enable Apple M* bootloader
> > updates.
> >
> > With this, installboot(8) will pick up apple-boot.bin from the firmware
> > directory and writes it to $ESP/m1n1/boot.bin if both file and target
> > directory exist.
> > Creation of the m1n1/ directory is expected to happen during the initial
> > Asahi Linux EFI environment installation so it is safe to assume it is
> > already there when we need it.
> >
> > Running this on my M2 gives me:
> >
> > kischt# installboot -v sd0
> > Using / as root
> > installing bootstrap on /dev/rsd0c
> > using first-stage /usr/mdec/BOOTAA64.EFI
> > copying /etc/firmware/apple-boot.bin to
> > /tmp/installboot.0JSQ0XrWRB/m1n1/boot.bin
> > copying /usr/mdec/BOOTAA64.EFI to
> > /tmp/installboot.0JSQ0XrWRB/efi/boot/bootaa64.efi
> > writing /tmp/installboot.0JSQ0XrWRB/efi/boot/startup.nsh
>
> Hmm...
>
> > Index: efi_installboot.c
> > ===
> > RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 efi_installboot.c
> > --- efi_installboot.c 14 Sep 2022 16:43:00 - 1.6
> > +++ efi_installboot.c 19 Nov 2022 16:45:36 -
> > @@ -191,6 +191,7 @@ write_filesystem(struct disklabel *dl, c
> > struct msdosfs_args args;
> > char cmd[60];
> > char dst[PATH_MAX];
> > + struct stat st;
> > char *src;
> > size_t mntlen, pathlen, srclen;
> > int rslt;
> > @@ -245,7 +246,36 @@ write_filesystem(struct disklabel *dl, c
> > }
> > }
> >
> > + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> > + src = fileprefix(root, "/etc/firmware/apple-boot.bin");
> > + if (src == NULL) {
> > + rslt = -1;
> > + goto umount;
>
> Doesn't this mean that if /etc/firmware/apple-boot.bin doesn't exist,
> we won't write the OpenBSD bootloader to the filesystem? That would
> be wrong.
>
> So I think the Apple "magic" needs to be done at the end of the
> function.
That is what I also first thought, but fileprefix() just concats the strings
and doesn't actually touch the file system. This is also why I added my own
stat() && S_ISDIR() check below to make sure the directory exists.
Wit apple-boot manually deleted I get:
kischt# installboot -v sd0
Using / as root
installing bootstrap on /dev/rsd0c
using first-stage /usr/mdec/BOOTAA64.EFI
copying /usr/mdec/BOOTAA64.EFI to
/tmp/installboot.Tm5s4GRaRT/efi/boot/bootaa64.efi
writing /tmp/installboot.Tm5s4GRaRT/efi/boot/startup.nsh
>
> > + }
> > + pathlen = strlen(dst);
> > + if (strlcat(dst, "/m1n1", sizeof(dst)) >= sizeof(dst)) {
> > + rslt = -1;
> > + warn("unable to build /m1n1 path");
> > + goto umount;
> > + }
> > + if ((access(src, R_OK) == 0) &&
> > + (stat(dst, ) == 0) && S_ISDIR(st.st_mode)) {
> > + if (strlcat(dst, "/boot.bin", sizeof(dst)) >= sizeof(dst)) {
> > + rslt = -1;
> > + warn("unable to build /m1n1/boot.bin path");
> > + goto umount;
> > + }
> > + if (verbose)
> > + fprintf(stderr, "%s %s to %s\n",
> > + (nowrite ? "would copy" : "copying"), src, dst);
> > + if (!nowrite)
> > + rslt = filecopy(src, dst);
> > + if (rslt == -1)
> > + goto umount;
> > + }
> > +
> > /* Create "/efi/boot" directory in .. */
> > + dst[pathlen] = '\0';
> > if (strlcat(dst, "/efi", sizeof(dst)) >= sizeof(dst)) {
> > rslt = -1;
> > warn("unable to build /efi directory");
> >
> >
Re: installboot(8): copy apple-boot to ESP
> Date: Sat, 19 Nov 2022 18:26:36 +0100
> From: Tobias Heider
>
> Here is the promised last diff we need to enable Apple M* bootloader updates.
>
> With this, installboot(8) will pick up apple-boot.bin from the firmware
> directory and writes it to $ESP/m1n1/boot.bin if both file and target
> directory exist.
> Creation of the m1n1/ directory is expected to happen during the initial
> Asahi Linux EFI environment installation so it is safe to assume it is
> already there when we need it.
>
> Running this on my M2 gives me:
>
> kischt# installboot -v sd0
> Using / as root
> installing bootstrap on /dev/rsd0c
> using first-stage /usr/mdec/BOOTAA64.EFI
> copying /etc/firmware/apple-boot.bin to
> /tmp/installboot.0JSQ0XrWRB/m1n1/boot.bin
> copying /usr/mdec/BOOTAA64.EFI to
> /tmp/installboot.0JSQ0XrWRB/efi/boot/bootaa64.efi
> writing /tmp/installboot.0JSQ0XrWRB/efi/boot/startup.nsh
Hmm...
> Index: efi_installboot.c
> ===
> RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 efi_installboot.c
> --- efi_installboot.c 14 Sep 2022 16:43:00 - 1.6
> +++ efi_installboot.c 19 Nov 2022 16:45:36 -
> @@ -191,6 +191,7 @@ write_filesystem(struct disklabel *dl, c
> struct msdosfs_args args;
> char cmd[60];
> char dst[PATH_MAX];
> + struct stat st;
> char *src;
> size_t mntlen, pathlen, srclen;
> int rslt;
> @@ -245,7 +246,36 @@ write_filesystem(struct disklabel *dl, c
> }
> }
>
> + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> + src = fileprefix(root, "/etc/firmware/apple-boot.bin");
> + if (src == NULL) {
> + rslt = -1;
> + goto umount;
Doesn't this mean that if /etc/firmware/apple-boot.bin doesn't exist,
we won't write the OpenBSD bootloader to the filesystem? That would
be wrong.
So I think the Apple "magic" needs to be done at the end of the
function.
> + }
> + pathlen = strlen(dst);
> + if (strlcat(dst, "/m1n1", sizeof(dst)) >= sizeof(dst)) {
> + rslt = -1;
> + warn("unable to build /m1n1 path");
> + goto umount;
> + }
> + if ((access(src, R_OK) == 0) &&
> + (stat(dst, ) == 0) && S_ISDIR(st.st_mode)) {
> + if (strlcat(dst, "/boot.bin", sizeof(dst)) >= sizeof(dst)) {
> + rslt = -1;
> + warn("unable to build /m1n1/boot.bin path");
> + goto umount;
> + }
> + if (verbose)
> + fprintf(stderr, "%s %s to %s\n",
> + (nowrite ? "would copy" : "copying"), src, dst);
> + if (!nowrite)
> + rslt = filecopy(src, dst);
> + if (rslt == -1)
> + goto umount;
> + }
> +
> /* Create "/efi/boot" directory in .. */
> + dst[pathlen] = '\0';
> if (strlcat(dst, "/efi", sizeof(dst)) >= sizeof(dst)) {
> rslt = -1;
> warn("unable to build /efi directory");
>
>
