[PATCH 0/5] sandbox: implement cold reset

[PATCH 0/5] sandbox: implement cold reset

[Heinrich Schuchardt]

The command to shut down a device is 'poweroff'. It is a deficit of the
sandbox that it does not support resetting yet but shuts down upong seeing
the 'reset' command.

The patch series implements the cold reset function as a relaunch of the
U-Boot binary via execv().

Unit tests are adjusted.

A bug leading to closing the console input file descriptor is resolved.

Heinrich Schuchardt (5):
  sandbox: eth-raw: do not close the console input
  sandbox: enable poweroff command
  test/py: test poweroff
  sandbox: implement reset
  test: adjust sysreset tests

 arch/Kconfig                              |  3 ++-
 arch/sandbox/cpu/eth-raw-os.c             |  8 ++++----
 arch/sandbox/cpu/os.c                     | 19 ++++++++++++++++++-
 arch/sandbox/cpu/start.c                  | 22 ++++++++++++++++++++++
 arch/sandbox/cpu/state.c                  |  1 +
 arch/sandbox/include/asm/u-boot-sandbox.h |  3 +++
 drivers/sysreset/sysreset_sandbox.c       |  3 +++
 include/os.h                              |  5 +++++
 test/dm/sysreset.c                        | 11 ++++++++---
 test/py/tests/test_sandbox_exit.py        |  8 ++++----
 10 files changed, 70 insertions(+), 13 deletions(-)

--
2.28.0

[PATCH 1/5] sandbox: eth-raw: do not close the console input

[Heinrich Schuchardt]

When the sandbox eth-raw device host_lo is removed this leads to closing
the console input.

Do not call close(0).

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 arch/sandbox/cpu/eth-raw-os.c | 8 ++++----
 arch/sandbox/cpu/os.c         | 5 ++++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c
index da01d1addf..6a8d809756 100644
--- a/arch/sandbox/cpu/eth-raw-os.c
+++ b/arch/sandbox/cpu/eth-raw-os.c
@@ -53,7 +53,7 @@ int sandbox_eth_raw_os_is_local(const char *ifname)
 	}
 	ret = !!(ifr.ifr_flags & IFF_LOOPBACK);
 out:
-	close(fd);
+	os_close(fd);
 	return ret;
 }

@@ -220,7 +220,7 @@ int sandbox_eth_raw_os_send(void *packet, int length,
 		struct sockaddr_in addr;

 		if (priv->local_bind_sd != -1)
-			close(priv->local_bind_sd);
+			os_close(priv->local_bind_sd);

 		/* A normal UDP socket is required to bind */
 		priv->local_bind_sd = socket(AF_INET, SOCK_DGRAM, 0);
@@ -284,11 +284,11 @@ void sandbox_eth_raw_os_stop(struct eth_sandbox_raw_priv *priv)
 {
 	free(priv->device);
 	priv->device = NULL;
-	close(priv->sd);
+	os_close(priv->sd);
 	priv->sd = -1;
 	if (priv->local) {
 		if (priv->local_bind_sd != -1)
-			close(priv->local_bind_sd);
+			os_close(priv->local_bind_sd);
 		priv->local_bind_sd = -1;
 		priv->local_bind_udp_port = 0;
 	}
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index e7ec892bdf..c461fb0db0 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -86,7 +86,10 @@ int os_open(const char *pathname, int os_flags)

 int os_close(int fd)
 {
-	return close(fd);
+	/* Do not close the console input */
+	if (fd)
+		return close(fd);
+	return -1;
 }

 int os_unlink(const char *pathname)
--
2.28.0

[PATCH 1/5] sandbox: eth-raw: do not close the console input

[Simon Glass]

On Sun, 25 Oct 2020 at 00:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> When the sandbox eth-raw device host_lo is removed this leads to closing
> the console input.
>
> Do not call close(0).
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  arch/sandbox/cpu/eth-raw-os.c | 8 ++++----
>  arch/sandbox/cpu/os.c         | 5 ++++-
>  2 files changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 2/5] sandbox: enable poweroff command

[Heinrich Schuchardt]

The command to shut down a device is 'poweroff'. It is a deficit of the
sandbox that it does not support resetting yet but shuts down upong seeing
the 'reset' command.

Once the sandbox properly supports reset we need the 'poweroff' command to
leave the sandbox.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 arch/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 683e384319..63e9d725b7 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -87,6 +87,7 @@ config SANDBOX
 	bool "Sandbox"
 	select BOARD_LATE_INIT
 	select BZIP2
+	select CMD_POWEROFF
 	select DM
 	select DM_GPIO
 	select DM_I2C
@@ -102,7 +103,7 @@ config SANDBOX
 	select PCI_ENDPOINT
 	select SPI
 	select SUPPORT_OF_CONTROL
-	select SYSRESET_CMD_POWEROFF if CMD_POWEROFF
+	select SYSRESET_CMD_POWEROFF
 	imply BITREVERSE
 	select BLOBLIST
 	imply CMD_DM
--
2.28.0

[PATCH 2/5] sandbox: enable poweroff command

[Simon Glass]

On Sun, 25 Oct 2020 at 00:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> The command to shut down a device is 'poweroff'. It is a deficit of the
> sandbox that it does not support resetting yet but shuts down upong seeing
> the 'reset' command.
>
> Once the sandbox properly supports reset we need the 'poweroff' command to
> leave the sandbox.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  arch/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/5] test/py: test poweroff

