* [Qemu-devel] [PATCH RFC qom-cpu] linux-user: Avoid conditional cpu_reset() @ 2013-07-10 16:30 Andreas Färber 2013-07-10 18:43 ` Eduardo Habkost 0 siblings, 1 reply; 4+ messages in thread From: Andreas Färber @ 2013-07-10 16:30 UTC (permalink / raw) To: qemu-devel; +Cc: Riku Voipio, Andreas Färber, Eduardo Habkost Some CPUs reset as part of cpu_init(), some others were reset afterwards, some not at all. While some targets didn't implement a cpu_[state_]reset() function, QOM cpu_reset() is always available. There's nothing wrong with resetting twice on startup, so drop the #ifdef. Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Andreas Färber <afaerber@suse.de> Cc: Eduardo Habkost <ehabkost@redhat.com> --- This had been discussed as a possible cleanup for the #ifdef. I am uncertain whether we should do this since it hides the TODO item of investigating ppc and sparc CPU reset. linux-user/main.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/linux-user/main.c b/linux-user/main.c index 7f15d3d..e904d8c 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -3637,9 +3637,7 @@ int main(int argc, char **argv, char **envp) fprintf(stderr, "Unable to find CPU definition\n"); exit(1); } -#if defined(TARGET_SPARC) || defined(TARGET_PPC) cpu_reset(ENV_GET_CPU(env)); -#endif thread_cpu = ENV_GET_CPU(env); -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-cpu] linux-user: Avoid conditional cpu_reset() 2013-07-10 16:30 [Qemu-devel] [PATCH RFC qom-cpu] linux-user: Avoid conditional cpu_reset() Andreas Färber @ 2013-07-10 18:43 ` Eduardo Habkost 2013-07-10 18:51 ` Peter Maydell 0 siblings, 1 reply; 4+ messages in thread From: Eduardo Habkost @ 2013-07-10 18:43 UTC (permalink / raw) To: Andreas Färber; +Cc: Riku Voipio, qemu-devel On Wed, Jul 10, 2013 at 06:30:38PM +0200, Andreas Färber wrote: > Some CPUs reset as part of cpu_init(), some others were reset > afterwards, some not at all. While some targets didn't implement a > cpu_[state_]reset() function, QOM cpu_reset() is always available. > There's nothing wrong with resetting twice on startup, so drop > the #ifdef. > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Andreas Färber <afaerber@suse.de> > Cc: Eduardo Habkost <ehabkost@redhat.com> > --- > This had been discussed as a possible cleanup for the #ifdef. > I am uncertain whether we should do this since it hides the TODO item > of investigating ppc and sparc CPU reset. > > linux-user/main.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/linux-user/main.c b/linux-user/main.c > index 7f15d3d..e904d8c 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -3637,9 +3637,7 @@ int main(int argc, char **argv, char **envp) > fprintf(stderr, "Unable to find CPU definition\n"); > exit(1); > } > -#if defined(TARGET_SPARC) || defined(TARGET_PPC) > cpu_reset(ENV_GET_CPU(env)); > -#endif Most of the cpu_reset() implementations I have looked at (including sparc) contain something like: memset(env, 0, offsetof(CPUXXXState, breakpoints)); Isn't this clearing userspace registers that are not supposed to be touched by clone()? -- Eduardo ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-cpu] linux-user: Avoid conditional cpu_reset() 2013-07-10 18:43 ` Eduardo Habkost @ 2013-07-10 18:51 ` Peter Maydell 2013-07-10 19:02 ` Eduardo Habkost 0 siblings, 1 reply; 4+ messages in thread From: Peter Maydell @ 2013-07-10 18:51 UTC (permalink / raw) To: Eduardo Habkost; +Cc: Riku Voipio, Andreas Färber, qemu-devel On 10 July 2013 19:43, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Jul 10, 2013 at 06:30:38PM +0200, Andreas Färber wrote: >> Some CPUs reset as part of cpu_init(), some others were reset >> afterwards, some not at all. While some targets didn't implement a >> cpu_[state_]reset() function, QOM cpu_reset() is always available. >> There's nothing wrong with resetting twice on startup, so drop >> the #ifdef. >> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> --- >> This had been discussed as a possible cleanup for the #ifdef. >> I am uncertain whether we should do this since it hides the TODO item >> of investigating ppc and sparc CPU reset. >> >> linux-user/main.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/linux-user/main.c b/linux-user/main.c >> index 7f15d3d..e904d8c 100644 >> --- a/linux-user/main.c >> +++ b/linux-user/main.c >> @@ -3637,9 +3637,7 @@ int main(int argc, char **argv, char **envp) >> fprintf(stderr, "Unable to find CPU definition\n"); >> exit(1); >> } >> -#if defined(TARGET_SPARC) || defined(TARGET_PPC) >> cpu_reset(ENV_GET_CPU(env)); >> -#endif > > Most of the cpu_reset() implementations I have looked at (including > sparc) contain something like: > > memset(env, 0, offsetof(CPUXXXState, breakpoints)); > > Isn't this clearing userspace registers that are not supposed to be > touched by clone()? You're thinking about the other cpu_reset() -- this one is in main.c and happens only for the main thread, immediately after we've created that thread's CPU, and before we set up its registers for initial program start. The cpu_reset() that sometimes happens in the clone() path is in syscall.c (and is addressed by patch http://patchwork.ozlabs.org/patch/257232/ ) -- PMM ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-cpu] linux-user: Avoid conditional cpu_reset() 2013-07-10 18:51 ` Peter Maydell @ 2013-07-10 19:02 ` Eduardo Habkost 0 siblings, 0 replies; 4+ messages in thread From: Eduardo Habkost @ 2013-07-10 19:02 UTC (permalink / raw) To: Peter Maydell; +Cc: Riku Voipio, Andreas Färber, qemu-devel On Wed, Jul 10, 2013 at 07:51:32PM +0100, Peter Maydell wrote: > On 10 July 2013 19:43, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Wed, Jul 10, 2013 at 06:30:38PM +0200, Andreas Färber wrote: > >> Some CPUs reset as part of cpu_init(), some others were reset > >> afterwards, some not at all. While some targets didn't implement a > >> cpu_[state_]reset() function, QOM cpu_reset() is always available. > >> There's nothing wrong with resetting twice on startup, so drop > >> the #ifdef. > >> > >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> > >> Signed-off-by: Andreas Färber <afaerber@suse.de> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> --- > >> This had been discussed as a possible cleanup for the #ifdef. > >> I am uncertain whether we should do this since it hides the TODO item > >> of investigating ppc and sparc CPU reset. > >> > >> linux-user/main.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/linux-user/main.c b/linux-user/main.c > >> index 7f15d3d..e904d8c 100644 > >> --- a/linux-user/main.c > >> +++ b/linux-user/main.c > >> @@ -3637,9 +3637,7 @@ int main(int argc, char **argv, char **envp) > >> fprintf(stderr, "Unable to find CPU definition\n"); > >> exit(1); > >> } > >> -#if defined(TARGET_SPARC) || defined(TARGET_PPC) > >> cpu_reset(ENV_GET_CPU(env)); > >> -#endif > > > > Most of the cpu_reset() implementations I have looked at (including > > sparc) contain something like: > > > > memset(env, 0, offsetof(CPUXXXState, breakpoints)); > > > > Isn't this clearing userspace registers that are not supposed to be > > touched by clone()? > > You're thinking about the other cpu_reset() -- this one is in > main.c and happens only for the main thread, immediately > after we've created that thread's CPU, and before we set up > its registers for initial program start. The cpu_reset() > that sometimes happens in the clone() path is in syscall.c > (and is addressed by patch > http://patchwork.ozlabs.org/patch/257232/ ) Oh, nevermind. We discussed do_fork() for so long that I didn't even notice we had _another_ cpu_reset() #ifdef on main.c. In this case the patch looks simple and safe, to me. -- Eduardo ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-07-10 19:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-07-10 16:30 [Qemu-devel] [PATCH RFC qom-cpu] linux-user: Avoid conditional cpu_reset() Andreas Färber 2013-07-10 18:43 ` Eduardo Habkost 2013-07-10 18:51 ` Peter Maydell 2013-07-10 19:02 ` Eduardo Habkost
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).