[U-Boot,1/1] distro_bootcmd: Switch bootefi to use loadaddr by default.

Message ID 1531229342-21401-2-git-send-email-kristian.amlie@northern.tech
State New
Delegated to: Tom Rini
Headers show
Series
  • distro_bootcmd: Switch bootefi to use loadaddr by default.
Related show

Commit Message

Kristian Amlie July 10, 2018, 1:29 p.m.
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(-)

Comments

Kristian Amlie Aug. 6, 2018, noon | #1
Ping. Any objections to this change?
Alexander Graf Aug. 6, 2018, 10:06 p.m. | #2
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};"                                \
>>
Kristian Amlie Aug. 7, 2018, 8:26 a.m. | #3
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
Alexander Graf Aug. 7, 2018, 11:10 p.m. | #4
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
>
Kristian Amlie Aug. 9, 2018, 8:32 a.m. | #5
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!

Patch

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};"                                \