Bug 1751307

Hello Peter,

(In reply to Peter Robinson from comment #0)
> Add the ability to add GRUB_DEVICETREE to /etc/default/grub and if it's
> present (and only if) add the equivalent devicetree= line to the grub.cfg
> kernel entries.

Yes, this can be easily added. But I want to agree with you on the implementation. I think that there are two options on how this could work:

a) Make the blscfg module to add a "devicetree /boot/foo.img" command to the menu entries populated from the BLS files, if there's a devicetree grub2 environment variable set.

   So a user could do 'grub2-editenv - set devicetree="foo.dtb"' or 'echo GRUB_DEVICETREE=foo.dtb >> /etc/default/grub && grub2-mkconfig -o /boot/grub2/grub.cfg'. The grub2-mkconfig tool would set the devicetree env var if a GRUB_DEVICETREE has been set.

b) Make the blscfg module to parse a devicetree field from the BLS snippets. So a user could do 'echo devicetree /foo-bar.dtb >> /boot/loader/entries/bar.conf'

   In this case the /usr/lib/kernel/install.d/20-grub.install kernel-install plugin would append a devicetree field to the installed BLS snippet, but only if GRUB_DEVICETREE is set (or maybe an DEVICETREE=true in /etc/sysconfig/kernel to not pollute /etc/default/grub). The script could set this field to /boot/dtb-$KERNEL_VERSION.

The advantage of (a) is that the DTB can be set for all the BLS entries and it would even work for kernels that have already been installed. The disadvantage is that the BLS snippets won't be the source of truth anymore, there will be information that isn't present in the BLS file but used to populate the menu entry.

The advantage of (b) is that all the information about the menu entry will be contained in the BLS snippet, and also that it will allow to have a different DTB for each kernel. The disadvantage is that it won't work with already installed kernels (unless you re-install) because the devicetree field will be added on kernel-install time. The existing BLS snippets won't have that field.

I would prefer option (b), since as mentioned I think is better if the BLS file contains all the information about the menu entry. Also, the https://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/ file format already specifies a devicetree field. Finally, I think that will be safer to not use the same DTB for all the entries. Since even when the DTB binding should never break old kernels and be backward compatible, my experience is that sometimes they do and using a newer DTB with an old kernel could cause issues.

We could also do both, parse the devicetree field and if there isn't one fallback to a devicetree grub2 env var. If none is present, a devicetree command wont be added to the populated grub2 menu entry.

I just wanted to know your opinion on the implementation before writing the patch.

So the thing that concerns me about b) is that it seems it has to be done manually for each kernel installed. We have a single DT path/name per device so we don't need per kernel DTs. Ultimately I'd like to be able to set it once and have it on all kernels just like if you set any kernel command line parameters, it was the kernel cmdline logic in /etc/default/grub that made me think that maybe that was the best place. And example output we would need in the resulting grub config would be "devicetree=dtb/qcom/sdm845-db845c.dtb" (we don't want /boot in there as that's a linux mount point and not something grub cares about.

(In reply to Peter Robinson from comment #2)
> So the thing that concerns me about b) is that it seems it has to be done
> manually for each kernel installed. We have a single DT path/name per device
> so we don't need per kernel DTs. Ultimately I'd like to be able to set it

Ok, thanks for the clarification.

> once and have it on all kernels just like if you set any kernel command line
> parameters, it was the kernel cmdline logic in /etc/default/grub that made
> me think that maybe that was the best place. And example output we would
> need in the resulting grub config would be

Sure, let's do (a) then.

If you don't mind I would like to first lookup if there's a devicetree field in the BLS snippet though, to allow a user to override the default DT if needed (there won't be any tool that sets this field in the DTS though so in the default case it would use what's set as devicetree in grubenv through GRUB_DEVICETREE in /etc/default/grub).

> "devicetree=dtb/qcom/sdm845-db845c.dtb" (we don't want /boot in there as
> that's a linux mount point and not something grub cares about.

Right, I was just writing my thoughts and missed that detail.

> If you don't mind I would like to first lookup if there's a devicetree field
> in the BLS snippet though, to allow a user to override the default DT if
> needed (there won't be any tool that sets this field in the DTS though so in
> the default case it would use what's set as devicetree in grubenv through
> GRUB_DEVICETREE in /etc/default/grub).

By default we shouldn't need to set any devicetree by default. In just about all cases now where we use device tree the firmware loads/sets the device tree and passes a memory address to grub -> kernel. This is a hack to deal with a set of devices which have a horrible ACPI (Windows on Arm devices) table/hack and currently Linux has no means of supporting the non standard ACPI hack these devices use so we need to specify a device tree so they're usable in the short/medium term. In general this is a corner case, I was hoping to avoid it entirely, but I wasn't lucky enough :-/

We already had a GRUB_DEFAULT_DTB option for /etc/default/grub that's used in the non-BLS case, so I just reused this instead of adding a separate GRUB_DEVICETREE option.

Now this can also be used for a BLS configuration as you suggested, i.e:

$ echo "GRUB_DEFAULT_DTB=foo.dtb" >> /etc/default/grub

$ grub2-mkconfig -o /boot/efi/EFI/fedora/grub.cfg

The grub2-mkconfig tool will set a devicetree variable in grubenv:

$ grub2-editenv list | grep devicetree
devicetree=foo.dtb

And the populated menu entries from BLS snippets will add a devicetree command for grub2:

devicetree ($root)/foo.dtb

Setting a devicetree field in a BLS snippet will have a higher precedence as mentioned, so users could set different DT for each kernel if needed, i.e:

$ grep devicetree /boot/loader/entries/bar-kernel.conf
devicetree /bar.dtb

And the populated entry will pass this DTB instead of the one set in the devicetree variable:

devicetree ($root)/bar.dtb

Please let me know if anything else is needed, a scratch build for you to test can be found here:

https://koji.fedoraproject.org/koji/taskinfo?taskID=37669480