sandbox: CONFIG_SYS_RELOC_GD_ENV_ADDR?

sandbox: CONFIG_SYS_RELOC_GD_ENV_ADDR?

[AKASHI Takahiro]

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.

Thanks,
-Takahiro Akashi

sandbox: CONFIG_SYS_RELOC_GD_ENV_ADDR?

[Simon Glass]

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.

Regards,
Simon

sandbox: CONFIG_SYS_RELOC_GD_ENV_ADDR?

[Tom Rini]

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>

sandbox: CONFIG_SYS_RELOC_GD_ENV_ADDR?

[AKASHI Takahiro]

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

sandbox: CONFIG_SYS_RELOC_GD_ENV_ADDR?

[Simon Glass]

Hi AKASHI,

On Sun, 16 Feb 2020 at 22:01, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> 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.

OK I see, thank you. Yes it seems like this should not be used on sandbox.

Regards,
Simon
[..]