[Heinrich Schuchardt]

It is the 'poweroff' and not the 'reset' command that should shut down the
sandbox.

Adjust the unit test accordingly

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 test/py/tests/test_sandbox_exit.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/py/tests/test_sandbox_exit.py b/test/py/tests/test_sandbox_exit.py
index a301f4b559..2d242ae0f6 100644
--- a/test/py/tests/test_sandbox_exit.py
+++ b/test/py/tests/test_sandbox_exit.py
@@ -6,11 +6,11 @@ import pytest
 import signal

 @pytest.mark.boardspec('sandbox')
- at pytest.mark.buildconfigspec('sysreset')
-def test_reset(u_boot_console):
-    """Test that the "reset" command exits sandbox process."""
+ at pytest.mark.buildconfigspec('sysreset_cmd_poweroff')
+def test_poweroff(u_boot_console):
+    """Test that the "poweroff" command exits sandbox process."""

-    u_boot_console.run_command('reset', wait_for_prompt=False)
+    u_boot_console.run_command('poweroff', wait_for_prompt=False)
     assert(u_boot_console.validate_exited())

 @pytest.mark.boardspec('sandbox')
--
2.28.0

[PATCH 3/5] test/py: test poweroff

[Simon Glass]

On Sun, 25 Oct 2020 at 00:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> It is the 'poweroff' and not the 'reset' command that should shut down the
> sandbox.
>
> Adjust the unit test accordingly
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  test/py/tests/test_sandbox_exit.py | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 4/5] sandbox: implement reset

[Heinrich Schuchardt]

Up to now the sandbox would shutdown upon a cold reset request. Instead it
should be reset.

In our coding we use static variables. The only safe way to return to an
initial state is to relaunch the U-Boot binary.

The reset implementation uses a longjmp() to return to the main() function
and then relaunches U-Boot using execv().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 arch/sandbox/cpu/os.c                     | 14 ++++++++++++++
 arch/sandbox/cpu/start.c                  | 22 ++++++++++++++++++++++
 arch/sandbox/cpu/state.c                  |  1 +
 arch/sandbox/include/asm/u-boot-sandbox.h |  3 +++
 drivers/sysreset/sysreset_sandbox.c       |  3 +++
 include/os.h                              |  5 +++++
 6 files changed, 48 insertions(+)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index c461fb0db0..ed044e87fb 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -817,3 +817,17 @@ void *os_find_text_base(void)

 	return base;
 }
+
+void os_relaunch(int argc, char *argv[])
+{
+	char **args;
+
+	args = calloc(argc + 1, sizeof(char *));
+	if (!args)
+		goto out;
+	memcpy(args, argv, sizeof(char *) * argc);
+	args[argc] = NULL;
+	execv(args[0], args);
+out:
+	os_exit(1);
+}
diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
index c6a2bbe468..ee1d4b9581 100644
--- a/arch/sandbox/cpu/start.c
+++ b/arch/sandbox/cpu/start.c
@@ -5,6 +5,7 @@

 #include <common.h>
 #include <command.h>
+#include <dm/root.h>
 #include <errno.h>
 #include <init.h>
 #include <os.h>
@@ -14,11 +15,15 @@
 #include <asm/io.h>
 #include <asm/malloc.h>
 #include <asm/sections.h>
+#include <asm/setjmp.h>
 #include <asm/state.h>
 #include <linux/ctype.h>

 DECLARE_GLOBAL_DATA_PTR;

+/* longjmp buffer for reset */
+static struct jmp_buf_data reset_jmp;
+
 /* Compare two options so that they can be sorted into alphabetical order */
 static int h_compare_opt(const void *p1, const void *p2)
 {
@@ -394,12 +399,29 @@ void state_show(struct sandbox_state *state)
 	printf("\n");
 }

