[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Reza Alizadeh Majd]

There are situations that u-boot doesn't have to load from the device
tree. some provide the device tree using a vendor bootloader (like what
raspberry-pi does) or with an external bootloader that chainloads the
u-boot (what Asahi does for m1n1 bootloader). 

Unfortunately we couldn't find any reliable document to enforce u-boot
to pass the device tree via `extlinux.conf`, however during our tests,
we found that removing the `FDTDIR` line from the `extlinux.conf` tend
us to do so.

There is also no reliable way to guess if u-boot bootloader should load
device tree or not on a specific hardware. in addition, there are
hardware that can be booted with both firmware device tree on some
kernels and with special device tree on other (modified) kernels. 

so we propose the following patch to define an optional parameter in
`bootloader` record, called `ignore-fdtdir?` which by default is set to
`#f` to keep the current behavior unchanged. if this paramter is set to
`#t`, the `FDTDIR` line will be discarded from the `extlinux.conf` and
u-boot doesn't load the device tree automatically.

This patch was tested on a Raspberry PI and in a VM. It is proven to
preserve the old behavior. We’d love to hear your feedback and hope we
can support more arm hardware in the future.


Regards,
Reza

-- 
Reza Alizadeh Majd
PantherX Team
https://pantherx.org

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Reza Alizadeh Majd]

