| Message ID | 1359506059-765-2-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Wed, 30 Jan 2013 01:34:18 +0100 Andreas Färber <afaerber@suse.de> wrote: > Commit b4558d7481aefc865b0b52bf9b285ebcf2e8b59f ((x86/Sparc/PPC)-user: > fix cpu_copy) added a CPU reset after cpu_copy() inside linux-user code. > This reverses the register copying that cpu_copy() does. > > Clean this up by moving the cpu_reset() call to after cpu_init() but > before memcpy(). This matches the initial CPU creation in linux-user. > > Cc: Blue Swirl <blauwirbel@gmail.com> > Signed-off-by: Andreas Färber <afaerber@suse.de> > Cc: Peter Maydell <peter.maydell@linaro.org> > --- > exec.c | 6 ++++++ > linux-user/syscall.c | 3 --- > 2 Dateien geändert, 6 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-) > > diff --git a/exec.c b/exec.c > index b85508b..8dfa458 100644 > --- a/exec.c > +++ b/exec.c > @@ -537,6 +537,12 @@ CPUArchState *cpu_copy(CPUArchState *env) > CPUWatchpoint *wp; > #endif > > +#ifdef CONFIG_USER_ONLY unnecessary ifdef-feniry here, cpu_copy() is linux-user only thing. > +#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC) > + cpu_reset(ENV_GET_CPU(new_env)); > +#endif > +#endif > + > memcpy(new_env, env, sizeof(CPUArchState)); > > /* Preserve chaining. */ > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 693e66f..6c254ba 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -4361,9 +4361,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp, > init_task_state(ts); > /* we create a new CPU instance. */ > new_env = cpu_copy(env); > -#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC) > - cpu_reset(ENV_GET_CPU(new_env)); > -#endif > /* Init regs that differ from the parent. */ > cpu_clone_regs(new_env, newsp); > new_env->opaque = ts; > -- > 1.7.10.4 > > Otherwise looks good.
Am 30.01.2013 08:18, schrieb Igor Mammedov: > On Wed, 30 Jan 2013 01:34:18 +0100 > Andreas Färber <afaerber@suse.de> wrote: > >> Commit b4558d7481aefc865b0b52bf9b285ebcf2e8b59f ((x86/Sparc/PPC)-user: >> fix cpu_copy) added a CPU reset after cpu_copy() inside linux-user code. >> This reverses the register copying that cpu_copy() does. >> >> Clean this up by moving the cpu_reset() call to after cpu_init() but >> before memcpy(). This matches the initial CPU creation in linux-user. >> >> Cc: Blue Swirl <blauwirbel@gmail.com> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> --- >> exec.c | 6 ++++++ >> linux-user/syscall.c | 3 --- >> 2 Dateien geändert, 6 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-) >> >> diff --git a/exec.c b/exec.c >> index b85508b..8dfa458 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -537,6 +537,12 @@ CPUArchState *cpu_copy(CPUArchState *env) >> CPUWatchpoint *wp; >> #endif >> >> +#ifdef CONFIG_USER_ONLY > unnecessary ifdef-feniry here, cpu_copy() is linux-user only thing. It is currently used only by linux-user (and hopefully will stay that way :)) but I don't see the code limited to CONFIG_USER_ONLY? Andreas >> +#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC) >> + cpu_reset(ENV_GET_CPU(new_env)); >> +#endif >> +#endif >> + >> memcpy(new_env, env, sizeof(CPUArchState)); >> >> /* Preserve chaining. */ >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 693e66f..6c254ba 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -4361,9 +4361,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp, >> init_task_state(ts); >> /* we create a new CPU instance. */ >> new_env = cpu_copy(env); >> -#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC) >> - cpu_reset(ENV_GET_CPU(new_env)); >> -#endif >> /* Init regs that differ from the parent. */ >> cpu_clone_regs(new_env, newsp); >> new_env->opaque = ts; >> -- >> 1.7.10.4 >> >> > Otherwise looks good.
On Wed, 30 Jan 2013 08:46:34 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 30.01.2013 08:18, schrieb Igor Mammedov: > > On Wed, 30 Jan 2013 01:34:18 +0100 > > Andreas Färber <afaerber@suse.de> wrote: > > > >> Commit b4558d7481aefc865b0b52bf9b285ebcf2e8b59f ((x86/Sparc/PPC)-user: > >> fix cpu_copy) added a CPU reset after cpu_copy() inside linux-user code. > >> This reverses the register copying that cpu_copy() does. > >> > >> Clean this up by moving the cpu_reset() call to after cpu_init() but > >> before memcpy(). This matches the initial CPU creation in linux-user. > >> > >> Cc: Blue Swirl <blauwirbel@gmail.com> > >> Signed-off-by: Andreas Färber <afaerber@suse.de> > >> Cc: Peter Maydell <peter.maydell@linaro.org> > >> --- > >> exec.c | 6 ++++++ > >> linux-user/syscall.c | 3 --- > >> 2 Dateien geändert, 6 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-) > >> > >> diff --git a/exec.c b/exec.c > >> index b85508b..8dfa458 100644 > >> --- a/exec.c > >> +++ b/exec.c > >> @@ -537,6 +537,12 @@ CPUArchState *cpu_copy(CPUArchState *env) > >> CPUWatchpoint *wp; > >> #endif > >> > >> +#ifdef CONFIG_USER_ONLY > > unnecessary ifdef-feniry here, cpu_copy() is linux-user only thing. > > It is currently used only by linux-user (and hopefully will stay that > way :)) but I don't see the code limited to CONFIG_USER_ONLY? If we don't have to have this define (i.e. nothing breaks without it), lets skip it. As alternative we could move cpu_copy() to linux-user as the only user of it, it will help to cleanup exec.c of uncommon code. > > Andreas [...] >
Am 30.01.2013 09:46, schrieb Igor Mammedov: > On Wed, 30 Jan 2013 08:46:34 +0100 > Andreas Färber <afaerber@suse.de> wrote: > >> Am 30.01.2013 08:18, schrieb Igor Mammedov: >>> On Wed, 30 Jan 2013 01:34:18 +0100 >>> Andreas Färber <afaerber@suse.de> wrote: >>> >>>> Commit b4558d7481aefc865b0b52bf9b285ebcf2e8b59f ((x86/Sparc/PPC)-user: >>>> fix cpu_copy) added a CPU reset after cpu_copy() inside linux-user code. >>>> This reverses the register copying that cpu_copy() does. >>>> >>>> Clean this up by moving the cpu_reset() call to after cpu_init() but >>>> before memcpy(). This matches the initial CPU creation in linux-user. >>>> >>>> Cc: Blue Swirl <blauwirbel@gmail.com> >>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>> Cc: Peter Maydell <peter.maydell@linaro.org> >>>> --- >>>> exec.c | 6 ++++++ >>>> linux-user/syscall.c | 3 --- >>>> 2 Dateien geändert, 6 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-) >>>> >>>> diff --git a/exec.c b/exec.c >>>> index b85508b..8dfa458 100644 >>>> --- a/exec.c >>>> +++ b/exec.c >>>> @@ -537,6 +537,12 @@ CPUArchState *cpu_copy(CPUArchState *env) >>>> CPUWatchpoint *wp; >>>> #endif >>>> >>>> +#ifdef CONFIG_USER_ONLY >>> unnecessary ifdef-feniry here, cpu_copy() is linux-user only thing. >> >> It is currently used only by linux-user (and hopefully will stay that >> way :)) but I don't see the code limited to CONFIG_USER_ONLY? > If we don't have to have this define (i.e. nothing breaks without it), lets > skip it. I disagree. > As alternative we could move cpu_copy() to linux-user as the only user of it, > it will help to cleanup exec.c of uncommon code. That's the plan mentioned in the cover letter. I'll wait if we get any feedback from the linux-user maintainer. Problem is, syscall.c is a real big mess and I don't want to make it more so. Plan was to split it up by architecture but I have had other priorities and no volunteer stepped up. Nothing breaks any more than it is by not applying this. Andreas
On Wed, 30 Jan 2013 09:53:29 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 30.01.2013 09:46, schrieb Igor Mammedov: > > On Wed, 30 Jan 2013 08:46:34 +0100 > > Andreas Färber <afaerber@suse.de> wrote: > > > >> Am 30.01.2013 08:18, schrieb Igor Mammedov: > >>> On Wed, 30 Jan 2013 01:34:18 +0100 > >>> Andreas Färber <afaerber@suse.de> wrote: > >>> > >>>> Commit b4558d7481aefc865b0b52bf9b285ebcf2e8b59f ((x86/Sparc/PPC)-user: > >>>> fix cpu_copy) added a CPU reset after cpu_copy() inside linux-user > >>>> code. This reverses the register copying that cpu_copy() does. > >>>> > >>>> Clean this up by moving the cpu_reset() call to after cpu_init() but > >>>> before memcpy(). This matches the initial CPU creation in linux-user. > >>>> > >>>> Cc: Blue Swirl <blauwirbel@gmail.com> > >>>> Signed-off-by: Andreas Färber <afaerber@suse.de> > >>>> Cc: Peter Maydell <peter.maydell@linaro.org> > >>>> --- > >>>> exec.c | 6 ++++++ > >>>> linux-user/syscall.c | 3 --- > >>>> 2 Dateien geändert, 6 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-) > >>>> > >>>> diff --git a/exec.c b/exec.c > >>>> index b85508b..8dfa458 100644 > >>>> --- a/exec.c > >>>> +++ b/exec.c > >>>> @@ -537,6 +537,12 @@ CPUArchState *cpu_copy(CPUArchState *env) > >>>> CPUWatchpoint *wp; > >>>> #endif > >>>> > >>>> +#ifdef CONFIG_USER_ONLY > >>> unnecessary ifdef-feniry here, cpu_copy() is linux-user only thing. > >> > >> It is currently used only by linux-user (and hopefully will stay that > >> way :)) but I don't see the code limited to CONFIG_USER_ONLY? > > If we don't have to have this define (i.e. nothing breaks without it), > > lets skip it. > > I disagree. Just some time people really object add not must have ifdef-s, so there is no clear guide on matter. Anyway, It was nitpicking for my side, so I've no real objection to it. > > > As alternative we could move cpu_copy() to linux-user as the only user of > > it, it will help to cleanup exec.c of uncommon code. > > That's the plan mentioned in the cover letter. I'll wait if we get any > feedback from the linux-user maintainer. Problem is, syscall.c is a real > big mess and I don't want to make it more so. Plan was to split it up by > architecture but I have had other priorities and no volunteer stepped up. It's fine if we do it later. So far there were no objections from maintainer or Blue about fixing this, thus if needed my: Reviewed-By: Igor Mammedov <imammedo@redhat.com> > > Nothing breaks any more than it is by not applying this. > > Andreas >
diff --git a/exec.c b/exec.c index b85508b..8dfa458 100644 --- a/exec.c +++ b/exec.c @@ -537,6 +537,12 @@ CPUArchState *cpu_copy(CPUArchState *env) CPUWatchpoint *wp; #endif +#ifdef CONFIG_USER_ONLY +#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC) + cpu_reset(ENV_GET_CPU(new_env)); +#endif +#endif + memcpy(new_env, env, sizeof(CPUArchState)); /* Preserve chaining. */ diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 693e66f..6c254ba 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -4361,9 +4361,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp, init_task_state(ts); /* we create a new CPU instance. */ new_env = cpu_copy(env); -#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC) - cpu_reset(ENV_GET_CPU(new_env)); -#endif /* Init regs that differ from the parent. */ cpu_clone_regs(new_env, newsp); new_env->opaque = ts;
Commit b4558d7481aefc865b0b52bf9b285ebcf2e8b59f ((x86/Sparc/PPC)-user: fix cpu_copy) added a CPU reset after cpu_copy() inside linux-user code. This reverses the register copying that cpu_copy() does. Clean this up by moving the cpu_reset() call to after cpu_init() but before memcpy(). This matches the initial CPU creation in linux-user. Cc: Blue Swirl <blauwirbel@gmail.com> Signed-off-by: Andreas Färber <afaerber@suse.de> Cc: Peter Maydell <peter.maydell@linaro.org> --- exec.c | 6 ++++++ linux-user/syscall.c | 3 --- 2 Dateien geändert, 6 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-)