+void sandbox_reset(void)
+{
+	/* Do this here while it still has an effect */
+	os_fd_restore();
+	if (state_uninit())
+		os_exit(2);
+
+	if (dm_uninit())
+		os_exit(2);
+
+	/* This is considered normal termination for now */
+	longjmp(&reset_jmp, 1);
+}
+
 int main(int argc, char *argv[])
 {
 	struct sandbox_state *state;
 	gd_t data;
 	int ret;

+	if (setjmp(&reset_jmp))
+		os_relaunch(argc, argv);
+
 	memset(&data, '\0', sizeof(data));
 	gd = &data;
 	gd->arch.text_base = os_find_text_base();
diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
index 34b6fff7e7..59f37fab0b 100644
--- a/arch/sandbox/cpu/state.c
+++ b/arch/sandbox/cpu/state.c
@@ -358,6 +358,7 @@ void state_reset_for_test(struct sandbox_state *state)
 	/* No reset yet, so mark it as such. Always allow power reset */
 	state->last_sysreset = SYSRESET_COUNT;
 	state->sysreset_allowed[SYSRESET_POWER_OFF] = true;
+	state->sysreset_allowed[SYSRESET_COLD] = true;
 	state->allow_memio = false;

 	memset(&state->wdt, '\0', sizeof(state->wdt));
diff --git a/arch/sandbox/include/asm/u-boot-sandbox.h b/arch/sandbox/include/asm/u-boot-sandbox.h
index 798d003077..b1bdcbcde5 100644
--- a/arch/sandbox/include/asm/u-boot-sandbox.h
+++ b/arch/sandbox/include/asm/u-boot-sandbox.h
@@ -84,6 +84,9 @@ void sandbox_set_enable_pci_map(int enable);
  */
 int sandbox_read_fdt_from_file(void);

+/* Reset sandbox */
+void sandbox_reset(void);
+
 /* Exit sandbox (quit U-Boot) */
 void sandbox_exit(void);

diff --git a/drivers/sysreset/sysreset_sandbox.c b/drivers/sysreset/sysreset_sandbox.c
index 69c22a7000..c92132798c 100644
--- a/drivers/sysreset/sysreset_sandbox.c
+++ b/drivers/sysreset/sysreset_sandbox.c
@@ -56,6 +56,9 @@ static int sandbox_sysreset_request(struct udevice *dev, enum sysreset_t type)
 	switch (type) {
 	case SYSRESET_COLD:
 		state->last_sysreset = type;
+		if (!state->sysreset_allowed[type])
+			return -EACCES;
+		sandbox_reset();
 		break;
 	case SYSRESET_POWER_OFF:
 		state->last_sysreset = type;
diff --git a/include/os.h b/include/os.h
index 1874ae674f..187dbf06f2 100644
--- a/include/os.h
+++ b/include/os.h
@@ -355,4 +355,9 @@ int os_read_file(const char *name, void **bufp, int *sizep);
  */
 void *os_find_text_base(void);

+/**
+ * os_relaunch() - restart the sandbox
+ */
+void os_relaunch(int argc, char *argv[]);
+
 #endif
--
2.28.0

[PATCH 4/5] sandbox: implement reset

[Simon Glass]

Hi Heinrich,

On Sun, 25 Oct 2020 at 00:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Up to now the sandbox would shutdown upon a cold reset request. Instead it
> should be reset.
>
> In our coding we use static variables. The only safe way to return to an
> initial state is to relaunch the U-Boot binary.

This is unfortunate, but I suspect you may be right. Have you looked at it?

>
> The reset implementation uses a longjmp() to return to the main() function
> and then relaunches U-Boot using execv().
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  arch/sandbox/cpu/os.c                     | 14 ++++++++++++++
>  arch/sandbox/cpu/start.c                  | 22 ++++++++++++++++++++++
>  arch/sandbox/cpu/state.c                  |  1 +
>  arch/sandbox/include/asm/u-boot-sandbox.h |  3 +++
>  drivers/sysreset/sysreset_sandbox.c       |  3 +++
>  include/os.h                              |  5 +++++
>  6 files changed, 48 insertions(+)
>
> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> index c461fb0db0..ed044e87fb 100644
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> @@ -817,3 +817,17 @@ void *os_find_text_base(void)
>
>         return base;
>  }
> +
> +void os_relaunch(int argc, char *argv[])
> +{
> +       char **args;
> +
> +       args = calloc(argc + 1, sizeof(char *));
> +       if (!args)
> +               goto out;
> +       memcpy(args, argv, sizeof(char *) * argc);
> +       args[argc] = NULL;
> +       execv(args[0], args);
> +out:
> +       os_exit(1);
> +}
> diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
> index c6a2bbe468..ee1d4b9581 100644
> --- a/arch/sandbox/cpu/start.c
> +++ b/arch/sandbox/cpu/start.c
> @@ -5,6 +5,7 @@
>
>  #include <common.h>
>  #include <command.h>
> +#include <dm/root.h>

Put before linux/ below

>  #include <errno.h>
>  #include <init.h>
>  #include <os.h>
> @@ -14,11 +15,15 @@
>  #include <asm/io.h>
>  #include <asm/malloc.h>
>  #include <asm/sections.h>
> +#include <asm/setjmp.h>
>  #include <asm/state.h>
>  #include <linux/ctype.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +/* longjmp buffer for reset */
> +static struct jmp_buf_data reset_jmp;
> +
>  /* Compare two options so that they can be sorted into alphabetical order */
>  static int h_compare_opt(const void *p1, const void *p2)
>  {
> @@ -394,12 +399,29 @@ void state_show(struct sandbox_state *state)
>         printf("\n");
>  }
>
> +void sandbox_reset(void)
> +{
> +       /* Do this here while it still has an effect */
> +       os_fd_restore();
> +       if (state_uninit())
> +               os_exit(2);
> +
> +       if (dm_uninit())
> +               os_exit(2);
> +
> +       /* This is considered normal termination for now */
> +       longjmp(&reset_jmp, 1);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>         struct sandbox_state *state;
>         gd_t data;
>         int ret;
>
> +       if (setjmp(&reset_jmp))
> +               os_relaunch(argc, argv);
> +
>         memset(&data, '\0', sizeof(data));
>         gd = &data;
>         gd->arch.text_base = os_find_text_base();
> diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
> index 34b6fff7e7..59f37fab0b 100644
> --- a/arch/sandbox/cpu/state.c
> +++ b/arch/sandbox/cpu/state.c
> @@ -358,6 +358,7 @@ void state_reset_for_test(struct sandbox_state *state)
>         /* No reset yet, so mark it as such. Always allow power reset */
>         state->last_sysreset = SYSRESET_COUNT;
>         state->sysreset_allowed[SYSRESET_POWER_OFF] = true;
> +       state->sysreset_allowed[SYSRESET_COLD] = true;
>         state->allow_memio = false;
>
>         memset(&state->wdt, '\0', sizeof(state->wdt));
> diff --git a/arch/sandbox/include/asm/u-boot-sandbox.h b/arch/sandbox/include/asm/u-boot-sandbox.h
> index 798d003077..b1bdcbcde5 100644
> --- a/arch/sandbox/include/asm/u-boot-sandbox.h
> +++ b/arch/sandbox/include/asm/u-boot-sandbox.h
> @@ -84,6 +84,9 @@ void sandbox_set_enable_pci_map(int enable);
>   */
>  int sandbox_read_fdt_from_file(void);
>
> +/* Reset sandbox */

I think this needs a better comment, explaining how it resets.

> +void sandbox_reset(void);
> +
>  /* Exit sandbox (quit U-Boot) */
>  void sandbox_exit(void);
>
> diff --git a/drivers/sysreset/sysreset_sandbox.c b/drivers/sysreset/sysreset_sandbox.c
> index 69c22a7000..c92132798c 100644
> --- a/drivers/sysreset/sysreset_sandbox.c
> +++ b/drivers/sysreset/sysreset_sandbox.c
> @@ -56,6 +56,9 @@ static int sandbox_sysreset_request(struct udevice *dev, enum sysreset_t type)
>         switch (type) {
>         case SYSRESET_COLD:
>                 state->last_sysreset = type;
> +               if (!state->sysreset_allowed[type])
> +                       return -EACCES;
> +               sandbox_reset();
>                 break;
>         case SYSRESET_POWER_OFF:
>                 state->last_sysreset = type;
> diff --git a/include/os.h b/include/os.h
> index 1874ae674f..187dbf06f2 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -355,4 +355,9 @@ int os_read_file(const char *name, void **bufp, int *sizep);
>   */
>  void *os_find_text_base(void);
>
> +/**
> + * os_relaunch() - restart the sandbox

again I think a bit more detail would help

> + */
> +void os_relaunch(int argc, char *argv[]);
> +
>  #endif
> --
> 2.28.0
>

Regards,
Simon

[PATCH 4/5] sandbox: implement reset

[Heinrich Schuchardt]

On 27.10.20 05:52, Simon Glass wrote:
> Hi Heinrich,
>
> On Sun, 25 Oct 2020 at 00:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Up to now the sandbox would shutdown upon a cold reset request. Instead it
>> should be reset.
>>
>> In our coding we use static variables. The only safe way to return to an
>> initial state is to relaunch the U-Boot binary.
>
> This is unfortunate, but I suspect you may be right. Have you looked at it?

I am not so much worried about the sandbox specific code, where you find
static variables like:

arch/sandbox/cpu/os.c:165:static bool term_setup;
arch/sandbox/cpu/os.c:166:static bool term_nonblock;

My worry is about the main U-Boot where it is completely legal to expect
that static variables are initialized, e.g.

lib/efi_loader/efi_boottime.c:28:LIST_HEAD(efi_obj_list);
lib/efi_loader/efi_boottime.c:34:LIST_HEAD(efi_event_queue);
lib/efi_loader/efi_boottime.c:40:LIST_HEAD(efi_register_notify_events);

Best regards

Heinrich

>
>>
>> The reset implementation uses a longjmp() to return to the main() function
>> and then relaunches U-Boot using execv().
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH 4/5] sandbox: implement reset

[Heinrich Schuchardt]

On 27.10.20 05:52, Simon Glass wrote:
> Hi Heinrich,
>
> On Sun, 25 Oct 2020 at 00:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Up to now the sandbox would shutdown upon a cold reset request. Instead it
>> should be reset.
>>
>> In our coding we use static variables. The only safe way to return to an
>> initial state is to relaunch the U-Boot binary.
>
> This is unfortunate, but I suspect you may be right. Have you looked at it?
>
>>
>> The reset implementation uses a longjmp() to return to the main() function
>> and then relaunches U-Boot using execv().
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  arch/sandbox/cpu/os.c                     | 14 ++++++++++++++
>>  arch/sandbox/cpu/start.c                  | 22 ++++++++++++++++++++++
>>  arch/sandbox/cpu/state.c                  |  1 +
>>  arch/sandbox/include/asm/u-boot-sandbox.h |  3 +++
>>  drivers/sysreset/sysreset_sandbox.c       |  3 +++
>>  include/os.h                              |  5 +++++
>>  6 files changed, 48 insertions(+)
>>
>> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
>> index c461fb0db0..ed044e87fb 100644
>> --- a/arch/sandbox/cpu/os.c
>> +++ b/arch/sandbox/cpu/os.c
>> @@ -817,3 +817,17 @@ void *os_find_text_base(void)
>>
>>         return base;
>>  }
>> +
>> +void os_relaunch(int argc, char *argv[])
>> +{
>> +       char **args;
>> +
>> +       args = calloc(argc + 1, sizeof(char *));
>> +       if (!args)
>> +               goto out;
>> +       memcpy(args, argv, sizeof(char *) * argc);
>> +       args[argc] = NULL;
>> +       execv(args[0], args);
>> +out:
>> +       os_exit(1);
>> +}
>> diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
>> index c6a2bbe468..ee1d4b9581 100644
>> --- a/arch/sandbox/cpu/start.c
>> +++ b/arch/sandbox/cpu/start.c
>> @@ -5,6 +5,7 @@
>>
>>  #include <common.h>
>>  #include <command.h>
>> +#include <dm/root.h>
>
> Put before linux/ below

https://www.denx.de/wiki/U-Boot/CodingStyle does not allow this. It
requires: "Within that order, sort your includes.". Alphabetically dm/*
goes before errno.h.

Should the driver model require anything else, please, update the coding
style guideline accordingly. Looking at dm/root.h I cannot see any such
requirement.

I think we should move the code style guideline to the HTML
documentation and get rid of the Wiki page.

Best regards

Heinrich

>
>>  #include <errno.h>
>>  #include <init.h>
>>  #include <os.h>
>> @@ -14,11 +15,15 @@
>>  #include <asm/io.h>
>>  #include <asm/malloc.h>
>>  #include <asm/sections.h>
>> +#include <asm/setjmp.h>
>>  #include <asm/state.h>
>>  #include <linux/ctype.h>
>>
>>  DECLARE_GLOBAL_DATA_PTR;

[PATCH 4/5] sandbox: implement reset

[Rasmus Villemoes]

On 25/10/2020 07.04, Heinrich Schuchardt wrote:
> Up to now the sandbox would shutdown upon a cold reset request. Instead it
> should be reset.
> 
> In our coding we use static variables. The only safe way to return to an
> initial state is to relaunch the U-Boot binary.
> 
> The reset implementation uses a longjmp() to return to the main() function
> and then relaunches U-Boot using execv().
> 

That seems to be needlessly fragile.

1. getopt_long can permute the elements of the argv array
2. From reading "man longjmp", I'm not sure argc and argv are actually
guaranteed to have the values they had originally upon reaching the
setjmp() the second time

Now, 1. is probably mostly when there's a mix of options and positional
arguments, and ./u-boot doesn't take the latter. And 2. possibly also
doesn't apply because we don't currently modify argc or argv in main()
itself - but that could change with some future refactoring.

So perhaps it works, and maybe that's even guaranteed with the current
code and APIs that are used. But, is there any reason to muck with a
complex beast like setjmp/longjmp when we could just

static char **saved_argv;

os_relaunch(void) {
  execve(saved_argv[0], saved_argv);
}

static int save_argv(int argc, char **argv)
{
   /* essentially the prologue of your os_relaunch() */
}

