Re: [Qemu-devel] [PATCH RFC qom-cpu] linux-user: Avoid conditional cpu_reset()

[Eduardo Habkost]

On Wed, Jul 10, 2013 at 07:51:32PM +0100, Peter Maydell wrote:
> On 10 July 2013 19:43, Eduardo Habkost <address@hidden> 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 <address@hidden>
> >> Signed-off-by: Andreas Färber <address@hidden>
> >> Cc: Eduardo Habkost <address@hidden>
> >> ---
> >>  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