| Message ID | 20201025060441.11961-5-xypron.glpk@gmx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | sandbox: implement cold reset | expand |
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
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>
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;
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
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
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
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 >
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
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(+) -- 2.28.0