* sandbox: CONFIG_SYS_RELOC_GD_ENV_ADDR?
2020-02-14 19:16 ` Simon Glass
@ 2020-02-14 19:43 ` Tom Rini
2020-02-17 6:01 ` AKASHI Takahiro
1 sibling, 0 replies; 5+ messages in thread
From: Tom Rini @ 2020-02-14 19:43 UTC (permalink / raw)
To: u-boot
On Fri, Feb 14, 2020 at 12:16:33PM -0700, Simon Glass wrote:
> Hi AKASHI,
>
> On Thu, 13 Feb 2020 at 18:39, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Tom, Simon,
> >
> > Is CONFIG_SYS_RELOC_GD_ENV_ADDR really needed on sandbox?
> >
> > When I try to have a variable environment on emulated SPI flash,
> > the U-Boot binary always crashes: (NOTE: assuming CONFIG_ENV_ADDR == 0)
> > $ dd if=/dev/zero of=./spi.bin bs=1M count=4
> > $ u-boot -T
> > U-Boot 2020.04-rc2-00015-gc9afef2b1938-dirty (Feb 14 2020 - 10:24:59 +0900)
> >
> > Model: sandbox
> > DRAM: 128 MiB
> > WDT: Started with servicing (60s timeout)
> > MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
> > Loading Environment from SPI Flash... SF: Detected m25p16 with page size 256 Bytes, erase size 64 KiB, total 2 MiB
> > *** Warning - bad CRC, using default environment
> >
> > Segmentation fault (core dumped)
> >
> > If this configuration is disabled, panic doesn't happen.
> > I think that it should be turned off in any sandbox*_defconfig.
> >
> > In addition, please update
> > - doc/arch/sandbox.rst
> > - doc/SPI/README.sandbox-spi
> > Both two still mention already-removed command line option, --spi_sf.
> > It is confusing.
>
> I'm not an expert on this, but I can't see any use for this in
> sandbox. One problem might be that it should be using map_sysmem()
> instead of a cast.
If the option needs to be dropped for things to work, we should just
drop it. When migrating some of the logic here in to CONFIG symbols I
did match, I'm pretty certain, the previous behavior. But there's been
a few other cases I got backwards, so this could have been another. So
long as 'make tests' is happy after the change, we should just do it.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200214/42a2ee13/attachment.sig>
^ permalink raw reply [flat|nested] 5+ messages in thread
* sandbox: CONFIG_SYS_RELOC_GD_ENV_ADDR?
2020-02-14 19:16 ` Simon Glass
2020-02-14 19:43 ` Tom Rini
@ 2020-02-17 6:01 ` AKASHI Takahiro
2020-02-27 23:40 ` Simon Glass
1 sibling, 1 reply; 5+ messages in thread
From: AKASHI Takahiro @ 2020-02-17 6:01 UTC (permalink / raw)
To: u-boot
Tom, Simon,
On Fri, Feb 14, 2020 at 12:16:33PM -0700, Simon Glass wrote:
> Hi AKASHI,
>
> On Thu, 13 Feb 2020 at 18:39, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Tom, Simon,
> >
> > Is CONFIG_SYS_RELOC_GD_ENV_ADDR really needed on sandbox?
> >
> > When I try to have a variable environment on emulated SPI flash,
> > the U-Boot binary always crashes: (NOTE: assuming CONFIG_ENV_ADDR == 0)
> > $ dd if=/dev/zero of=./spi.bin bs=1M count=4
> > $ u-boot -T
> > U-Boot 2020.04-rc2-00015-gc9afef2b1938-dirty (Feb 14 2020 - 10:24:59 +0900)
> >
> > Model: sandbox
> > DRAM: 128 MiB
> > WDT: Started with servicing (60s timeout)
> > MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
> > Loading Environment from SPI Flash... SF: Detected m25p16 with page size 256 Bytes, erase size 64 KiB, total 2 MiB
> > *** Warning - bad CRC, using default environment
> >
> > Segmentation fault (core dumped)
> >
> > If this configuration is disabled, panic doesn't happen.
> > I think that it should be turned off in any sandbox*_defconfig.
> >
> > In addition, please update
> > - doc/arch/sandbox.rst
> > - doc/SPI/README.sandbox-spi
> > Both two still mention already-removed command line option, --spi_sf.
> > It is confusing.
>
> I'm not an expert on this, but I can't see any use for this in
> sandbox. One problem might be that it should be using map_sysmem()
> instead of a cast.
No, map_sysmem() doesn't make sense here because gd->env_addr will be
set to &default_environment[0], which is a global variable, not any
(emulated) physical memory address, in env_init().
Please note that 'CONFIG_ENV_ADDR != 0' case doesn't make sense for SPI flash
because it is not a "memory-mapped" memory in general.
More details:
In board_init_f(),
* env_init() is called, and SPI flash is selected as backing storage.
Then, gd->env_addr is set to &default_environment[0], with
gd->env_valid set to ENV_VALID(!), because the initial content
of SPI flash is all zero'ed.
After relocation, in board_init_r(),
* initr_reloc_global_data() is called and gd->env_addr is forced to
be offset with gd->reloc_off.
* env_relocate() is called in intr_env(), then env_load() as gd->env_valid
is ENV_VALID. Called in turns are:
(Please also follow the backtrace output of gdb attached below for details.)
env_sf_load()
env_import()
himport_r()
hsearch_r()
env_callback_init()
env_get(".callbacks") <== (1)
env_get_f()
env_get_char() <== (2)
env_get_char_spec()
At (1), env_get_f() is called as GD_FLG_ENV_READY bit in gd->flags is
not set yet. At (2), env_get_char_spec() is called as gd->env_valid
is ENV_VALID(!).
As a result, env_get_char_spec() tries to access memory pointed to by gd->env_addr
and causes a segmentation fault because it has been wrongly "relocated"
due to CONFIG_SYS_RELOC_GD_ENV_ADDR.
Again, gd->env_addr should never be "relocated" here for sandbox as it is
just a pointer to some global variable (in this case).
(In other words, all the addresses of global variables have already been
"relocated" from the beginning on sandbox. Right?)
Any thoughts?
Thanks,
-Takahiro Akashi
===8<===
$ gdb u-boot
(gdb) run -T
Starting program: /home/akashi/x86/build/uboot_sandbox_cod/u-boot -T
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
U-Boot 2020.04-rc2-00017-g0e9304ed9276 (Feb 17 2020 - 14:13:04 +0900)
Model: sandbox
DRAM: 128 MiB
WDT: Started with servicing (60s timeout)
MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from SPI Flash... SF: Detected m25p16 with page size 256 Bytes, erase size 64 KiB, total 2 MiB
Program received signal SIGSEGV, Segmentation fault.
0x00005555555e3b21 in env_get_char_spec (index=index at entry=0)
@/home/akashi/arm/armv8/linaro/u-boot/env/env.c:169
169 return *(uchar *)(gd->env_addr + index);
(gdb) bt
#0 0x00005555555e3b21 in env_get_char_spec (index=index at entry=0)
at /home/akashi/arm/armv8/linaro/u-boot/env/env.c:169
#1 0x00005555555e3b3c in env_get_char (index=index at entry=0)
at /home/akashi/arm/armv8/linaro/u-boot/env/env.c:177
#2 0x000055555559720f in env_get_f (name=0x5555556c7cdc ".callbacks",
buf=0x156fae80 "115200", len=len at entry=32)
at /home/akashi/arm/armv8/linaro/u-boot/cmd/nvedit.c:708
#3 0x0000555555597332 in env_get (name=name at entry=0x5555556c7cdc ".callbacks")
at /home/akashi/arm/armv8/linaro/u-boot/cmd/nvedit.c:678
#4 0x00005555555e486d in env_callback_init (var_entry=0x15b111d8)
at /home/akashi/arm/armv8/linaro/u-boot/env/callback.c:54
#5 0x0000555555635c00 in hsearch_r (item=..., action=action at entry=ENV_ENTER,
retval=retval at entry=0x7fffffffd868,
htab=htab at entry=0x555555954670 <env_htab>, flag=flag at entry=0)
at /home/akashi/arm/armv8/linaro/u-boot/lib/hashtable.c:389
#6 0x0000555555636287 in himport_r (htab=htab at entry=0x555555954670 <env_htab>,
env=env at entry=0x15710154 "arch=sandbox", size=size at entry=2097148,
sep=sep at entry=0 '\000', flag=flag at entry=0, crlf_is_lf=crlf_is_lf at entry=0,
nvars=0, vars=0x0) at /home/akashi/arm/armv8/linaro/u-boot/lib/hashtable.c:935
#7 0x00005555555e37fd in env_import (
buf=buf at entry=0x15710150 "\352|\240\265arch=sandbox", check=check at entry=1)
at /home/akashi/arm/armv8/linaro/u-boot/env/common.c:125
#8 0x00005555555e49ae in env_sf_load ()
at /home/akashi/arm/armv8/linaro/u-boot/env/sf.c:274
#9 0x00005555555e3bab in env_load ()
at /home/akashi/arm/armv8/linaro/u-boot/env/env.c:201
#10 0x00005555555e38f1 in env_relocate ()
at /home/akashi/arm/armv8/linaro/u-boot/env/common.c:242
#11 0x000055555559a4b0 in initr_env ()
at /home/akashi/arm/armv8/linaro/u-boot/common/board_r.c:480
#12 0x000055555559a6a6 in initcall_run_list (
init_sequence=0x55555594ff00 <init_sequence_r>)
at /home/akashi/arm/armv8/linaro/u-boot/include/initcall.h:40
#13 board_init_r (new_gd=<optimized out>, dest_addr=<optimized out>)
at /home/akashi/arm/armv8/linaro/u-boot/common/board_r.c:900
#14 0x000055555557a97a in main (argc=2, argv=0x7fffffffdc98)
at /home/akashi/arm/armv8/linaro/u-boot/arch/sandbox/cpu/start.c:435
^ permalink raw reply [flat|nested] 5+ messages in thread