| Message ID | 1531229342-21401-2-git-send-email-kristian.amlie@northern.tech |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | distro_bootcmd: Switch bootefi to use loadaddr by default. | expand |
Ping. Any objections to this change?
On 06.08.18 13:00, Kristian Amlie wrote: > Ping. Any objections to this change? I definitely don't want to have the bootefi path behave any different from the other distro boot targets. That would just cause confusion down the road. Do they (pxe boot, extlinux, etc) make use of loadaddr? Thanks, Alex > > -- Kristian On 10/07/18 15:29, Kristian Amlie wrote: >> loadaddr is configurable in Kconfig using CONFIG_LOADADDR, while >> kernel_addr_r is not. Hence, loadaddr is the future. Provide the >> existing kernel_addr_r as a fallback if loadaddr is not set. >> >> Signed-off-by: Kristian Amlie <kristian.amlie@northern.tech> >> --- >> include/config_distro_bootcmd.h | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h >> index d672e8e..839afcc 100644 >> --- a/include/config_distro_bootcmd.h >> +++ b/include/config_distro_bootcmd.h >> @@ -129,12 +129,15 @@ >> "else " \ >> "bootefi bootmgr ${fdtcontroladdr};" \ >> "fi;" \ >> + "if test -z \"${loadaddr}\"; then " \ >> + "setenv loadaddr ${kernel_addr_r};" \ >> + "fi;" \ >> "load ${devtype} ${devnum}:${distro_bootpart} " \ >> - "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; " \ >> + "${loadaddr} efi/boot/"BOOTEFI_NAME"; " \ >> "if fdt addr ${fdt_addr_r}; then " \ >> - "bootefi ${kernel_addr_r} ${fdt_addr_r};" \ >> + "bootefi ${loadaddr} ${fdt_addr_r};" \ >> "else " \ >> - "bootefi ${kernel_addr_r} ${fdtcontroladdr};" \ >> + "bootefi ${loadaddr} ${fdtcontroladdr};" \ >> "fi\0" \ >> \ >> "load_efi_dtb=" \ >> @@ -277,12 +280,15 @@ >> "setenv efi_old_arch ${bootp_arch};" \ >> "setenv bootp_vci " BOOTENV_EFI_PXE_VCI ";" \ >> "setenv bootp_arch " BOOTENV_EFI_PXE_ARCH ";" \ >> - "if dhcp ${kernel_addr_r}; then " \ >> + "if test -z \"${loadaddr}\"; then " \ >> + "setenv loadaddr ${kernel_addr_r};" \ >> + "fi;" \ >> + "if dhcp ${loadaddr}; then " \ >> "tftpboot ${fdt_addr_r} dtb/${efi_fdtfile};" \ >> "if fdt addr ${fdt_addr_r}; then " \ >> - "bootefi ${kernel_addr_r} ${fdt_addr_r}; " \ >> + "bootefi ${loadaddr} ${fdt_addr_r}; " \ >> "else " \ >> - "bootefi ${kernel_addr_r} ${fdtcontroladdr};" \ >> + "bootefi ${loadaddr} ${fdtcontroladdr};" \ >> "fi;" \ >> "fi;" \ >> "setenv bootp_vci ${efi_old_vci};" \ >>
On 07/08/18 00:06, Alexander Graf wrote: > On 06.08.18 13:00, Kristian Amlie wrote: >> Ping. Any objections to this change? > > I definitely don't want to have the bootefi path behave any different > from the other distro boot targets. That would just cause confusion down > the road. > > Do they (pxe boot, extlinux, etc) make use of loadaddr? No, you're right, apparently they use kernel_addr_r AFAICS [1]. So maybe it is better to stay with kernel_addr_r. The problem is that not all boards follow this, and there are various "bootm ${loadaddr}" and "bootz ${loadaddr}" sprinkled across the various board headers, indicating that they use this. What I want out of this exercise is to make sure that the EFI path in distro_bootcmd will be able to boot an EFI app on any board, regardless of whether it uses kernel_addr_r or loadaddr. Sounds reasonable, right? Following that it seems natural to standardize on one of the two, but it is not entirely clear from the source code which one it should be. I picked loadaddr because it has a CONFIG_LOADADDR define, unlike kernel_addr_r which is just hardcoded in a lot of places, and thus seemed less "proper". But I'm fine with staying with kernel_addr_r as well. There are several things I could do from here on: 1. Keep the current patch, but add "loadaddr" to pxe and extlinux variants as well to get consistent behavior. 2. Reverse order of attempts, and try to use kernel_addr_r first, then loadaddr. Possibly this could also include a patch to pxe and extlinux to make them behave the same. 3. Do nothing, and keep kernel_addr_r as the one and only. IMHO this implicitly means that loadaddr should be deprecated and removed, given their overlapping meaning, and boards converted to using kernel_addr_r. 4. Opposite of 3, make loadaddr into the one and only, and convert all boards to that. Personally I think either option 1 or option 2 is the best, both quite smooth transition paths. Option 3 or 4 are perhaps cleaner solutions in the long term, but are *a lot* more work, and much more likely to cause breakage as well. What do you think? [1] http://git.denx.de/?p=u-boot.git;a=blob;f=cmd/pxe.c;h=5609545de575d090b27f48fd0d2d2ee61b8bdc73;hb=HEAD#l701
On 07.08.18 09:26, Kristian Amlie wrote: > On 07/08/18 00:06, Alexander Graf wrote: >> On 06.08.18 13:00, Kristian Amlie wrote: >>> Ping. Any objections to this change? >> >> I definitely don't want to have the bootefi path behave any different >> from the other distro boot targets. That would just cause confusion down >> the road. >> >> Do they (pxe boot, extlinux, etc) make use of loadaddr? > > No, you're right, apparently they use kernel_addr_r AFAICS [1]. > > So maybe it is better to stay with kernel_addr_r. The problem is that > not all boards follow this, and there are various "bootm ${loadaddr}" > and "bootz ${loadaddr}" sprinkled across the various board headers, > indicating that they use this. Yes, they haven't been converted to distro boot then :). So the path forward for those would be to convert them and slowly deprecate loadaddr. > What I want out of this exercise is to make sure that the EFI path in > distro_bootcmd will be able to boot an EFI app on any board, regardless > of whether it uses kernel_addr_r or loadaddr. Sounds reasonable, right? If you enable distro boot for a board, one of the requirement is that kernel_addr_r is set to a reasonable value: http://git.denx.de/?p=u-boot.git;a=blob;f=doc/README.distro;h=f8e9752a0fa4244ef0a6f7ee530b0aa218080cac;hb=HEAD#l236 > Following that it seems natural to standardize on one of the two, but it > is not entirely clear from the source code which one it should be. I > picked loadaddr because it has a CONFIG_LOADADDR define, unlike > kernel_addr_r which is just hardcoded in a lot of places, and thus > seemed less "proper". But I'm fine with staying with kernel_addr_r as well. I think kernel_addr_r is the defined one for distro boot, yes. > > There are several things I could do from here on: > > > 1. Keep the current patch, but add "loadaddr" to pxe and extlinux > variants as well to get consistent behavior. > > 2. Reverse order of attempts, and try to use kernel_addr_r first, then > loadaddr. Possibly this could also include a patch to pxe and extlinux > to make them behave the same. > > 3. Do nothing, and keep kernel_addr_r as the one and only. IMHO this > implicitly means that loadaddr should be deprecated and removed, given > their overlapping meaning, and boards converted to using kernel_addr_r. I think this is the best option. Just convert boards you care about slowly over to distro boot (which implicitly means they also use kernel_addr_r). For other boards you can not rely on any EFI boot path enumeration anyway, because the boot command doesn't search for EFI binaries ;). Thanks, Alex > > 4. Opposite of 3, make loadaddr into the one and only, and convert all > boards to that. > > > Personally I think either option 1 or option 2 is the best, both quite > smooth transition paths. Option 3 or 4 are perhaps cleaner solutions in > the long term, but are *a lot* more work, and much more likely to cause > breakage as well. > > What do you think? > > > [1] > http://git.denx.de/?p=u-boot.git;a=blob;f=cmd/pxe.c;h=5609545de575d090b27f48fd0d2d2ee61b8bdc73;hb=HEAD#l701 >
On 08/08/18 01:10, Alexander Graf wrote: > On 07.08.18 09:26, Kristian Amlie wrote: >> What I want out of this exercise is to make sure that the EFI path in >> distro_bootcmd will be able to boot an EFI app on any board, regardless >> of whether it uses kernel_addr_r or loadaddr. Sounds reasonable, right? > > If you enable distro boot for a board, one of the requirement is that > kernel_addr_r is set to a reasonable value: > > > http://git.denx.de/?p=u-boot.git;a=blob;f=doc/README.distro;h=f8e9752a0fa4244ef0a6f7ee530b0aa218080cac;hb=HEAD#l236 Aha! This link clears up the confusion. >> There are several things I could do from here on: >> >> >> 1. Keep the current patch, but add "loadaddr" to pxe and extlinux >> variants as well to get consistent behavior. >> >> 2. Reverse order of attempts, and try to use kernel_addr_r first, then >> loadaddr. Possibly this could also include a patch to pxe and extlinux >> to make them behave the same. >> >> 3. Do nothing, and keep kernel_addr_r as the one and only. IMHO this >> implicitly means that loadaddr should be deprecated and removed, given >> their overlapping meaning, and boards converted to using kernel_addr_r. > > I think this is the best option. Just convert boards you care about > slowly over to distro boot (which implicitly means they also use > kernel_addr_r). For other boards you can not rely on any EFI boot path > enumeration anyway, because the boot command doesn't search for EFI > binaries ;). You're right, I had not thought of that. It makes a lot of sense, we will make this part of our recommendation for board porting then. Thanks!
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index d672e8e..839afcc 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -129,12 +129,15 @@ "else " \ "bootefi bootmgr ${fdtcontroladdr};" \ "fi;" \ + "if test -z \"${loadaddr}\"; then " \ + "setenv loadaddr ${kernel_addr_r};" \ + "fi;" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ - "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; " \ + "${loadaddr} efi/boot/"BOOTEFI_NAME"; " \ "if fdt addr ${fdt_addr_r}; then " \ - "bootefi ${kernel_addr_r} ${fdt_addr_r};" \ + "bootefi ${loadaddr} ${fdt_addr_r};" \ "else " \ - "bootefi ${kernel_addr_r} ${fdtcontroladdr};" \ + "bootefi ${loadaddr} ${fdtcontroladdr};" \ "fi\0" \ \ "load_efi_dtb=" \ @@ -277,12 +280,15 @@ "setenv efi_old_arch ${bootp_arch};" \ "setenv bootp_vci " BOOTENV_EFI_PXE_VCI ";" \ "setenv bootp_arch " BOOTENV_EFI_PXE_ARCH ";" \ - "if dhcp ${kernel_addr_r}; then " \ + "if test -z \"${loadaddr}\"; then " \ + "setenv loadaddr ${kernel_addr_r};" \ + "fi;" \ + "if dhcp ${loadaddr}; then " \ "tftpboot ${fdt_addr_r} dtb/${efi_fdtfile};" \ "if fdt addr ${fdt_addr_r}; then " \ - "bootefi ${kernel_addr_r} ${fdt_addr_r}; " \ + "bootefi ${loadaddr} ${fdt_addr_r}; " \ "else " \ - "bootefi ${kernel_addr_r} ${fdtcontroladdr};" \ + "bootefi ${loadaddr} ${fdtcontroladdr};" \ "fi;" \ "fi;" \ "setenv bootp_vci ${efi_old_vci};" \
loadaddr is configurable in Kconfig using CONFIG_LOADADDR, while kernel_addr_r is not. Hence, loadaddr is the future. Provide the existing kernel_addr_r as a fallback if loadaddr is not set. Signed-off-by: Kristian Amlie <kristian.amlie@northern.tech> --- include/config_distro_bootcmd.h | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)