main() {
  save_argv(argc, argv);
  ...
}

(one can argue whether memcpy'ing the argv array is sufficient, or if
one should really strdup() each element, since one is allowed to modify
the strings, though again, I don't think we do that currently).

Rasmus

[PATCH 4/5] sandbox: implement reset

[Heinrich Schuchardt]

On 27.10.20 13:12, Rasmus Villemoes wrote:
> On 25/10/2020 07.04, Heinrich Schuchardt wrote:
>> Up to now the sandbox would shutdown upon a cold reset request. Instead it
>> should be reset.
>>
>> In our coding we use static variables. The only safe way to return to an
>> initial state is to relaunch the U-Boot binary.
>>
>> The reset implementation uses a longjmp() to return to the main() function
>> and then relaunches U-Boot using execv().
>>
>
> That seems to be needlessly fragile.
>
> 1. getopt_long can permute the elements of the argv array
> 2. From reading "man longjmp", I'm not sure argc and argv are actually
> guaranteed to have the values they had originally upon reaching the
> setjmp() the second time
>
> Now, 1. is probably mostly when there's a mix of options and positional
> arguments, and ./u-boot doesn't take the latter. And 2. possibly also
> doesn't apply because we don't currently modify argc or argv in main()
> itself - but that could change with some future refactoring.
>
> So perhaps it works, and maybe that's even guaranteed with the current
> code and APIs that are used. But, is there any reason to muck with a
> complex beast like setjmp/longjmp when we could just

1) argc is passed by value.

