| Message ID | 20201023120825.30466-5-ard.biesheuvel@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux: implement LoadFile2 initrd loading | expand |
On Fri, Oct 23, 2020 at 14:08:25 +0200, Ard Biesheuvel wrote: > Now that we implemented supported for the LoadFile2 protocol for initrd > loading, there is no longer a need to pass the initrd parameters via > the device tree. This means there is no longer a reason to update the > device tree in the first place, and so we can ignore it entirely. There is a change in behaviour here which I don't think matters, but I'll call it out anyway: If there was ever a kernel out there with an EFI stub that depended on a chosen node existing in the DT, and the one provide by firmware did not contain one, that setup would break from this *if* it didn't use an initrd. / Leif > The only remaining reason to deal with the devicetree is if we are > using the 'devicetree' command to load one from disk, so tweak the > logic in grub_fdt_install() to take that into account. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > --- > grub-core/loader/arm64/linux.c | 22 ++++++++++---------- > grub-core/loader/efi/fdt.c | 7 +++++-- > 2 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c > index 285422c7bd43..9e282b6660fe 100644 > --- a/grub-core/loader/arm64/linux.c > +++ b/grub-core/loader/arm64/linux.c > @@ -93,21 +93,21 @@ finalize_params_linux (void) > > void *fdt; > > - fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE); > + /* Set initrd info */ > + if (initrd_start && initrd_end > initrd_start) > + { > + fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE); > > - if (!fdt) > - goto failure; > + if (!fdt) > + goto failure; > > - node = grub_fdt_find_subnode (fdt, 0, "chosen"); > - if (node < 0) > - node = grub_fdt_add_subnode (fdt, 0, "chosen"); > + node = grub_fdt_find_subnode (fdt, 0, "chosen"); > + if (node < 0) > + node = grub_fdt_add_subnode (fdt, 0, "chosen"); > > - if (node < 1) > - goto failure; > + if (node < 1) > + goto failure; > > - /* Set initrd info */ > - if (initrd_start && initrd_end > initrd_start) > - { > grub_dprintf ("linux", "Initrd @ %p-%p\n", > (void *) initrd_start, (void *) initrd_end); > > diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c > index ee9c5592c700..ab900b27d927 100644 > --- a/grub-core/loader/efi/fdt.c > +++ b/grub-core/loader/efi/fdt.c > @@ -85,13 +85,16 @@ grub_fdt_install (void) > grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID; > grub_efi_status_t status; > > + if (!fdt && !loaded_fdt) > + return GRUB_ERR_NONE; > + > b = grub_efi_system_table->boot_services; > - status = b->install_configuration_table (&fdt_guid, fdt); > + status = b->install_configuration_table (&fdt_guid, fdt ?: loaded_fdt); > if (status != GRUB_EFI_SUCCESS) > return grub_error (GRUB_ERR_IO, "failed to install FDT"); > > grub_dprintf ("fdt", "Installed/updated FDT configuration table @ %p\n", > - fdt); > + fdt ?: loaded_fdt); > return GRUB_ERR_NONE; > } > > -- > 2.17.1 >
On Fri, 23 Oct 2020 at 14:47, Leif Lindholm <leif@nuviainc.com> wrote: > > On Fri, Oct 23, 2020 at 14:08:25 +0200, Ard Biesheuvel wrote: > > Now that we implemented supported for the LoadFile2 protocol for initrd > > loading, there is no longer a need to pass the initrd parameters via > > the device tree. This means there is no longer a reason to update the > > device tree in the first place, and so we can ignore it entirely. > > There is a change in behaviour here which I don't think matters, but > I'll call it out anyway: > If there was ever a kernel out there with an EFI stub that depended on > a chosen node existing in the DT, and the one provide by firmware did > not contain one, that setup would break from this *if* it didn't use > an initrd. > I checked the Linux source, and the original code contributed by Roy already contained the logic to create the /chosen node if it wants there already. So we should be fine here.
On Fri, Oct 23, 2020 at 15:12:50 +0200, Ard Biesheuvel wrote: > On Fri, 23 Oct 2020 at 14:47, Leif Lindholm <leif@nuviainc.com> wrote: > > > > On Fri, Oct 23, 2020 at 14:08:25 +0200, Ard Biesheuvel wrote: > > > Now that we implemented supported for the LoadFile2 protocol for initrd > > > loading, there is no longer a need to pass the initrd parameters via > > > the device tree. This means there is no longer a reason to update the > > > device tree in the first place, and so we can ignore it entirely. > > > > There is a change in behaviour here which I don't think matters, but > > I'll call it out anyway: > > If there was ever a kernel out there with an EFI stub that depended on > > a chosen node existing in the DT, and the one provide by firmware did > > not contain one, that setup would break from this *if* it didn't use > > an initrd. > > I checked the Linux source, and the original code contributed by Roy > already contained the logic to create the /chosen node if it wants > there already. So we should be fine here. Excellent. Then, with this information now in a public archive for any unfortunate souls doing anything crazy non-linux to find: Reviewed-by: Leif Lindholm <leif@nuviainc.com>
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c index 285422c7bd43..9e282b6660fe 100644 --- a/grub-core/loader/arm64/linux.c +++ b/grub-core/loader/arm64/linux.c @@ -93,21 +93,21 @@ finalize_params_linux (void) void *fdt; - fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE); + /* Set initrd info */ + if (initrd_start && initrd_end > initrd_start) + { + fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE); - if (!fdt) - goto failure; + if (!fdt) + goto failure; - node = grub_fdt_find_subnode (fdt, 0, "chosen"); - if (node < 0) - node = grub_fdt_add_subnode (fdt, 0, "chosen"); + node = grub_fdt_find_subnode (fdt, 0, "chosen"); + if (node < 0) + node = grub_fdt_add_subnode (fdt, 0, "chosen"); - if (node < 1) - goto failure; + if (node < 1) + goto failure; - /* Set initrd info */ - if (initrd_start && initrd_end > initrd_start) - { grub_dprintf ("linux", "Initrd @ %p-%p\n", (void *) initrd_start, (void *) initrd_end); diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c index ee9c5592c700..ab900b27d927 100644 --- a/grub-core/loader/efi/fdt.c +++ b/grub-core/loader/efi/fdt.c @@ -85,13 +85,16 @@ grub_fdt_install (void) grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID; grub_efi_status_t status; + if (!fdt && !loaded_fdt) + return GRUB_ERR_NONE; + b = grub_efi_system_table->boot_services; - status = b->install_configuration_table (&fdt_guid, fdt); + status = b->install_configuration_table (&fdt_guid, fdt ?: loaded_fdt); if (status != GRUB_EFI_SUCCESS) return grub_error (GRUB_ERR_IO, "failed to install FDT"); grub_dprintf ("fdt", "Installed/updated FDT configuration table @ %p\n", - fdt); + fdt ?: loaded_fdt); return GRUB_ERR_NONE; }
Now that we implemented supported for the LoadFile2 protocol for initrd loading, there is no longer a need to pass the initrd parameters via the device tree. This means there is no longer a reason to update the device tree in the first place, and so we can ignore it entirely. The only remaining reason to deal with the devicetree is if we are using the 'devicetree' command to load one from disk, so tweak the logic in grub_fdt_install() to take that into account. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> --- grub-core/loader/arm64/linux.c | 22 ++++++++++---------- grub-core/loader/efi/fdt.c | 7 +++++-- 2 files changed, 16 insertions(+), 13 deletions(-)