* gnu/bootloader.scm (<bootloader>)[ignore-fdtdir?]: new field.
* gnu/bootloader/extlinux.scm (extlinux-configuration-file): add FDTDIR line based on bootloader <ignore-fdtdir?> field of <bootloader>.
---
 gnu/bootloader.scm          |  5 ++++-
 gnu/bootloader/extlinux.scm | 12 ++++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 9cf5457873..acf51bff7a 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -54,6 +54,7 @@ (define-module (gnu bootloader)
             bootloader-disk-image-installer
             bootloader-configuration-file
             bootloader-configuration-file-generator
+            bootloader-ignore-fdtdir?
 
             bootloader-configuration
             bootloader-configuration?
@@ -173,7 +174,9 @@ (define-record-type* <bootloader>
   (disk-image-installer            bootloader-disk-image-installer
                                    (default #f))
   (configuration-file              bootloader-configuration-file)
-  (configuration-file-generator    bootloader-configuration-file-generator))
+  (configuration-file-generator    bootloader-configuration-file-generator)
+  (ignore-fdtdir?                  bootloader-ignore-fdtdir?
+                                   (default #f)))
 
 \f
 ;;;
diff --git a/gnu/bootloader/extlinux.scm b/gnu/bootloader/extlinux.scm
index 6b5ff298e7..084ed1e7c9 100644
--- a/gnu/bootloader/extlinux.scm
+++ b/gnu/bootloader/extlinux.scm
@@ -38,6 +38,10 @@ (define* (extlinux-configuration-file config entries
   (define all-entries
     (append entries (bootloader-configuration-menu-entries config)))
 
+  (define ignore-fdtdir?
+    (let ((bootloader (bootloader-configuration-bootloader config)))
+      (bootloader-ignore-fdtdir? bootloader)))
+
   (define (menu-entry->gexp entry)
     (let ((label (menu-entry-label entry))
           (kernel (menu-entry-linux entry))
@@ -46,12 +50,16 @@ (define (menu-entry->gexp entry)
       #~(format port "LABEL ~a
   MENU LABEL ~a
   KERNEL ~a
-  FDTDIR ~a/lib/dtbs
+  ~a
   INITRD ~a
   APPEND ~a
 ~%"
                 #$label #$label
-                #$kernel (dirname #$kernel) #$initrd
+                #$kernel 
+                (if (not #$ignore-fdtdir?)
+                    (string-append "FDTDIR " (dirname #$kernel) "/lib/dtbs")
+                    "")
+                #$initrd
                 (string-join (list #$@kernel-arguments)))))
 
   (define builder
-- 
2.37.1

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Mathieu Othacehe]

Hello,

> * gnu/bootloader.scm (<bootloader>)[ignore-fdtdir?]: new field.

The bootloader record is supposed to be bootloader agnostic. This fdtdir
naming thing seems to be extlinux specific.

Should we maybe rename this field "device-tree-support?"

Thanks,

Mathieu

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Reza Alizadeh Majd]

[-- Attachment #1: Type: text/plain, Size: 160 bytes --]

It seems that I forgot to update some parts of the commit message, I
just submitted the edited one.


-- 
Reza Alizadeh Majd
PantherX Team
https://pantherx.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-bootloader-extlinux-support-for-optional-FDTDIR.patch --]
[-- Type: text/x-patch, Size: 9876 bytes --]

From eaa1c6db1e5a8a059f499c904861d8270194faf6 Mon Sep 17 00:00:00 2001
From: Reza Alizadeh Majd <r.majd@pantherx.org>
Date: Fri, 5 Aug 2022 20:00:42 +0430
Subject: [PATCH] bootloader: extlinux: support for optional FDTDIR

There are situations that u-boot doesn't have to load from the device tree.
some provide the device tree using a vendor bootloader (like what raspberry-pi
does) or with an external bootloader that chainloads the u-boot (what Asahi
does for m1n1 bootloader).

Unfortunately we couldn't find any reliable document to enforce u-boot to pass
the device tree via `extlinux.conf`, however during our tests, we found that
removing the `FDTDIR` line from the `extlinux.conf` tend us to do so.

There is also no reliable way to guess if u-boot bootloader should load device
tree or not on a specific hardware. in addition, there are hardware that can
be booted with both firmware device tree on some kernels and with special
device tree on other (modified) kernels.

the following changes provided to define an optional parameter in <bootloader>
record, called <device-tree-support?>  which by default is set to #t to keep
the current behavior unchanged. if this paramter is set to #f, the FDTDIR line
will be discarded from the <extlinux.conf> and u-boot doesn't load the device
tree automatically.

* gnu/bootloader.scm (<bootloader>)[device-tree-support?]: new field.
* gnu/bootloader/extlinux.scm (extlinux-configuration-file): add FDTDIR line
based on bootloader <device-tree-support?> field of <bootloader>.
* gnu/tests/bootloader.scm: add tests for FDTDIR modification.
* gnu/local.mk (GNU_SYSTEM_MODULES): Add gnu/tests/bootloader.scm
---
 gnu/bootloader.scm          |   6 +-
 gnu/bootloader/extlinux.scm |  13 +++-
 gnu/local.mk                |   2 +
 gnu/tests/bootloader.scm    | 119 ++++++++++++++++++++++++++++++++++++
 4 files changed, 137 insertions(+), 3 deletions(-)
 create mode 100644 gnu/tests/bootloader.scm

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 9cf5457873..3793cc981f 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2017 Leo Famulari <leo@famulari.name>
 ;;; Copyright © 2019, 2021 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -54,6 +55,7 @@ (define-module (gnu bootloader)
             bootloader-disk-image-installer
             bootloader-configuration-file
             bootloader-configuration-file-generator
+            bootloader-device-tree-support?
 
             bootloader-configuration
             bootloader-configuration?
@@ -173,7 +175,9 @@ (define-record-type* <bootloader>
   (disk-image-installer            bootloader-disk-image-installer
                                    (default #f))
   (configuration-file              bootloader-configuration-file)
-  (configuration-file-generator    bootloader-configuration-file-generator))
+  (configuration-file-generator    bootloader-configuration-file-generator)
+  (device-tree-support?            bootloader-device-tree-support?
+                                   (default #t)))
 
 \f
 ;;;
diff --git a/gnu/bootloader/extlinux.scm b/gnu/bootloader/extlinux.scm
index 6b5ff298e7..f3d69c0cc0 100644
--- a/gnu/bootloader/extlinux.scm
+++ b/gnu/bootloader/extlinux.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017 David Craven <david@craven.ch>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -38,6 +39,10 @@ (define* (extlinux-configuration-file config entries
   (define all-entries
     (append entries (bootloader-configuration-menu-entries config)))
 
+  (define with-fdtdir?
+    (let ((bootloader (bootloader-configuration-bootloader config)))
+      (bootloader-device-tree-support? bootloader)))
+
   (define (menu-entry->gexp entry)
     (let ((label (menu-entry-label entry))
           (kernel (menu-entry-linux entry))
@@ -46,12 +51,16 @@ (define (menu-entry->gexp entry)
       #~(format port "LABEL ~a
   MENU LABEL ~a
   KERNEL ~a
-  FDTDIR ~a/lib/dtbs
+  ~a
   INITRD ~a
   APPEND ~a
 ~%"
                 #$label #$label
-                #$kernel (dirname #$kernel) #$initrd
+                #$kernel
+                (if #$with-fdtdir?
+                    (string-append "FDTDIR " (dirname #$kernel) "/lib/dtbs")
+                    "")
+                #$initrd
                 (string-join (list #$@kernel-arguments)))))
 
   (define builder
diff --git a/gnu/local.mk b/gnu/local.mk
index 88100416d5..b430c664ea 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -51,6 +51,7 @@
 # Copyright © 2022 Remco van 't Veer <remco@remworks.net>
 # Copyright © 2022 Artyom V. Poptsov <poptsov.artyom@gmail.com>
 # Copyright © 2022 John Kehayias <john.kehayias@protonmail.com>
+# Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 #
 # This file is part of GNU Guix.
 #
@@ -736,6 +737,7 @@ GNU_SYSTEM_MODULES =				\
   %D%/tests.scm					\
   %D%/tests/audio.scm				\
   %D%/tests/base.scm				\
+  %D%/tests/bootloader.scm				\
   %D%/tests/ci.scm				\
   %D%/tests/cups.scm				\
   %D%/tests/databases.scm			\
diff --git a/gnu/tests/bootloader.scm b/gnu/tests/bootloader.scm
new file mode 100644
index 0000000000..0c3bffa58d
--- /dev/null
+++ b/gnu/tests/bootloader.scm
@@ -0,0 +1,119 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+
+(define-module (gnu tests bootloader)
+  #:use-module (gnu)
+  #:use-module (gnu bootloader u-boot)
+  #:use-module (gnu system vm)
+  #:use-module (gnu tests)
+  #:use-module (guix scripts system reconfigure)
+  #:export (%test-uboot-with-fdtdir
+            %test-uboot-without-fdtdir))
+
+
+(define %u-boot-with-fdtdir-bootloader
+  (bootloader
+   (inherit u-boot-bootloader)))
+
+
+(define %u-boot-without-fdtdir-bootloader
+  (bootloader
+   (inherit u-boot-bootloader)
+   (device-tree-support? #f)))
+
+
+(define (u-boot-os with-fdtdir?)
+  (operating-system
+    (inherit %simple-os)
+    (bootloader (bootloader-configuration
+                 (bootloader (if with-fdtdir?
+                                 %u-boot-with-fdtdir-bootloader
+                                 %u-boot-without-fdtdir-bootloader))))))
+
+
+(define* (run-uboot-fdtdir-test name #:key (with-fdtdir? #t))
+  "Run u-boot-bootloader installation with/without FDTDIR record for
+extlinux.conf"
+
+  (define os
+    (marionette-operating-system
+     (u-boot-os with-fdtdir?)))
+
+  (define vm (virtual-machine
+              (operating-system os)
+              (volatile? #f)))
+
+  (define (test script)
+    (with-imported-modules '((gnu build marionette))
+      #~(begin
+          (use-modules (gnu build marionette)
+                       (srfi srfi-64))
+
+          (define marionette
+            (make-marionette (list #$vm)))
+
+          (test-runner-current (system-test-runner #$output))
+          (test-begin #$name)
+
+          (test-assert "bootloader installed"
+            (marionette-eval
+             '(primitive-load #$script)
+             marionette))
+
+          (test-assert "extlinux.conf file created"
+            (marionette-eval
+             '(file-exists? "/boot/extlinux/extlinux.conf")
+             marionette))
+
+          (let ((content (wait-for-file "/boot/extlinux/extlinux.conf" marionette
+                                        #:read 'get-string-all
+                                        #:timeout 30)))
+            (if #$with-fdtdir?
+                (test-assert "FDTDIR exists"
+                  (string-contains content "FDTDIR"))
+                (test-assert "FDTDIR removed"
+                  (not (string-contains content "FDTDIR")))))
+
+          (test-end #$name))))
+
+  (let* ((bootcfg (operating-system-bootcfg os '()))
+         (bootloader ((compose bootloader-configuration-bootloader
+                               operating-system-bootloader) os))
+         (bootcfg-file (bootloader-configuration-file bootloader)))
+    (gexp->derivation "uboot"
+      (test (install-bootloader-program #f #f #f bootcfg bootcfg-file
+                                        '(#f) "/")))))
+
+
+(define %test-uboot-with-fdtdir
+  (system-test
+   (name "uboot-with-fdtdir")
+   (description "test uboot installation with fdtdir")
+   (value
+    (run-uboot-fdtdir-test "uboot-with-fdtdir"
+                           #:with-fdtdir? #t))))
+
+
+(define %test-uboot-without-fdtdir
+  (system-test
+   (name "uboot-without-fdtdir")
+   (description "test uboot installation without fdtdir")
+   (value
+    (run-uboot-fdtdir-test "uboot-without-fdtdir"
+                           #:with-fdtdir? #f))))
-- 
2.37.1

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Mathieu Othacehe]

Hey,

OK, so I read the long thread. There's still something unclear to me. If
FDTDIR is not passed, where is located the device tree that is loaded?
Directly in the /boot partition?

It looks like the corresponding code is in the label_boot function of
the u-boot/boot/pxe_utils.c file. I quickly read this part hoping to
find a way to define a priority between device trees located in the
FDTDIR directory and device trees installed elsewhere.

> * gnu/bootloader.scm (<bootloader>)[device-tree-support?]: new field.

You need to document this new field in the "Bootloader Configuration"
documentation section.

> * gnu/tests/bootloader.scm: add tests for FDTDIR modification.

This will test for regressions on an x86_64-linux machine that will
probably never use this FDTDIR thing. As those tests are expensive to
run an maintain we can probably remove the test.

Thanks,

Mathieu

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Reza Alizadeh Majd]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

Hi Mathieu,


On Thu, 25 Aug 2022 19:35:45 +0200
Mathieu Othacehe <othacehe@gnu.org> wrote:

>
>> * gnu/bootloader.scm (<bootloader>)[device-tree-support?]: new
>> field.  
>
>You need to document this new field in the "Bootloader Configuration"
>documentation section.
>

I just checked the "Bootloader Configuration" section. it describes the
"bootloader-configuration" record itself, but the proposed patch adds
the "device-tree-supports?" field to the "bootloader" record. 

unfortunately I couldn't find the section describing the "bootloader"
record fields. so I added the documentations as a note for the
"bootloader" field of "bootloader-configuration" record. 


>> * gnu/tests/bootloader.scm: add tests for FDTDIR modification.  
>
>This will test for regressions on an x86_64-linux machine that will
>probably never use this FDTDIR thing. As those tests are expensive to
>run an maintain we can probably remove the test.
>

OK, I removed the test from recent patch. 


Best,
Reza

-- 
Reza Alizadeh Majd
PantherX Team
https://pantherx.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-bootloader-extlinux-support-for-optional-FDTDIR.patch --]
[-- Type: text/x-patch, Size: 5716 bytes --]

From cbeba372def25a957f5d8942f01fbde6cdc03704 Mon Sep 17 00:00:00 2001
From: Reza Alizadeh Majd <r.majd@pantherx.org>
Date: Sun, 28 Aug 2022 12:34:46 +0430
Subject: [PATCH] bootloader: extlinux: support for optional FDTDIR

There are situations that u-boot doesn't have to load from the device tree.
some provide the device tree using a vendor bootloader (like what raspberry-pi
does) or with an external bootloader that chainloads the u-boot (what Asahi
does for m1n1 bootloader).

Unfortunately we couldn't find any reliable document to enforce u-boot to pass
the device tree via `extlinux.conf`, however during our tests, we found that
removing the `FDTDIR` line from the `extlinux.conf` tend us to do so.

There is also no reliable way to guess if u-boot bootloader should load device
tree or not on a specific hardware. in addition, there are hardware that can
be booted with both firmware device tree on some kernels and with special
device tree on other (modified) kernels.

the following changes provided to define an optional parameter in <bootloader>
record, called <device-tree-support?>  which by default is set to #t to keep
the current behavior unchanged. if this paramter is set to #f, the FDTDIR line
will be discarded from the <extlinux.conf> and u-boot doesn't load the device
tree automatically.

* gnu/bootloader.scm (<bootloader>)[device-tree-support?]: new field.
* gnu/bootloader/extlinux.scm (extlinux-configuration-file): add FDTDIR line
based on bootloader <device-tree-support?> field of <bootloader>.
* doc/guix.texi (Bootloader Configuration)[bootloader]: Add note about
bootloader's optional FDTDIR support.
---
 doc/guix.texi               |  9 +++++++++
 gnu/bootloader.scm          |  6 +++++-
 gnu/bootloader/extlinux.scm | 13 +++++++++++--
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 023b48ae35..25b336e958 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -36814,6 +36814,15 @@ modules.  In particular, @code{(gnu bootloader u-boot)} contains definitions
 of bootloaders for a wide range of ARM and AArch64 systems, using the
 @uref{https://www.denx.de/wiki/U-Boot/, U-Boot bootloader}.
 
+@quotation Note
+There are situations when @code{u-boot} shouldn't load the device tree from
+the file system. because it is either unnecessary for the platform being used
+or has already been loaded into RAM earlier in the boot process. in such cases
+you can set the @code{device-tree-support?} field of the @code{bootloader}
+record to @code{#f}. setting this flag to @code{#f} removes the @code{FDTDIR}
+from the @file{/boot/extlinux/extlinux.conf}.
+@end quotation
+
 @vindex grub-efi-bootloader
 @code{grub-efi-bootloader} allows to boot on modern systems using the
 @dfn{Unified Extensible Firmware Interface} (UEFI).  This is what you should
diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 70e1836179..32fd7f0c2e 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2017 Leo Famulari <leo@famulari.name>
 ;;; Copyright © 2019, 2021 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -54,6 +55,7 @@ (define-module (gnu bootloader)
             bootloader-disk-image-installer
             bootloader-configuration-file
             bootloader-configuration-file-generator
+            bootloader-device-tree-support?
 
             bootloader-configuration
             bootloader-configuration?
@@ -172,7 +174,9 @@ (define-record-type* <bootloader>
   (disk-image-installer            bootloader-disk-image-installer
                                    (default #f))
   (configuration-file              bootloader-configuration-file)
-  (configuration-file-generator    bootloader-configuration-file-generator))
+  (configuration-file-generator    bootloader-configuration-file-generator)
+  (device-tree-support?            bootloader-device-tree-support?
+                                   (default #t)))
 
 \f
 ;;;
diff --git a/gnu/bootloader/extlinux.scm b/gnu/bootloader/extlinux.scm
index 6b5ff298e7..f3d69c0cc0 100644
--- a/gnu/bootloader/extlinux.scm
+++ b/gnu/bootloader/extlinux.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017 David Craven <david@craven.ch>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -38,6 +39,10 @@ (define* (extlinux-configuration-file config entries
   (define all-entries
     (append entries (bootloader-configuration-menu-entries config)))
 
+  (define with-fdtdir?
+    (let ((bootloader (bootloader-configuration-bootloader config)))
+      (bootloader-device-tree-support? bootloader)))
+
   (define (menu-entry->gexp entry)
     (let ((label (menu-entry-label entry))
           (kernel (menu-entry-linux entry))
@@ -46,12 +51,16 @@ (define (menu-entry->gexp entry)
       #~(format port "LABEL ~a
   MENU LABEL ~a
   KERNEL ~a
-  FDTDIR ~a/lib/dtbs
+  ~a
   INITRD ~a
   APPEND ~a
 ~%"
                 #$label #$label
-                #$kernel (dirname #$kernel) #$initrd
+                #$kernel
+                (if #$with-fdtdir?
+                    (string-append "FDTDIR " (dirname #$kernel) "/lib/dtbs")
+                    "")
+                #$initrd
                 (string-join (list #$@kernel-arguments)))))
 
   (define builder
-- 
2.37.1

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Mathieu Othacehe]

Hey Reza,

Thanks for the updated version!

> I just checked the "Bootloader Configuration" section. it describes the
> "bootloader-configuration" record itself, but the proposed patch adds
> the "device-tree-supports?" field to the "bootloader" record. 

About that, any reason not to have this "device-tree-supports?" field in
the <bootloader-configuration> record?

The <bootloader> record is about how to install the bootloader while
<bootloader-configuration> is about its configuration. So maybe it would
be a better fit?

> OK, I removed the test from recent patch. 

Good. Let me know what you think about the proposal and we should be
good to proceed.

Mathieu

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Reza Alizadeh Majd]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

Hi Mathieu,


On Sun, 28 Aug 2022 17:49:36 +0200
Mathieu Othacehe <othacehe@gnu.org> wrote:

>
>About that, any reason not to have this "device-tree-supports?" field
>in the <bootloader-configuration> record?
>
>The <bootloader> record is about how to install the bootloader while
><bootloader-configuration> is about its configuration. So maybe it
>would be a better fit?
>

I wanted to limit my patch to affect as minimum sections as possible.
so I added the field to the <bootloader> record. 

I'm agree with your proposal, since the removal of FDTDIR is more of a
configuration for an existing bootloader.

I moved this option to the <bootloader-configuration> and submit a new
patch. it would be great to have your feedback whenever you had time. 


Regards, 
Reza

-- 
Reza Alizadeh Majd
PantherX Team
https://pantherx.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-bootloader-extlinux-support-for-optional-FDTDIR.patch --]
[-- Type: text/x-patch, Size: 8353 bytes --]

From a7e3e9afe22274397634e6dbe4caa9766adfb7a9 Mon Sep 17 00:00:00 2001
From: Reza Alizadeh Majd <r.majd@pantherx.org>
Date: Sun, 28 Aug 2022 12:34:46 +0430
Subject: [PATCH] bootloader: extlinux: support for optional FDTDIR

There are situations that u-boot doesn't have to load from the device tree.
some provide the device tree using a vendor bootloader (like what raspberry-pi
does) or with an external bootloader that chainloads the u-boot (what Asahi
does for m1n1 bootloader).

Unfortunately we couldn't find any reliable document to enforce u-boot to pass
the device tree via `extlinux.conf`, however during our tests, we found that
removing the `FDTDIR` line from the `extlinux.conf` tend us to do so.

There is also no reliable way to guess if u-boot bootloader should load device
tree or not on a specific hardware. in addition, there are hardware that can
be booted with both firmware device tree on some kernels and with special
device tree on other (modified) kernels.

the following changes provided to define an optional parameter in
<bootloader-configuration> record, called <device-tree-support?>  which by
default is set to #t to keep the current behavior unchanged. if this paramter
is set to #f, the FDTDIR line will be discarded from the <extlinux.conf> and
u-boot doesn't load the device tree automatically.

* gnu/bootloader.scm (<bootloader-configuration>)[device-tree-support?]: new field.
* gnu/bootloader/extlinux.scm (extlinux-configuration-file): add FDTDIR line
based on <device-tree-support?> field of <bootloader-configuration>.
* doc/guix.texi (Bootloader Configuration)[device-tree-support?]: Add
documentation for the new field.
---
 doc/guix.texi               |  7 ++++++
 gnu/bootloader.scm          | 50 ++++++++++++++++++++-----------------
 gnu/bootloader/extlinux.scm | 12 +++++++--
 3 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 023b48ae35..8171481040 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -36960,6 +36960,13 @@ corresponds to COM1 (@pxref{Serial terminal,,, grub,GNU GRUB manual}).
 The speed of the serial interface, as an integer.  For GRUB, the
 default value is chosen at run-time; currently GRUB chooses
 9600@tie{}bps (@pxref{Serial terminal,,, grub,GNU GRUB manual}).
+
+@item @code{device-tree-support} (default: @code{#t})
+There are situations when @code{u-boot} shouldn't load the device tree from
+the file system. because it is either unnecessary for the platform being used
+or has already been loaded into RAM earlier in the boot process. in such cases
+you can set this field to @code{#f}. setting this flag to @code{#f} removes
+the @code{FDTDIR} line from the @file{/boot/extlinux/extlinux.conf}.
 @end table
 
 @end deftp
diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 70e1836179..335133d224 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2017 Leo Famulari <leo@famulari.name>
 ;;; Copyright © 2019, 2021 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -69,6 +70,7 @@ (define-module (gnu bootloader)
             bootloader-configuration-terminal-inputs
             bootloader-configuration-serial-unit
             bootloader-configuration-serial-speed
+            bootloader-configuration-device-tree-support?
 
             %bootloaders
             lookup-bootloader-by-name
@@ -193,29 +195,31 @@ (define-with-syntax-properties (warn-target-field-deprecation
 (define-record-type* <bootloader-configuration>
   bootloader-configuration make-bootloader-configuration
   bootloader-configuration?
-  (bootloader         bootloader-configuration-bootloader) ;<bootloader>
-  (targets            %bootloader-configuration-targets    ;list of strings
-                      (default #f))
-  (target             %bootloader-configuration-target ;deprecated
-                      (default #f) (sanitize warn-target-field-deprecation))
-  (menu-entries       bootloader-configuration-menu-entries ;list of <menu-entry>
-                      (default '()))
-  (default-entry      bootloader-configuration-default-entry ;integer
-                      (default 0))
-  (timeout            bootloader-configuration-timeout ;seconds as integer
-                      (default 5))
-  (keyboard-layout    bootloader-configuration-keyboard-layout ;<keyboard-layout> | #f
-                      (default #f))
-  (theme              bootloader-configuration-theme ;bootloader-specific theme
-                      (default #f))
-  (terminal-outputs   bootloader-configuration-terminal-outputs ;list of symbols
-                      (default '(gfxterm)))
-  (terminal-inputs    bootloader-configuration-terminal-inputs ;list of symbols
-                      (default '()))
-  (serial-unit        bootloader-configuration-serial-unit ;integer | #f
-                      (default #f))
-  (serial-speed       bootloader-configuration-serial-speed ;integer | #f
-                      (default #f)))
+  (bootloader           bootloader-configuration-bootloader) ;<bootloader>
+  (targets              %bootloader-configuration-targets    ;list of strings
+                        (default #f))
+  (target               %bootloader-configuration-target ;deprecated
+                        (default #f) (sanitize warn-target-field-deprecation))
+  (menu-entries         bootloader-configuration-menu-entries ;list of <menu-entry>
+                        (default '()))
+  (default-entry        bootloader-configuration-default-entry ;integer
+                        (default 0))
+  (timeout              bootloader-configuration-timeout ;seconds as integer
+                        (default 5))
+  (keyboard-layout      bootloader-configuration-keyboard-layout ;<keyboard-layout> | #f
+                        (default #f))
+  (theme                bootloader-configuration-theme ;bootloader-specific theme
+                        (default #f))
+  (terminal-outputs     bootloader-configuration-terminal-outputs ;list of symbols
+                        (default '(gfxterm)))
+  (terminal-inputs      bootloader-configuration-terminal-inputs ;list of symbols
+                        (default '()))
+  (serial-unit          bootloader-configuration-serial-unit ;integer | #f
+                        (default #f))
+  (serial-speed         bootloader-configuration-serial-speed ;integer | #f
+                        (default #f))
+  (device-tree-support? bootloader-configuration-device-tree-support?
+                        (default #t)))
 
 (define-deprecated (bootloader-configuration-target config)
   bootloader-configuration-targets
diff --git a/gnu/bootloader/extlinux.scm b/gnu/bootloader/extlinux.scm
index 6b5ff298e7..d9b6d8bf8a 100644
--- a/gnu/bootloader/extlinux.scm
+++ b/gnu/bootloader/extlinux.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017 David Craven <david@craven.ch>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -38,6 +39,9 @@ (define* (extlinux-configuration-file config entries
   (define all-entries
     (append entries (bootloader-configuration-menu-entries config)))
 
+  (define with-fdtdir?
+    (bootloader-configuration-device-tree-support? config))
+
   (define (menu-entry->gexp entry)
     (let ((label (menu-entry-label entry))
           (kernel (menu-entry-linux entry))
@@ -46,12 +50,16 @@ (define (menu-entry->gexp entry)
       #~(format port "LABEL ~a
   MENU LABEL ~a
   KERNEL ~a
-  FDTDIR ~a/lib/dtbs
+  ~a
   INITRD ~a
   APPEND ~a
 ~%"
                 #$label #$label
-                #$kernel (dirname #$kernel) #$initrd
+                #$kernel
+                (if #$with-fdtdir?
+                    (string-append "FDTDIR " (dirname #$kernel) "/lib/dtbs")
+                    "")
+                #$initrd
                 (string-join (list #$@kernel-arguments)))))
 
   (define builder
-- 
2.37.1

bug#57070: [PATCH] bootloader: extlinux: support for optional FDTDIR

[Mathieu Othacehe]

Hello Reza,

> * gnu/bootloader.scm (<bootloader-configuration>)[device-tree-support?]: new field.
> * gnu/bootloader/extlinux.scm (extlinux-configuration-file): add FDTDIR line
> based on <device-tree-support?> field of <bootloader-configuration>.
> * doc/guix.texi (Bootloader Configuration)[device-tree-support?]: Add
> documentation for the new field.

I pushed this patch with a few modifications.

Thanks,

Mathieu

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 919 bytes --]


On 09-08-2022 12:27, Reza Alizadeh Majd wrote:
> This patch was tested on a Raspberry PI and in a VM. It is proven to
> preserve the old behavior. We’d love to hear your feedback and hope we
> can support more arm hardware in the future.

PantherX claiming it's the upstream of Guix (see tooltip of 'operating 
system' on the home page), but that's incorrect, it's the other way around.

It is also implies that most of it is developed by PantherX people, but 
IIUC most of it is based on Guix System.

The website also hides well that it's mostly Guix (e.g.: on the home 
page it's only mentioned in the tooltip).

The About page mentions both "Less waste" and "other tech should follow 
the foot-steps [of Bitcoin]", this seems contradictory.

It also claims privacy, but the website includes stuff from an external 
service (dkkma.com), and apparently it's a tracker.

Greetings,
Maxime.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

Maxime Devos 写道：
> PantherX claiming it's the upstream of Guix (see tooltip of 
> 'operating
> system' on the home page), but that's incorrect, it's the other 
> way
> around.

For the record: they did when Maxime posted this, but at least 
that tooltip has since been corrected and now reads:

  ‘PantherX is a downstream distribution of GNU Guix and
   uses the Linux Kernel’

Keep on auditing everything,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 319 bytes --]


On 09-08-2022 12:27, Reza Alizadeh Majd wrote:
> This patch was tested on a Raspberry PI and in a VM. It is proven to
> preserve the old behavior. [...]

The VM thing sounds like a good system test, add it to gnu/tests/ so we 
can verify it and avoid breaking things with future changes.

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Reza Alizadeh Majd]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

Sorry for the late response, I was a little busy to apply the suggested
changes. 

On Wed, 10 Aug 2022 11:32:38 +0200
Maxime Devos <maximedevos@telenet.be> wrote:

>The VM thing sounds like a good system test, add it to gnu/tests/ so
>we can verify it and avoid breaking things with future changes.

I implemented the `gnu/tests/bootloader.scm` tests related to the
changes we are proposing.


also about other remarks you mentioned me directly, I applied them on
my recent patch. 

> Can FDTDIR be set automatically or unset automatically depending on
> the hardware? That would reduce the required configuration

No, that's not possible. As I mentioned in the initial message, some
hardware may or may not require it depending on the kernel.



On Mon, 15 Aug 2022 11:27:19 +0200
Mathieu Othacehe <othacehe@gnu.org> wrote:

>The bootloader record is supposed to be bootloader agnostic. This
>fdtdir naming thing seems to be extlinux specific.
>
>Should we maybe rename this field "device-tree-support?"

in the recent patch I renamed the `ignore-fdtdir?` parameter to the
`device-tree-support?`


Regards,
Reza

-- 
Reza Alizadeh Majd
PantherX Team
https://pantherx.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-bootloader-extlinux-support-for-optional-FDTDIR.patch --]
[-- Type: text/x-patch, Size: 9864 bytes --]

From 06edac43226f8ef4bd45c973cd47c1ccd9d7da7a Mon Sep 17 00:00:00 2001
From: Reza Alizadeh Majd <r.majd@pantherx.org>
Date: Fri, 5 Aug 2022 20:00:42 +0430
Subject: [PATCH] bootloader: extlinux: support for optional FDTDIR

There are situations that u-boot doesn't have to load from the device tree.
some provide the device tree using a vendor bootloader (like what raspberry-pi
does) or with an external bootloader that chainloads the u-boot (what Asahi
does for m1n1 bootloader).

Unfortunately we couldn't find any reliable document to enforce u-boot to pass
the device tree via `extlinux.conf`, however during our tests, we found that
removing the `FDTDIR` line from the `extlinux.conf` tend us to do so.

There is also no reliable way to guess if u-boot bootloader should load device
tree or not on a specific hardware. in addition, there are hardware that can
be booted with both firmware device tree on some kernels and with special
device tree on other (modified) kernels.

the following changes provided to define an optional parameter in <bootloader>
record, called <device-tree-support?>  which by default is set to #t to keep
the current behavior unchanged. if this paramter is set to #f, the FDTDIR line
will be discarded from the <extlinux.conf> and u-boot doesn't load the device
tree automatically.

* gnu/bootloader.scm (<bootloader>)[ignore-fdtdir?]: new field.
* gnu/bootloader/extlinux.scm (extlinux-configuration-file): add FDTDIR line
based on bootloader <ignore-fdtdir?> field of <bootloader>.
* gnu/tests/bootloader.scm: add tests for FDTDIR modification.
* gnu/local.mk (GNU_SYSTEM_MODULES): Add gnu/tests/bootloader.scm
---
 gnu/bootloader.scm          |   6 +-
 gnu/bootloader/extlinux.scm |  13 +++-
 gnu/local.mk                |   2 +
 gnu/tests/bootloader.scm    | 119 ++++++++++++++++++++++++++++++++++++
 4 files changed, 137 insertions(+), 3 deletions(-)
 create mode 100644 gnu/tests/bootloader.scm

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 9cf5457873..3793cc981f 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2017 Leo Famulari <leo@famulari.name>
 ;;; Copyright © 2019, 2021 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -54,6 +55,7 @@ (define-module (gnu bootloader)
             bootloader-disk-image-installer
             bootloader-configuration-file
             bootloader-configuration-file-generator
+            bootloader-device-tree-support?
 
             bootloader-configuration
             bootloader-configuration?
@@ -173,7 +175,9 @@ (define-record-type* <bootloader>
   (disk-image-installer            bootloader-disk-image-installer
                                    (default #f))
   (configuration-file              bootloader-configuration-file)
-  (configuration-file-generator    bootloader-configuration-file-generator))
+  (configuration-file-generator    bootloader-configuration-file-generator)
+  (device-tree-support?            bootloader-device-tree-support?
+                                   (default #t)))
 
 \f
 ;;;
diff --git a/gnu/bootloader/extlinux.scm b/gnu/bootloader/extlinux.scm
index 6b5ff298e7..f3d69c0cc0 100644
--- a/gnu/bootloader/extlinux.scm
+++ b/gnu/bootloader/extlinux.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2017 David Craven <david@craven.ch>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -38,6 +39,10 @@ (define* (extlinux-configuration-file config entries
   (define all-entries
     (append entries (bootloader-configuration-menu-entries config)))
 
+  (define with-fdtdir?
+    (let ((bootloader (bootloader-configuration-bootloader config)))
+      (bootloader-device-tree-support? bootloader)))
+
   (define (menu-entry->gexp entry)
     (let ((label (menu-entry-label entry))
           (kernel (menu-entry-linux entry))
@@ -46,12 +51,16 @@ (define (menu-entry->gexp entry)
       #~(format port "LABEL ~a
   MENU LABEL ~a
   KERNEL ~a
-  FDTDIR ~a/lib/dtbs
+  ~a
   INITRD ~a
   APPEND ~a
 ~%"
                 #$label #$label
-                #$kernel (dirname #$kernel) #$initrd
+                #$kernel
+                (if #$with-fdtdir?
+                    (string-append "FDTDIR " (dirname #$kernel) "/lib/dtbs")
+                    "")
+                #$initrd
                 (string-join (list #$@kernel-arguments)))))
 
   (define builder
diff --git a/gnu/local.mk b/gnu/local.mk
index 88100416d5..b430c664ea 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -51,6 +51,7 @@
 # Copyright © 2022 Remco van 't Veer <remco@remworks.net>
 # Copyright © 2022 Artyom V. Poptsov <poptsov.artyom@gmail.com>
 # Copyright © 2022 John Kehayias <john.kehayias@protonmail.com>
+# Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
 #
 # This file is part of GNU Guix.
 #
@@ -736,6 +737,7 @@ GNU_SYSTEM_MODULES =				\
   %D%/tests.scm					\
   %D%/tests/audio.scm				\
   %D%/tests/base.scm				\
+  %D%/tests/bootloader.scm				\
   %D%/tests/ci.scm				\
   %D%/tests/cups.scm				\
   %D%/tests/databases.scm			\
diff --git a/gnu/tests/bootloader.scm b/gnu/tests/bootloader.scm
new file mode 100644
index 0000000000..0c3bffa58d
--- /dev/null
+++ b/gnu/tests/bootloader.scm
@@ -0,0 +1,119 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2022 Reza Alizadeh Majd <r.majd@pantherx.org>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+
+(define-module (gnu tests bootloader)
+  #:use-module (gnu)
+  #:use-module (gnu bootloader u-boot)
+  #:use-module (gnu system vm)
+  #:use-module (gnu tests)
+  #:use-module (guix scripts system reconfigure)
+  #:export (%test-uboot-with-fdtdir
+            %test-uboot-without-fdtdir))
+
+
+(define %u-boot-with-fdtdir-bootloader
+  (bootloader
+   (inherit u-boot-bootloader)))
+
+
+(define %u-boot-without-fdtdir-bootloader
+  (bootloader
+   (inherit u-boot-bootloader)
+   (device-tree-support? #f)))
+
+
+(define (u-boot-os with-fdtdir?)
+  (operating-system
+    (inherit %simple-os)
+    (bootloader (bootloader-configuration
+                 (bootloader (if with-fdtdir?
+                                 %u-boot-with-fdtdir-bootloader
+                                 %u-boot-without-fdtdir-bootloader))))))
+
+
+(define* (run-uboot-fdtdir-test name #:key (with-fdtdir? #t))
+  "Run u-boot-bootloader installation with/without FDTDIR record for
+extlinux.conf"
+
+  (define os
+    (marionette-operating-system
+     (u-boot-os with-fdtdir?)))
+
+  (define vm (virtual-machine
+              (operating-system os)
+              (volatile? #f)))
+
+  (define (test script)
+    (with-imported-modules '((gnu build marionette))
+      #~(begin
+          (use-modules (gnu build marionette)
+                       (srfi srfi-64))
+
+          (define marionette
+            (make-marionette (list #$vm)))
+
+          (test-runner-current (system-test-runner #$output))
+          (test-begin #$name)
+
+          (test-assert "bootloader installed"
+            (marionette-eval
+             '(primitive-load #$script)
+             marionette))
+
+          (test-assert "extlinux.conf file created"
+            (marionette-eval
+             '(file-exists? "/boot/extlinux/extlinux.conf")
+             marionette))
+
+          (let ((content (wait-for-file "/boot/extlinux/extlinux.conf" marionette
+                                        #:read 'get-string-all
+                                        #:timeout 30)))
+            (if #$with-fdtdir?
+                (test-assert "FDTDIR exists"
+                  (string-contains content "FDTDIR"))
+                (test-assert "FDTDIR removed"
+                  (not (string-contains content "FDTDIR")))))
+
+          (test-end #$name))))
+
+  (let* ((bootcfg (operating-system-bootcfg os '()))
+         (bootloader ((compose bootloader-configuration-bootloader
+                               operating-system-bootloader) os))
+         (bootcfg-file (bootloader-configuration-file bootloader)))
+    (gexp->derivation "uboot"
+      (test (install-bootloader-program #f #f #f bootcfg bootcfg-file
+                                        '(#f) "/")))))
+
+
+(define %test-uboot-with-fdtdir
+  (system-test
+   (name "uboot-with-fdtdir")
+   (description "test uboot installation with fdtdir")
+   (value
+    (run-uboot-fdtdir-test "uboot-with-fdtdir"
+                           #:with-fdtdir? #t))))
+
+
+(define %test-uboot-without-fdtdir
+  (system-test
+   (name "uboot-without-fdtdir")
+   (description "test uboot installation without fdtdir")
+   (value
+    (run-uboot-fdtdir-test "uboot-without-fdtdir"
+                           #:with-fdtdir? #f))))
-- 
2.37.1

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1372 bytes --]


On 16-08-2022 19:08, Reza Alizadeh Majd wrote:
>> Can FDTDIR be set automatically or unset automatically depending on
>> the hardware? That would reduce the required configuration
> No, that's not possible. As I mentioned in the initial message, some
> hardware may or may not require it depending on the kernel.

My question has a part 'depending on the hardware', so possibly the 
relevant code could check what the hardware is.  Likewise, the code 
could check the kernel version.  More generally, when something can be 
decided manually, it can often be detected automatically with some 
work.  I'm not seeing any impossibility here.

Also, again, why are you submitting this work-around when it appears to 
be simply a kernel bug that needs a kernel package to be updated and 
maybe a devicetree fix to be backported? As written in a previous response:

> ‘There is also no reliable way to guess if u-boot bootloader should load
> device tree or not on a specific hardware. in addition, there are
> hardware that can be booted with both firmware device tree on some
> kernels and with special device tree on other (modified) kernels.’
>
> If I'm guessing correctly, that sounds like the problem is that device 
> tree information is missing from the kernel. Proposal: upstream the 
> device tree information.

Greetings,
Maxime.


[-- Attachment #1.1.1.2: Type: text/html, Size: 2069 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Pavel Shlyak]

Maxime, how is your information about pantherx related to the patch? This patch in question makes guix bootloader config more flexible with the perspective of supporting additional hardware. It doesn’t rely on any component of pantherx, thus I am not sure it’s the right place to discuss that.

As for the tests, I ran the image on a non-guix arm machine and I doubt it’s possible or makes sense to run a VM during tests phase. That would require much effort and probably hardware support for virtualization (that’s not present on many relevant machines). I believe a simple test like checking if the line is present in a newly generated extlinux.conf would have more sense.

Sincerely,
Pavel

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1285 bytes --]

On 10-08-2022 16:37, Pavel Shlyak wrote:

> Maxime, how is your information about pantherx related to the patch? This patch in question makes guix bootloader config more flexible with the perspective of supporting additional hardware. It doesn’t rely on any component of pantherx, thus I am not sure it’s the right place to discuss that.

The patch appears to be by PantherX (see: the @panterx.org mail address, 
the use of "we" and the corporate phrasing). The mail asks for feedback, 
so I give feedback. It appears that PantherX will interact with Guix in 
the future; PantherX making false or misleading claims makes me less 
interested in reviewing patches, so this seems important feedback to give.

I could of course out-of-the-blue send a mail to whatever PantherX main 
e-mail address is, but I'm not a customer of PantherX, so that would 
seem to be a pointless endeavor to me -- if it's anything like other 
companies, it will just be moved into the trash.  I believe that 
directly responding to the apparent employee of PantherX (and at the 
same time writing some more technical comments) has higher chances of 
success.

Summarised: this seems a good place to make such comments and I'm not 
aware of any better places.

Greetings,
Maxime.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Pavel Shlyak]

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

Hello, Maxime

Firstly, we always welcome feedback and we’re glad to answer your questions if you contact us directly. Here are the options: https://www.pantherx.org/contact/ <https://www.pantherx.org/contact/>. That might be a better place for it to avoid making other devs who’re subscribed to the mailing list read about things they’re not really into.
Secondly, we’ve already taken some steps to correct the issues you’ve mentioned on our website. I hope it will be clear and polished by the time we publish the first PantherX release. 
Finally, I’m personally glad and thankful that you’ve found time to inspect our website. Even I, as a member of the team, haven’t looked at it that closely :)

P. S. Reza’s working on the tests now. It will just take some time before he sends the following patch here.

Sincerely,
Pavel

[-- Attachment #2: Type: text/html, Size: 1871 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Pavel Shlyak]

P. S. Just to clarify: patch was written by Reza, I was the one who took part in testing it.

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Pavel Shlyak]

Dear Maxime,

I do not think I can agree that «a relevant code could check what the hardware is» as it’s not hardware-defined, but also user-defined. It’s not just about detection, it’s also about choice. The same system definition I have on RPI4, for example, can boot both with kernel-provided FDT (loaded with uboot) and bootloader-provided FDT (loaded with RPI bootloader). There are also a lot of different devices out there on the market and there’s no common way to know how OS definition is written for it. If you have ideas - you’re welcome to propose your way to do it.

It does not appear to «be simply a kernel bug» to me. Kernel does not pass configuration to the bootloader and we’re configuring a bootloader entry here. It’s simple as that: bootloader entry may include or not include that line and that’s user-defined.

Of course, you’re always welcome to suggest your ideas how the automation you like to have could be implemented. Also, please, keep in mind this change doesn’t change anything on current boards/systems and most users won’t even notice this change is merged.

Sincerely,
Pavel Shlyak

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 2183 bytes --]

On 20-08-2022 12:15, Pavel Shlyak wrote:

> I do not think I can agree that «a relevant code could check what the hardware is» as it’s not hardware-defined, but also user-defined. It’s not just about detection, it’s also about choice. The same system definition I have on RPI4, for example, can boot both with kernel-provided FDT (loaded with uboot) and bootloader-provided FDT (loaded with RPI bootloader).

If the user really wants to choose a different DT, they can customise 
their kernel by overriding the sourcce.

If the bootloader DT is more precise than the kernel DT (or the kernel 
DT is missing), why not submit the bootloader DT to the kernel? Then 
everyone can benefit, not only people using the 'RPI bootloader.' 
Between an inferior and a superior DT, I do not see the point of 
providing an option in Guix for selecting the inferior one.

Likewise, if they are equivalent, I don't see the point either.

You write that the system definition can both boot with the 
kernel-provided FDT and bootloader FDT, then why are you writing this 
patch if things work?

>   There are also a lot of different devices out there on the market and there’s no common way to know how OS definition is written for it. If you have ideas - you’re welcome to propose your way to do it.

The kernel has multiple DTs. I assume that, somehow, the kernel can 
figure out which one.

If you must go for this work-around, you could try porting the logic 
that the kernel uses to figure out the right DT, and extend it for the 
device that required the patch.

> It does not appear to «be simply a kernel bug» to me. Kernel does not pass configuration to the bootloader and we’re configuring a bootloader entry here. It’s simple as that: bootloader entry may include or not include that line and that’s user-defined.

AFAIK, device tree information is used by the kernel, not the 
bootloader. AFAIK, at most the bootloader passes a DT to the kernel.  We 
could just not support overriding the DT in Guix in the bootloader 
entry? I don't see the point if updating the DT in the kernel appears to 
be sufficient.

Greetings,
Maxime.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Pavel Shlyak]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

> If the user really wants to choose a different DT, they can customise their kernel by overriding the sourcce.
Yes, unless it’s generated by bootloader.
> why not submit the bootloader DT to the kernel?
Because it passes board-specific parameters. We cannot submit DTs for all board revisions, memory sizes etc.
> Likewise, if they are equivalent, I don't see the point either.
They are not
> You write that the system definition can both boot with the kernel-provided FDT and bootloader FDT, then why are you writing this patch if things work?
It can boot on RPI4b, but not on RPI3b+ or Compute Module 4
> The kernel has multiple DTs. I assume that, somehow, the kernel can figure out which one.
DTs are loaded by the bootloader. Kernel cannot figure anything out.
> If you must go for this work-around, you could try porting the logic that the kernel
No, kernel does not include this logic
> AFAIK, device tree information is used by the kernel, not the bootloader.
Uboot uses DT on some platforms
> I don't see the point if updating the DT in the kernel appears to be sufficient.
I hope dynamic DT with some data that only bootloader can know is sufficient for you. Again, this is how things work on Raspberry and some other boards on any distro. We don’t support that - we don’t support these devices. I personally don’t loose much as we can apply this patch directly on pantherx channel, making pantherx richer in device support. However, I do not quite like the idea of me answering «Install PantherX» to the people who cannot get GUIX on their devices.

I would be also happy if someone more competent on the topic joined this discussion. 


[-- Attachment #2: Type: text/html, Size: 3568 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 3717 bytes --]

On 22-08-2022 12:52, Pavel Shlyak wrote:

> > If the user really wants to choose a different DT, they can 
> customise their kernel by overriding the sourcce.
> Yes, unless it’s generated by bootloader.
Could you point me at the documentation or code that claims or does 
that? I am not finding any evidence that device trees are generated at boot.
> > why not submit the bootloader DT to the kernel?
> Because it passes board-specific parameters. We cannot submit DTs for 
> all board revisions, memory sizes etc.
If the bootloader can, surely the kernel can.
> > You write that the system definition can both boot with the 
> kernel-provided FDT and bootloader FDT, then why are you writing this 
> patch if things work?
> It can boot on RPI4b, but not on RPI3b+ or Compute Module 4
I believe the kernel folks will appreciate a patch fixing the DT for 
RPI3b+ and Compute Module 4.
> > The kernel has multiple DTs. I assume that, somehow, the kernel can 
> figure out which one.
> DTs are loaded by the bootloader. Kernel cannot figure anything out.

DTs are a kernel thing, e.g. the Linux documentation 
https://www.kernel.org/doc/html/latest/devicetree/usage-model.html 
mentions DT, also, Linux.  I could not find any information on 
bootloaders loading DTs.

> > If you must go for this work-around, you could try porting the logic 
> that the kernel
> No, kernel does not include this logic
The page 
https://www.kernel.org/doc/html/latest/devicetree/usage-model.html 
mentions various properties for specifying the model in the DT info, it 
has a section '2.2 Platform Identification' on how Linux decides which 
one is the right one. Plenty of logic there.
> > AFAIK, device tree information is used by the kernel, not the bootloader.
> Uboot uses DT on some platforms
OK. Is the board you are trying to support one of them, and does for 
that case the pre-patch behaviour suffice there (Linux will load its own 
DT later anyway?).

This response makes me wonder where the boot failed -- did it fail in 
the bootloader, or in the kernel startup?

> > I don't see the point if updating the DT in the kernel appears to be 
> sufficient.
> I hope dynamic DT with some data that only bootloader can know is 
> sufficient for you. 

It is neither sufficient nor insufficient for me -- it is you that is 
adding support for some boards, not me, GRUB+x86_64 works just nice here.

Besides, the bootloader/kernel distinction is just a matter of 
convention, bootloaders don't magically have access to more information 
than kernels. Anything a bootloader can determine, a kernel can as well, 
and vice-versa, they are both just software running on a CPU and various 
associated hardware.

> Again, this is how things work on Raspberry and some other boards on 
> any distro. We don’t support that - we don’t support these devices.

That's what I thought the patch was for -- adding support for some 
devices, turning the "it's not supported" into an "it's supported". 
Moving support from the bootloader to the kernel would accomplish that 
as well. Also, ad populum.

If you don't want to support new platforms, that's fine, but why are you 
sending a patch then?

> I personally don’t loose much as we can apply this patch directly on 
> pantherx channel, making pantherx richer in device support. However, I 
> do not quite like the idea of me answering «Install PantherX» to the 
> people who cannot get GUIX on their devices.

My point is that supporting more devices would be nice, but this patch 
isn't the way to do it.

Additionally, the proper capitalisation is Guix, GUIX is another thing.

Greetings,
Maxime.


[-- Attachment #1.1.1.2: Type: text/html, Size: 7301 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Pavel Shlyak]

> Could you point me at the documentation or code that claims or does that? I am not finding any evidence that device trees are generated at boot.

Google «device tree raspberry bootloader», 1st result https://forums.raspberrypi.com/viewtopic.php?t=329799

> If the bootloader can, surely the kernel can.

Bootloader runs on GPU on Raspberry. It cannot run kernel. 
Also, check m1n1 on Apple. It has docs.

> I believe the kernel folks will appreciate a patch fixing the DT for RPI3b+ and Compute Module 4.

And for other devices that behave the same way? You’re literally promoting making GUIX not bootable on all devices alike.

> DTs are a kernel thing, e.g. the Linux documentation https://www.kernel.org/doc/html/latest/devicetree/usage-model.html mentions DT, also, Linux.  I could not find any information on bootloaders loading DTs.

Because you didn’t search for it. Google «device tree raspberry bootloader», the first link is about bootloader forming the device tree https://forums.raspberrypi.com/viewtopic.php?t=329799. Google «uboot device tree» https://u-boot.readthedocs.io/en/latest/usage/fdt_overlays.html to know how uboot manipulates them.
Moreover, Raspberry PI uboot uses DTB to boot on the board as in https://patchwork.ozlabs.org/project/uboot/patch/20191106144104.28177-1-matthias.bgg@kernel.org/ (Instead of using the embedded DTB as done in RPi3 we use the devicetree provided by the firmware.)

> bootloaders don't magically have access to more information than kernels

They do, if they are run on a separate core on the SOC that linux or arm core has no access to.  Check https://github.com/christinaa/rpi-open-firmware

> My point is that supporting more devices would be nice, but this patch isn't the way to do it.

Well, there is no other way to support devices that require DTB not to be loaded with uboot. The solutions you suggest are not possible.

Moreover, keep in mind FDTDIR is not in the http://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/ specification and making is permanent we basically violate it. 

Since we’ve not come to any understanding here, I kindly invite Vagrant and Tobias to join the discussion. They seem to be familiar with the relevant parts of GUIX.

Sincerely,
Pavel

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 4909 bytes --]


On 22-08-2022 21:19, Pavel Shlyak wrote:
> Subject:
> Re: [bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR
> From:
> Pavel Shlyak <p.shlyak@pantherx.org>
> Date:
> 22-08-2022 21:19
>
> To:
> Maxime Devos <maximedevos@telenet.be>
> CC:
> 57070@debbugs.gnu.org, vagrant@debian.org, Tobias Geerinckx-Rice 
> <me@tobias.gr>
>
>
>> Could you point me at the documentation or code that claims or does that? I am not finding any evidence that device trees are generated at boot.
> Google «device tree raspberry bootloader», 1st resulthttps://forums.raspberrypi.com/viewtopic.php?t=329799

That web page does not claim that anywhere.

>> If the bootloader can, surely the kernel can.
> Bootloader runs on GPU on Raspberry. It cannot run kernel.
> Also, check m1n1 on Apple. It has docs.

I have never claimed that the GPU can run the kernel.

What does m1n1 have to do with anything here? m1n1 isn't extlinux and 
isn't packaged in Guix.

Bootloaders running on the GPU is something I'm not used to at all, it's 
not something I had expected, see later.

>> I believe the kernel folks will appreciate a patch fixing the DT for RPI3b+ and Compute Module 4.
> And for other devices that behave the same way? You’re literally promoting making GUIX not bootable on all devices alike.

I literally never wrote such a thing. In what sentences did I promote that?

Even if you meant 'implied' instead of 'literally', then that still 
doesn't make sense to me; I'm running Guix System, it's in my own 
interest to keep it bootable on my device.

>> DTs are a kernel thing, e.g. the Linux documentationhttps://www.kernel.org/doc/html/latest/devicetree/usage-model.html  mentions DT, also, Linux.  I could not find any information on bootloaders loading DTs.
> Because you didn’t search for it.
I did search for it, figuring out an _appropriate_ query and finding 
relevant results is another matter.
>   Google
No. It has monopoly and privacy problems.
>   «device tree raspberry bootloader», the first link is about bootloader forming the device treehttps://forums.raspberrypi.com/viewtopic.php?t=329799.
Generating the DT is a different matter from loading the DT.  It's also 
about firmware, not the bootloader.
>   Google «uboot device tree»https://u-boot.readthedocs.io/en/latest/usage/fdt_overlays.html  to know how uboot manipulates them.
These overlays look rather manual, to be done by the user for individual 
models, I don't see the relevancy.
> Moreover, Raspberry PI uboot uses DTB to boot on the board as inhttps://patchwork.ozlabs.org/project/uboot/patch/20191106144104.28177-1-matthias.bgg@kernel.org/  (Instead of using the embedded DTB as done in RPi3 we use the devicetree provided by the firmware.)
Going by the mention of 'defconfig' and 'arch/arm' and 'configs', this 
appears to be a patch to Linux, not uboot. As such, it appears that the 
device tree information is used by Linux here, there is no information 
there on whether it is used by U-Boot.
>> bootloaders don't magically have access to more information than kernels
> They do, if they are run on a separate core on the SOC that linux or arm core has no access to.  Checkhttps://github.com/christinaa/rpi-open-firmware

That's a setup I would not have expected.

AFAIK nothing is stopping Linux from sending some code to the separate 
core to figure out the relevant information and sending it back to 
Linux. But given the unusual setup, I would consider it plausible that 
Linux people want to delegate such responsibility to the bootloader.

Was this (i.e. "they are run on a separate core on the SOC ...") the 
case for the Raspberry device you are trying to support?

If so, figuring out the appropriate options to let U-boot pass the 
device tree to Linux seems reasonable to me.

>> My point is that supporting more devices would be nice, but this patch isn't the way to do it.
> Well, there is no other way to support devices that require DTB not to be loaded with uboot. The solutions you suggest are not possible.
>
> Moreover, keep in mind FDTDIR is not in thehttp://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/  specification and making is permanent we basically violate it.

And is this a bad thing, and if so, perhaps FDTDIR could be added to the 
specification? Guix being in violation of some specification is not in 
itself a bug, for example the store model of Guix is not 'Linux Standard 
Base'.

It might be a bad thing, but there's a step missing in your argument.

> Since we’ve not come to any understanding here, I kindly invite Vagrant and Tobias to join the discussion. They seem to be familiar with the relevant parts of GUIX.

See the 'If so, figuring out the appropriate options ...' above.

Also, again, it's Guix, not GUIX. GUIX is a Microsoft thing.

Greetings,
Maxime

[-- Attachment #1.1.1.2: Type: text/html, Size: 12394 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Pavel Shlyak]

[-- Attachment #1: Type: text/plain, Size: 3262 bytes --]

> That web page does not claim that anywhere.

Here's a rough list of the DT changes performed by the firmware, besides merging the obvious dtoverlays and dtparams:
* Applying the "upstream" overlay, if "upstream_kernel=1".
* Changing the CPU ID declarations on a Pi 2+ (BCM2837 vs BCM2836).
* Adding i2c_vc and i2c_arm labels and aliases (and their relatives).
* If the board has a POWER_LOW GPIO declared in dt-blob.bin, using that to configure the pwr_led node.
* Adding the Bluetooth flow control pins to the "uart0_pins" node, on boards that support it.
* Setting numerous items to /chosen - bootargs (the command line), rpi-boardrev-ext, rpi-country-code (Pi 400), details of the bootloader version, boot-mode, linux,initrd-start and linux,initrd-end.
* Adding a copy of the bootloader configuration.
* Loading the vl805 overlay on CM4s which have "VL805=1" in their bootloader configuration.
* Automatically loading overlays for supported cameras that are detected.
* Automatically loading overlays for the PoE HATs, switching between firmware-driven and I2C-driven as needed.
* Expanding the dma-ranges property of the emmc2bus node on BCM2711C0.
* Disabling the old OTG USB controller and enabling the XHCI controller if "otg_mode=1".
* Loading the appropriate rpi- display overlay.
* Setting the aliases "serial0" and "serial1" to point to the console/user UART and the Bluetooth/spare UART, respectively.
* Adding information about the HAT in "/hat".
* Copying any significant error messages to "/chosen/user-warning", from where it can be read by the GUI and turned into notifications.
* Declaring the available RAM.
* Passing the board revision, serial number, kaslr seed and rng seed.
* Limiting the size of the CMA region to 256MB if < 2GB RAM or gpu_mem > 256.
* Expanding the inbound window declared by the pcie0 dma-ranges property on a C0, moving the base address to 0x4_00000000 regardless.
* Setting the MDIO address of the Ethernet PHY.
* Declaring a simple-framebuffer, if wanted.

> What does m1n1 have to do with anything here? m1n1 isn't extlinux and isn't packaged in Guix.

It chainloads uboot

> Going by the mention of 'defconfig' and 'arch/arm' and 'configs', this appears to be a patch to Linux, not uboot. As such, it appears that the device tree information is used by Linux here, there is no information there on whether it is used by U-Boot.

I cannot agree.
https://github.com/u-boot/u-boot/blob/master/arch/arm/mach-bcm283x/Kconfig <https://github.com/u-boot/u-boot/blob/master/arch/arm/mach-bcm283x/Kconfig>
https://github.com/u-boot/u-boot/blob/master/configs/A10-OLinuXino-Lime_defconfig <https://github.com/u-boot/u-boot/blob/master/configs/A10-OLinuXino-Lime_defconfig>

> And is this a bad thing, and if so, perhaps FDTDIR could be added to the specification?

I don’t think so. Keep in mind the source code in Guix is named extlinux.scm and the file is named extlinux.conf and extlinux doesn’t support FDTDIR

> If so, figuring out the appropriate options to let U-boot pass the device tree to Linux seems reasonable to me.

Of course it passes device tree. The problem in question is the source of that device tree: in my case, it should not be loaded from a file.



[-- Attachment #2: Type: text/html, Size: 4898 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 4736 bytes --]

On 2022-08-22, Pavel Shlyak wrote:
>> Could you point me at the documentation or code that claims or does
>> that? I am not finding any evidence that device trees are generated
>> at boot.

I don't know exactly where in the code off the top of my head, but
u-boot definitely has support for modifying the device-tree it passes to
the kernel, and in some cases (e.g. memory layout, framebuffer)
*requires* modifying the device-tree that is passed to the kernel. FWIW,
I say this as someone who's maintained u-boot in debian (and to some
degree guix) for years...

Also, for some virtual platforms (qemu-riscv64?) the device-tree is
created on-the-fly when bootstrapping the virtual machine; in these
cases you have to rely on the device-tree passed via the boot "firmware"
as you shouldn't need to recompile the kernel just to pass a different
set of qemu arguments!

There are cases where the kernel cannot possibly track the combinatorial
explosion of potential add-on hardware modules (rpi hats, beagleboard
capes, etc.) that need to be represented in the device-tree in order to
work, sometimes before the kernel is even booted. There is some support
for device-tree overlays both in-kernel and in u-boot (and plausibly
other bootloaders) kernel to handle this, but in many cases, a simpler
and more reliable method is to provide a single custom device-tree.


> And for other devices that behave the same way? You’re literally
> promoting making GUIX not bootable on all devices alike.
>
>> DTs are a kernel thing, e.g. the Linux documentation
>> https://www.kernel.org/doc/html/latest/devicetree/usage-model.html
>> mentions DT, also, Linux.

Yes and no. Device-trees are used in, by and for both bootloaders and
kernels alike. There is long-standing debate around device-tree should
being treated as a boot firmware thing rather than a kernel thing. There
are some pretty simple questions:

You don't see ACPI tables for every supported x86 board in the kernel,
why should boards using device-tree be any different?

Why is the linux kernel the source of device-trees for, say, the Hurd,
*BSD, etc.?

So yes, it is de-facto where most device-trees come from at the end of
the day, but ... that doesn't necessarily mean it *should* be.


>> I could not find any information on bootloaders loading DTs.

Well, that's what the FDTDIR and FDT settings for extlinux do; it tells
the bootloader (e.g. u-boot) to load a device-tree. Grub also has a
devicetree option, though no correlary to FDTDIR. If grub isn't passed a
devicetree argument, it passes whatever devicetree that was passed from
the boot firmware, as I understand it.


>> My point is that supporting more devices would be nice, but this patch isn't the way to do it.
>
> Well, there is no other way to support devices that require DTB not to be loaded with uboot. The solutions you suggest are not possible.
>
> Moreover, keep in mind FDTDIR is not in the http://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/ specification and making is permanent we basically violate it. 

Hrm, that's unfortunate. I daresay supporting FDTDIR is a good thing, as
it allows you to use the same boot media to boot multiple different
devices, presuming they all have a .dtb present for the relevent boards.


Booting arm systems have come a long way from you need a specifically
compiled kernel for every different board, but it is still a horrible
mess.

Sometimes things are implemented in kernel, sometimes in the bootloader,
sometimes in the boot firmware, with possibly 3-4 different boot
firmware layers... and exactly where may be specific to a particular
board. There are arguments that any particular thing might be better
implemented at any of those layers, and maybe there is a convincing
reason to move something from one layer to another...

At the end of the day, the reality is that board support for some
platforms (e.g. arm* and possibly riscv*) is a mess of inconsistancy and
the usually you need to support multiple different ways of doing
seemingly the same things.

I think this tends to be hidden from view for x86 as quirks are just
worked around in the kernel, whereas with some platforms where more of
the boot firmware is free software it is possible to fix it in a more
appropriate layer... or whatever layer the someone just happens to get
it to work at first!


If adding an option to drop the FDTDIR extlinux configuration allows
booting more platforms, I see no fundamental reason why it is wrong, as
long as it doesn't break existing platforms... the implementation
details, I'll leave to people more savvy with scheme. :)


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Pavel Shlyak]

Hello, Mathieu
Thanks for joining us here!

Firstly, I kindly ask everyone in this thread to CC me as emails from this thread are not delivered to me for some reason I don’t really understand. I get emails from all other threads but this one.

Secondly, to reply you question "where is located the device tree that is loaded?", I would like to refer uboot source code comments, as I have not seen this point documented anywhere else. Check boot/pxe_utils.c

/*
	 * fdt usage is optional:
	 * It handles the following scenarios.
	 *
	 * Scenario 1: If fdt_addr_r specified and "fdt" or "fdtdir" label is
	 * defined in pxe file, retrieve fdt blob from server. Pass fdt_addr_r to
	 * bootm, and adjust argc appropriately.
	 *
	 * If retrieve fails and no exact fdt blob is specified in pxe file with
	 * "fdt" label, try Scenario 2.
	 *
	 * Scenario 2: If there is an fdt_addr specified, pass it along to
	 * bootm, and adjust argc appropriately.
	 *
	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
	 * bootm, and adjust argc appropriately.
	 *
	 * Scenario 4: fdt blob is not available.
	 */

In other words, if there’s no fdtdir, uboot just passes the fdt_addr to bootm command, that means it passes the pointer to device tree loaded by firmware (or previous bootloader) to the kernel. If fdt_addr is also missing, it passes fdtcontroladdr to the kernel. If all of the previous scenarios don’t work, it passes no FDT and kernel boots without device tree, that is absolutely normal on some devices, check x86.

As for the tests and documentation, I think I should leave these questions to Reza, as they have more experience and deeper understanding.

Sincerely,
Pavel

[bug#57070] [PATCH] bootloader: extlinux: support for optional FDTDIR

[Mathieu Othacehe]

Hey Pavel,

> In other words, if there’s no fdtdir, uboot just passes the fdt_addr to bootm
> command, that means it passes the pointer to device tree loaded by firmware
> (or previous bootloader) to the kernel. If fdt_addr is also missing, it passes
> fdtcontroladdr to the kernel. If all of the previous scenarios don’t work, it
> passes no FDT and kernel boots without device tree, that is absolutely normal
> on some devices, check x86.

Thanks for the clarification,

Mathieu