argv is defined as "char * const argv[]". It is interesting that
getopt_long() ignores the const keyword which explicitly forbids
permuting the sequence. Cf.
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/getopt.c;h=8d251dd5fabb46e17f5d593bf12d0619a8867bee;hb=HEAD#l730

But even if the sequence is permuted why should the result be different
if getopt_long is called again?

2) longjmp() restores all registers including the stack pointer. argc
and &argv[0] are on the stack or in a register depending on the
architecture. Local variables() in main are also on the stack.

So the only "fragile" thing is the stack. But if U-Boot corrupts the
stack or memory, we are anyway lost.

>
> static char **saved_argv;
>
> os_relaunch(void) {
>   execve(saved_argv[0], saved_argv);
> }
>
> static int save_argv(int argc, char **argv)
> {
>    /* essentially the prologue of your os_relaunch() */
> }
>
> main() {
>   save_argv(argc, argv);
>   ...
> }
>
> (one can argue whether memcpy'ing the argv array is sufficient, or if
> one should really strdup() each element, since one is allowed to modify
> the strings, though again, I don't think we do that currently).

No C program is allowed to change the strings passed to main() in argv.
Unfortunately Simon forgot to add const.

The following does not compile:

int main(int argc, const char *argv[])
{
????????argv[0][1] = 'a';
????????return 0;
}

Best regards

Heinrich

[PATCH 4/5] sandbox: implement reset

[Rasmus Villemoes]

On 27/10/2020 14.33, Heinrich Schuchardt wrote:
> On 27.10.20 13:12, Rasmus Villemoes wrote:
>> On 25/10/2020 07.04, Heinrich Schuchardt wrote:
>>> Up to now the sandbox would shutdown upon a cold reset request. Instead it
>>> should be reset.
>>>
>>> In our coding we use static variables. The only safe way to return to an
>>> initial state is to relaunch the U-Boot binary.
>>>
>>> The reset implementation uses a longjmp() to return to the main() function
>>> and then relaunches U-Boot using execv().
>>>
>>
>> That seems to be needlessly fragile.
>>
>> 1. getopt_long can permute the elements of the argv array
>> 2. From reading "man longjmp", I'm not sure argc and argv are actually
>> guaranteed to have the values they had originally upon reaching the
>> setjmp() the second time
>>
>> Now, 1. is probably mostly when there's a mix of options and positional
>> arguments, and ./u-boot doesn't take the latter. And 2. possibly also
>> doesn't apply because we don't currently modify argc or argv in main()
>> itself - but that could change with some future refactoring.
>>
>> So perhaps it works, and maybe that's even guaranteed with the current
>> code and APIs that are used. But, is there any reason to muck with a
>> complex beast like setjmp/longjmp when we could just
> 
> 1) argc is passed by value.

