[bug#41011] gnu: grub: Support for network boot via TFTP.

* gnu/bootloader/grub.scm (grub-efi-netboot-bootloader): New bootloader for
network booting.
(install-grub-efi-netboot): New bootloader installer for network booting.
(grub-root-search): Set the root to "(tftp)" if the searched file is not stored
on a local devices, i.e. an NFS share.
---
 gnu/bootloader/grub.scm | 97 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

On Sun, Sep 13, 2020 at 07:46:01PM +0200, Stefan wrote:
> * gnu/bootloader/grub.scm (grub-efi-netboot-bootloader): New bootloader for
> network booting.
> (install-grub-efi-netboot): New bootloader installer for network booting.
> (grub-root-search): Set the root to "(tftp)" if the searched file is not stored
> on a local devices, i.e. an NFS share.
> ---
>  gnu/bootloader/grub.scm | 97 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
> index e3febeefd0..552bc34f5a 100644
> --- a/gnu/bootloader/grub.scm
> +++ b/gnu/bootloader/grub.scm
> @@ -23,8 +23,10 @@
>  ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
>  
>  (define-module (gnu bootloader grub)
> +  #:use-module (guix build union)
>    #:use-module (guix records)
> -  #:use-module ((guix utils) #:select (%current-system))
> +  #:use-module (guix store)
> +  #:use-module ((guix utils) #:select (%current-system %current-target-system))
>    #:use-module (guix gexp)
>    #:use-module (gnu artwork)
>    #:use-module (gnu bootloader)
> @@ -46,8 +48,11 @@
>              grub-theme-color-highlight
>              grub-theme-gfxmode
>  
> +            install-grub-efi-netboot
> +
>              grub-bootloader
>              grub-efi-bootloader
> +            grub-efi-netboot-bootloader
>              grub-mkrescue-bootloader
>              grub-minimal-bootloader
>  
> @@ -295,6 +300,9 @@ code."
>          ((? file-system-label? label)
>           (format #f "search --label --set ~a"
>                   (file-system-label->string label)))
> +        ((? (lambda (device)
> +              (and (string? device) (string-contains device ":/"))) nfs-uri)
> +         "set root=(tftp)")
>          ((or #f (? string?))
>           #~(format #f "search --file --set ~a" #$file)))))
>  
> @@ -501,6 +509,87 @@ fi~%"))))
>                        "--bootloader-id=Guix"
>                        "--efi-directory" target-esp))))
>  
> +(define (install-grub-efi-netboot subdir)
> +  "Define a grub-efi-netboot bootloader installer for installation in SUBDIR,
> +which is usually efi/Guix or efi/boot."
> +  (let* ((system (string-split (or (%current-target-system)
> +                                   (%current-system))
> +                               #\-))
> +         (boot-efi-link (match system
> +                          (("i686" _ ...) "/bootia32.efi")
> +                          (("x86_64" _ ...) "/bootx64.efi")
> +                          (("arm" _ ...) "/bootarm.efi")
> +                          (("armhf" _ ...) "/bootarm.efi")
> +                          (("aarch64" _ ...) "/bootaa64.efi")
> +                          (("riscv" _ ...) "/bootriscv32.efi")
> +                          (("riscv64" _ ...) "/bootriscv64.efi")))

Don't forget the fall-through case, even if it's just
((_ ...) "/bootTODO.efi")

> +         (efi-bootloader (string-append (match system
> +                                          (("i686" _ ...) "i386-efi")
> +                                          (("x86_64" _ ...) "x86_64-efi")
> +                                          (("arm" _ ...) "arm-efi")
> +                                          (("armhf" _ ...) "arm-efi")
> +                                          (("aarch64" _ ...) "arm64-efi")
> +                                          (("riscv" _ ...) "riscv32-efi")
> +                                          (("riscv64" _ ...) "riscv64-efi"))
> +                                        "/core.efi")))

With the fall-through case here, can this be changed to
(("i686" _ ...) "i386-efi")
(("aarch64" _ ...) "arm64-efi")
(("riscv" _ ...) "riscv32-efi")
((_ ...) (string-append (first
                         (string-split
                           (nix-system->gnu-triplet
                             (or (%current-system)
                                 (%current-target-system)))
                           #\_))
                        "-efi"))

> +    (with-imported-modules
> +     '((guix build union))
> +     #~(lambda (bootloader target mount-point)
> +         "Install the BOOTLOADER, which must be the package grub, as e.g.
> +bootx64.efi or bootarm64.efi into SUBDIR, which is usually efi/Guix or efi/boot,
> +below the directory TARGET for the system whose root is mounted at MOUNT-POINT.
> +
> +MOUNT-POINT is the last argument in 'guix system init /etc/config.scm mnt/point'
> +or '/' for other 'guix system' commands.
> +
> +TARGET is the target argument given to the bootloader-configuration in
> +(operating-system
> + (bootloader (bootloader-configuration
> +              (target \"/boot\")
> +              …))
> + …)
> +TARGET is required to be an absolute path and must be provided by a TFTP server
> +as the TFTP root directory.
> +
> +GRUB will load tftp://server/SUBDIR/grub.cfg and this file will instruct it to
> +load more files from the store like tftp://server/gnu/store/…-linux…/Image.
> +
> +To make this possible two symlinks will be created. The first symlink points
> +relatively form TARGET/SUBDIR/grub.cfg to /boot/grub/grub.cfg. And the second
> +symlink points relatively from TARGET/%store-prefix to %store-prefix.
> +
> +It is important to note that these symlinks need to be relativ, as the absolute
> +paths on the TFTP server side are unknown.
> +
> +It is also important to note that both symlinks will point outside the TFTP root
> +directory and that the TARGET/%store-prefix symlink makes the whole store
> +accessible via TFTP. Possibly the TFTP server must be configured
> +to allow accesses outside its TFTP root directory. This may need to be
> +considered for security aspects."
> +         (use-modules ((guix build union) #:select (symlink-relative)))
> +         (let* ((net-dir (string-append mount-point target "/"))
> +                (sub-dir (string-append net-dir #$subdir "/"))
> +                (store-link (string-append net-dir (%store-prefix)))
> +                (grub-cfg "/boot/grub/grub.cfg")
> +                (grub-cfg-link (string-append sub-dir (basename grub-cfg)))
> +                (boot-efi-link (string-append sub-dir #$boot-efi-link)))
> +           ;; Prepare the symlink to the store.
> +           (mkdir-p (dirname store-link))
> +           (false-if-exception (delete-file store-link))
> +           (symlink-relative (%store-prefix) store-link)
> +           ;; Prepare the symlink to the grub.cfg, which points into the store.
> +           (false-if-exception (delete-file grub-cfg-link))
> +           (symlink-relative grub-cfg grub-cfg-link)
> +           ;; Install GRUB, which refers to the grub.cfg, with support for
> +           ;; encrypted partitions,
> +           (setenv "GRUB_ENABLE_CRYPTODISK" "y")
> +           (invoke/quiet (string-append bootloader "/bin/grub-mknetdir")
> +                         (string-append "--net-directory=" net-dir)
> +                         (string-append "--subdir=" #$subdir))
> +           ;; Prepare the bootloader symlink, which points to GRUB.
> +           (false-if-exception (delete-file boot-efi-link))
> +           (symlink #$efi-bootloader boot-efi-link))))))
> +
>  ^L
>  
>  ;;;
> @@ -533,6 +622,12 @@ fi~%"))))
>     (name 'grub-efi)
>     (package grub-efi)))
>  
> +(define grub-efi-netboot-bootloader
> +  (bootloader
> +   (inherit grub-efi-bootloader)
> +   (name 'grub-efi-netboot-bootloader)
> +   (installer (install-grub-efi-netboot "efi/Guix"))))
> +
>  (define grub-mkrescue-bootloader
>    (bootloader
>     (inherit grub-efi-bootloader)
> -- 
> 2.26.0
> 
> 
> 
>

Hi Stefan,

On Sun, 13 Sep 2020 19:46:01 +0200
Stefan <stefan-guix@vodafonemail.de> wrote:

> diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
> index e3febeefd0..552bc34f5a 100644
> --- a/gnu/bootloader/grub.scm
> +++ b/gnu/bootloader/grub.scm
> @@ -295,6 +300,9 @@ code."
>          ((? file-system-label? label)
>           (format #f "search --label --set ~a"
>                   (file-system-label->string label)))
> +        ((? (lambda (device)
> +              (and (string? device) (string-contains device ":/"))) nfs-uri)
> +         "set root=(tftp)")
>          ((or #f (? string?))
>           #~(format #f "search --file --set ~a" #$file)))))

After careful consideration, I've pushed this to guix master, with a big
comment in the source code as to why we are doing what we are doing.

I can see no downside to defaulting to TFTP for the time being.

I do wonder if there are cases now where Grub tries to use TFTP when the user
meant to boot locally--but nothing comes to mind. 

Thank you for your elaborative E-Mails.

I will still review the remainder.  Please take Efraim's comments into
consideration.

Hi Efraim!

>> +         (boot-efi-link (match system
>> +                          (("i686" _ ...) "/bootia32.efi")
>> +                          (("x86_64" _ ...) "/bootx64.efi")
>> +                          (("arm" _ ...) "/bootarm.efi")
>> +                          (("armhf" _ ...) "/bootarm.efi")
>> +                          (("aarch64" _ ...) "/bootaa64.efi")
>> +                          (("riscv" _ ...) "/bootriscv32.efi")
>> +                          (("riscv64" _ ...) "/bootriscv64.efi")))
> 
> Don't forget the fall-through case, even if it's just
> ((_ ...) "/bootTODO.efi")

There was a contradicting remark by Danny:

> The major advantage of using "match" is its failure mode.  If the thing matched
> on is not a list (for some unfathomable reason) or if the first element is not
> matched on (!) then you get an exception--which is much better than doing weird
> unknown stuff.

Actually I would prefer an error here as well. Imagine a successful ‘guix system init’ silently creating a bootTODO.efi. Booting that system will certainly fail and someone will have a hard time to figure out that the generated bootTODO.efi is the reason.

>> +         (efi-bootloader (string-append (match system
>> +                                          (("i686" _ ...) "i386-efi")
>> +                                          (("x86_64" _ ...) "x86_64-efi")
>> +                                          (("arm" _ ...) "arm-efi")
>> +                                          (("armhf" _ ...) "arm-efi")
>> +                                          (("aarch64" _ ...) "arm64-efi")
>> +                                          (("riscv" _ ...) "riscv32-efi")
>> +                                          (("riscv64" _ ...) "riscv64-efi"))
>> +                                        "/core.efi")))
> 
> With the fall-through case here, can this be changed to
> (("i686" _ ...) "i386-efi")
> (("aarch64" _ ...) "arm64-efi")
> (("riscv" _ ...) "riscv32-efi")
> ((_ ...) (string-append (first
>                         (string-split
>                           (nix-system->gnu-triplet
>                             (or (%current-system)
>                                 (%current-target-system)))
>                           #\_))
>                        "-efi"))

There was a contradicting remark by Danny, which applies to the first part as well:

> Also, I have a slight preference for greppable file names even when it's a
> little more redundant

I understand your point in generating the arch part. I also understand the usage of (nix-system->gnu-triplet) to convert armhf to arm. I also think the risk is low that the first part raises no error and this part is doing something wrong without raising an error, too, leading to a not booting system. So having a default here seems fine to me.

However, Danny’s argument is convincing, too. And keeping this block similar to the first block eases the understanding, at least mine. My first thought seeing your suggestion was: “Why does this block need to be different from the other and what is this code doing differently?”

What do you think?


Bye

Stefan

Hello Stefan,

I have reviewed this new patch and it looks good to me.

There's one thing, though: Where does this new bootloader get used?

I think--from reading the source code and from your previous comments--that the
bootloader gets installed on the TFTP server, but as files to be served, not to
boot the TFTP server itself.  One is basically using "guix system init" to
initialize an operating system, complete with bootloader--only it is served
by an TFTP server instead of booted locally.

Is that correct?  If so, I think we should document that fact--because that's
absolutely not obvious to a casual observer.

Also, the same files also need to be exported by an NFS server on the same
host as the TFTP server (which is a limitation.  I understand the rationale,
but the limitation has to be documented).  This should be documented not in
the bootloader (which does not require NFS at all, right?), but in
doc/guix.texi--where ideally we'd document an example system configuration
for it.

I'll leave a week for comments and then will merge either this version or an
eventual newer version to guix master.

Thanks.

On Tue, Sep 15, 2020 at 10:28:53PM +0200, Stefan wrote:
> Hi Efraim!
> 
> >> +         (boot-efi-link (match system
> >> +                          (("i686" _ ...) "/bootia32.efi")
> >> +                          (("x86_64" _ ...) "/bootx64.efi")
> >> +                          (("arm" _ ...) "/bootarm.efi")
> >> +                          (("armhf" _ ...) "/bootarm.efi")
> >> +                          (("aarch64" _ ...) "/bootaa64.efi")
> >> +                          (("riscv" _ ...) "/bootriscv32.efi")
> >> +                          (("riscv64" _ ...) "/bootriscv64.efi")))
> > 
> > Don't forget the fall-through case, even if it's just
> > ((_ ...) "/bootTODO.efi")
> 
> There was a contradicting remark by Danny:
> 
> > The major advantage of using "match" is its failure mode.  If the thing matched
> > on is not a list (for some unfathomable reason) or if the first element is not
> > matched on (!) then you get an exception--which is much better than doing weird
> > unknown stuff.
> 
> Actually I would prefer an error here as well. Imagine a successful ‘guix system init’ silently creating a bootTODO.efi. Booting that system will certainly fail and someone will have a hard time to figure out that the generated bootTODO.efi is the reason.
> 

My concern is more for architectures which aren't on the list having
unfortunate errors while doing something unrelated. Another option then
I suppose would be
((_ ...) #f)
It should still fail if you try to use it but there's still a code path
for, say, ppc64el. I like the #f idea better than bootTODO.efi.

> >> +         (efi-bootloader (string-append (match system
> >> +                                          (("i686" _ ...) "i386-efi")
> >> +                                          (("x86_64" _ ...) "x86_64-efi")
> >> +                                          (("arm" _ ...) "arm-efi")
> >> +                                          (("armhf" _ ...) "arm-efi")
> >> +                                          (("aarch64" _ ...) "arm64-efi")
> >> +                                          (("riscv" _ ...) "riscv32-efi")
> >> +                                          (("riscv64" _ ...) "riscv64-efi"))
> >> +                                        "/core.efi")))
> > 
> > With the fall-through case here, can this be changed to
> > (("i686" _ ...) "i386-efi")
> > (("aarch64" _ ...) "arm64-efi")
> > (("riscv" _ ...) "riscv32-efi")
> > ((_ ...) (string-append (first
> >                           (string-split
> >                             (nix-system->gnu-triplet
> >                               (or (%current-system)
> >                                   (%current-target-system)))
> >                           #\_))
> >                         "-efi"))

I re-noticed that system is passed above so my code block could be a bit
more contained

((_ ...) (string-append (first
                          (string-split
                            (nix-system->gnu-triplet system)
                          #\_))
                        "-efi"))

> 
> There was a contradicting remark by Danny, which applies to the first part as well:
> 
> > Also, I have a slight preference for greppable file names even when it's a
> > little more redundant
> 
> I understand your point in generating the arch part. I also understand the usage of (nix-system->gnu-triplet) to convert armhf to arm. I also think the risk is low that the first part raises no error and this part is doing something wrong without raising an error, too, leading to a not booting system. So having a default here seems fine to me.
> 
> However, Danny’s argument is convincing, too. And keeping this block similar to the first block eases the understanding, at least mine. My first thought seeing your suggestion was: “Why does this block need to be different from the other and what is this code doing differently?”
> 
> What do you think?
> 

The benefit is that there's less chance of typoing a mistake in the
code, either when writing it or when editing it later. There's also the
benefit again of not dismissing architectures which are currently not
listed in the list.

For something greppable, I don't really have a counter argument. Perhaps
a hand-wavey "code correctness" of reusing macros.

For unsupported architectures, ((_ ...) #f) again would work to make
sure there is at least a code path which would definitely fail if
someone tried to use it. That's my primary concern.

> 
> Bye
> 
> Stefan
> 

Thanks