Yes, but which value. To be concrete, on x86-64, upon entry to main(),
the OS-supplied value of argc is in %rdi. When you get to setjmp() the
second time, how do you know that the value of argc you use to call
os_relaunch matches that original value?

> argv is defined as "char * const argv[]". It is interesting that
> getopt_long() ignores the const keyword which explicitly forbids
> permuting the sequence. Cf.
> https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/getopt.c;h=8d251dd5fabb46e17f5d593bf12d0619a8867bee;hb=HEAD#l730
> 
> But even if the sequence is permuted why should the result be different
> if getopt_long is called again?

It probably would not be different. But why risk some hard-to-debug
scenario? There are indeed some cases where setjmp/longjmp is the best
solution to a problem. I really don't see that this is one of those.

> 2) longjmp() restores all registers including the stack pointer. 

Citation needed. man longjmp says "longjmp() may restore the values of
other registers in addition to the stack pointer and program counter",
which certainly doesn't suggest that it unconditionally restores every
register to the state it had at the point of setjmp().

Again, I think it all works currently, but only because we don't do
something that modifies argc or argv after the setjmp call. POSIX
(https://pubs.opengroup.org/onlinepubs/9699919799/):

... except that the values of objects of automatic storage duration are
unspecified if they meet all the following conditions:

    They are local to the function containing the corresponding setjmp()
invocation.

    They do not have volatile-qualified type.

    They are changed between the setjmp() invocation and longjmp() call.

argc and argv satisfy the first two bullets. Anybody adding something to
main() somewhere after that setjmp() call that modifies argc or argv
(not the pointed-to things, just argv itself) would tick off that last
bullet. So that would at the very least warrant a big comment above the
setjmp() that argc and argv are not to be modfied within main().


> architecture. Local variables() in main are also on the stack.

No, they are not. That may be how a compiler from 1980 would do things.

>> (one can argue whether memcpy'ing the argv array is sufficient, or if
>> one should really strdup() each element, since one is allowed to modify
>> the strings, though again, I don't think we do that currently).
> 
> No C program is allowed to change the strings passed to main() in argv.

Wrong. C99, 5.1.2.2.1  Program startup:

The  parameters argc and argv and  *the  strings  pointed  to  by  the
argv array*  shall be  modifiable  by  the  program,  and  retain  their
 last-stored  values  between  program startup and program termination.

(emphasis mine). This is a perfectly compliant program:

#include <string.h>
int main(int argc, char *argv[])
{
  if (argc) memset(argv[0], '#', strlen(argv[0]));
  return 0;
}

[it also so happens that on linux, that modification ends up being
visible in /proc/$pid/cmdline]. Yes, u-boot/sandbox does not make use of
that currently, and it's hard to think of a reason to do stuff like that.

> Unfortunately Simon forgot to add const.
> 
> The following does not compile:
> 
> int main(int argc, const char *argv[])
> {
> ????????argv[0][1] = 'a';
> ????????return 0;
> }

Of course not, but that's not particularly relevant. That prototype of
main() is not standards-conformant (see 5.1.2.2.1 again), except perhaps
as an implementation-defined extension (so most likely any real compiler
will accept it, and then of course will prevent you from doing stores
through the argv[i] pointers).

Rasmus

[PATCH 4/5] sandbox: implement reset

[Heinrich Schuchardt]

On 27.10.20 15:36, Rasmus Villemoes wrote:
> On 27/10/2020 14.33, Heinrich Schuchardt wrote:
>> On 27.10.20 13:12, Rasmus Villemoes wrote:
>>> On 25/10/2020 07.04, Heinrich Schuchardt wrote:
>>>> Up to now the sandbox would shutdown upon a cold reset request. Instead it
>>>> should be reset.
>>>>
>>>> In our coding we use static variables. The only safe way to return to an
>>>> initial state is to relaunch the U-Boot binary.
>>>>
>>>> The reset implementation uses a longjmp() to return to the main() function
>>>> and then relaunches U-Boot using execv().
>>>>
>>>
>>> That seems to be needlessly fragile.
>>>
>>> 1. getopt_long can permute the elements of the argv array
>>> 2. From reading "man longjmp", I'm not sure argc and argv are actually
>>> guaranteed to have the values they had originally upon reaching the
>>> setjmp() the second time
>>>
>>> Now, 1. is probably mostly when there's a mix of options and positional
>>> arguments, and ./u-boot doesn't take the latter. And 2. possibly also
>>> doesn't apply because we don't currently modify argc or argv in main()
>>> itself - but that could change with some future refactoring.
>>>
>>> So perhaps it works, and maybe that's even guaranteed with the current
>>> code and APIs that are used. But, is there any reason to muck with a
>>> complex beast like setjmp/longjmp when we could just
>>
>> 1) argc is passed by value.
>
> Yes, but which value. To be concrete, on x86-64, upon entry to main(),
> the OS-supplied value of argc is in %rdi. When you get to setjmp() the
> second time, how do you know that the value of argc you use to call
> os_relaunch matches that original value?
>
>> argv is defined as "char * const argv[]". It is interesting that
>> getopt_long() ignores the const keyword which explicitly forbids
>> permuting the sequence. Cf.
>> https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/getopt.c;h=8d251dd5fabb46e17f5d593bf12d0619a8867bee;hb=HEAD#l730
>>
>> But even if the sequence is permuted why should the result be different
>> if getopt_long is called again?
>
> It probably would not be different. But why risk some hard-to-debug
> scenario? There are indeed some cases where setjmp/longjmp is the best
> solution to a problem. I really don't see that this is one of those.

Could you, please, explain how you would do a cold reset on the sandbox
without longjmp().

Take this example: U-Boot calls an EFI binary which calls the Reset()
service in the UEFI API implemented via efi_reset_system_boottime().

Best regards

Heinrich

>
>> 2) longjmp() restores all registers including the stack pointer.
>
> Citation needed. man longjmp says "longjmp() may restore the values of
> other registers in addition to the stack pointer and program counter",
> which certainly doesn't suggest that it unconditionally restores every
> register to the state it had at the point of setjmp().
>
> Again, I think it all works currently, but only because we don't do
> something that modifies argc or argv after the setjmp call. POSIX
> (https://pubs.opengroup.org/onlinepubs/9699919799/):
>
> ... except that the values of objects of automatic storage duration are
> unspecified if they meet all the following conditions:
>
>     They are local to the function containing the corresponding setjmp()
> invocation.
>
>     They do not have volatile-qualified type.
>
>     They are changed between the setjmp() invocation and longjmp() call.
>
> argc and argv satisfy the first two bullets. Anybody adding something to
> main() somewhere after that setjmp() call that modifies argc or argv
> (not the pointed-to things, just argv itself) would tick off that last
> bullet. So that would at the very least warrant a big comment above the
> setjmp() that argc and argv are not to be modfied within main().
>
>
>> architecture. Local variables() in main are also on the stack.
>
> No, they are not. That may be how a compiler from 1980 would do things.
>
>>> (one can argue whether memcpy'ing the argv array is sufficient, or if
>>> one should really strdup() each element, since one is allowed to modify
>>> the strings, though again, I don't think we do that currently).
>>
>> No C program is allowed to change the strings passed to main() in argv.
>
> Wrong. C99, 5.1.2.2.1  Program startup:
>
> The  parameters argc and argv and  *the  strings  pointed  to  by  the
> argv array*  shall be  modifiable  by  the  program,  and  retain  their
>  last-stored  values  between  program startup and program termination.
>
> (emphasis mine). This is a perfectly compliant program:
>
> #include <string.h>
> int main(int argc, char *argv[])
> {
>   if (argc) memset(argv[0], '#', strlen(argv[0]));
>   return 0;
> }
>
> [it also so happens that on linux, that modification ends up being
> visible in /proc/$pid/cmdline].

This shows why you should not change argv[].

Yes, u-boot/sandbox does not make use of
> that currently, and it's hard to think of a reason to do stuff like that.
>
>> Unfortunately Simon forgot to add const.
>>
>> The following does not compile:
>>
>> int main(int argc, const char *argv[])
>> {
>> ????????argv[0][1] = 'a';
>> ????????return 0;
>> }
>
> Of course not, but that's not particularly relevant. That prototype of
> main() is not standards-conformant (see 5.1.2.2.1 again), except perhaps
> as an implementation-defined extension (so most likely any real compiler
> will accept it, and then of course will prevent you from doing stores
> through the argv[i] pointers).
>
> Rasmus
>

[PATCH 5/5] test: adjust sysreset tests

[Heinrich Schuchardt]

As we have a working COLD_RESET on the sandbox the sysreset test has to be
adjusted.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 test/dm/sysreset.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/test/dm/sysreset.c b/test/dm/sysreset.c
index aec97b1cbb..691683c567 100644
--- a/test/dm/sysreset.c
+++ b/test/dm/sysreset.c
@@ -37,7 +37,9 @@ static int dm_test_sysreset_base(struct unit_test_state *uts)
 	/* Device 2 is the cold sysreset device */
 	ut_assertok(uclass_get_device(UCLASS_SYSRESET, 2, &dev));
 	ut_asserteq(-ENOSYS, sysreset_request(dev, SYSRESET_WARM));
+	state->sysreset_allowed[SYSRESET_COLD] = false;
 	ut_asserteq(-EACCES, sysreset_request(dev, SYSRESET_COLD));
+	state->sysreset_allowed[SYSRESET_COLD] = true;
 	state->sysreset_allowed[SYSRESET_POWER] = false;
 	ut_asserteq(-EACCES, sysreset_request(dev, SYSRESET_POWER));
 	state->sysreset_allowed[SYSRESET_POWER] = true;
@@ -71,22 +73,25 @@ static int dm_test_sysreset_walk(struct unit_test_state *uts)
 	struct sandbox_state *state = state_get_current();

 	/* If we generate a power sysreset, we will exit sandbox! */
+	state->sysreset_allowed[SYSRESET_WARM] = false;
+	state->sysreset_allowed[SYSRESET_COLD] = false;
 	state->sysreset_allowed[SYSRESET_POWER] = false;
 	state->sysreset_allowed[SYSRESET_POWER_OFF] = false;
 	ut_asserteq(-EACCES, sysreset_walk(SYSRESET_WARM));
 	ut_asserteq(-EACCES, sysreset_walk(SYSRESET_COLD));
 	ut_asserteq(-EACCES, sysreset_walk(SYSRESET_POWER));
+	ut_asserteq(-EACCES, sysreset_walk(SYSRESET_POWER_OFF));

 	/*
 	 * Enable cold system reset - this should make cold system reset work,
 	 * plus a warm system reset should be promoted to cold, since this is
 	 * the next step along.
 	 */
-	state->sysreset_allowed[SYSRESET_COLD] = true;
+	state->sysreset_allowed[SYSRESET_WARM] = true;
 	ut_asserteq(-EINPROGRESS, sysreset_walk(SYSRESET_WARM));
-	ut_asserteq(-EINPROGRESS, sysreset_walk(SYSRESET_COLD));
+	ut_asserteq(-EACCES, sysreset_walk(SYSRESET_COLD));
 	ut_asserteq(-EACCES, sysreset_walk(SYSRESET_POWER));
-	state->sysreset_allowed[SYSRESET_COLD] = false;
+	state->sysreset_allowed[SYSRESET_COLD] = true;
 	state->sysreset_allowed[SYSRESET_POWER] = true;

 	return 0;
--
2.28.0

[PATCH 5/5] test: adjust sysreset tests

[Simon Glass]

On Sun, 25 Oct 2020 at 00:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> As we have a working COLD_RESET on the sandbox the sysreset test has to be
> adjusted.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  test/dm/sysreset.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>