diff mbox

[for-1.4] *-user: Don't reset X86CPU again

Message ID 1358667575-2915-1-git-send-email-afaerber@suse.de
State New
Headers show

Commit Message

Andreas Färber Jan. 20, 2013, 7:39 a.m. UTC 
Since commit 65dee38052597b6285eb208125369f01b29ba6c1 (target-i386:
move cpu_reset and reset callback to cpu.c) the x86 CPU is reset through
cpu_init() but was still reset immediately after in linux-user and
bsd-user. Similarly it was reset again in linux-user after cpu_copy(),
defeating its very purpose. Clean this up.

Fixing the ppc and sparc cases of cpu_copy() and overhauling its
implementation is left for another day.

Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 bsd-user/main.c      |    2 +-
 linux-user/main.c    |    2 +-
 linux-user/syscall.c |    2 +-
 3 Dateien geändert, 3 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-)

Comments

Igor Mammedov Jan. 21, 2013, 3:54 p.m. UTC | #1 
On Sun, 20 Jan 2013 08:39:35 +0100
Andreas Färber <afaerber@suse.de> wrote:
Subj could be more specific, something along the lines:
  Fix broken breakpoints duplication for i386-{bds,linux}-user

> Since commit 65dee38052597b6285eb208125369f01b29ba6c1 (target-i386:
> move cpu_reset and reset callback to cpu.c) the x86 CPU is reset through
> cpu_init() but was still reset immediately after in linux-user and
> bsd-user. Similarly it was reset again in linux-user after cpu_copy(),
> defeating its very purpose. Clean this up.
> 
> Fixing the ppc and sparc cases of cpu_copy() and overhauling its
> implementation is left for another day.
Before b55a37c98 cpu_reset() was called at the end of cpu_init() and copying
CPUState/duplicating breakpoints afterwards in cpu_copy() worked, BUT commit
b4558d7481 breaks it, by positioning cpu_reset() call after copying
CPUState/duplicating breakpoints. That should break as minimum breakpoints
handling since they all should be duplicated on all CPUs and cpu_reset()
deletes them explicitly.

From my POV patch fixes bug introduced by b4558d7481, Perhaps you should
update commit message to mention this commit at least and what this patch
fixes beside cleanups.

It would be nice though to get confirmation from Blue that all I've said above
is not total nonsense.

> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
>  bsd-user/main.c      |    2 +-
>  linux-user/main.c    |    2 +-
>  linux-user/syscall.c |    2 +-
>  3 Dateien geändert, 3 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-)
> 
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 1dc0330..ae24723 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -917,7 +917,7 @@ int main(int argc, char **argv)
>          fprintf(stderr, "Unable to find CPU definition\n");
>          exit(1);
>      }
> -#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
> +#if defined(TARGET_SPARC) || defined(TARGET_PPC)
>      cpu_reset(ENV_GET_CPU(env));
>  #endif
>      thread_env = env;
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 0181bc2..3df8aa2 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3540,7 +3540,7 @@ int main(int argc, char **argv, char **envp)
>          fprintf(stderr, "Unable to find CPU definition\n");
>          exit(1);
>      }
> -#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
> +#if defined(TARGET_SPARC) || defined(TARGET_PPC)
>      cpu_reset(ENV_GET_CPU(env));
>  #endif
>  
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 693e66f..7be6144 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4361,7 +4361,7 @@ 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)
> +#if defined(TARGET_SPARC) || defined(TARGET_PPC)
>          cpu_reset(ENV_GET_CPU(new_env));
>  #endif
>          /* Init regs that differ from the parent.  */
Andreas Färber Jan. 21, 2013, 4:05 p.m. UTC | #2 
Am 21.01.2013 16:54, schrieb Igor Mammedov:
> On Sun, 20 Jan 2013 08:39:35 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> Subj could be more specific, something along the lines:
>   Fix broken breakpoints duplication for i386-{bds,linux}-user

I agree I should expand to "linux-user: bsd-user: ..." at least, and I
intended it to carry qom-cpu tag since it's quite silent around
linux-user these days...

My endeavor here was purely reducing #ifdef'ery and not resolving a
particular bug. After all, ppc and sparc will still be broken; moving
their cpu_reset() into cpu_copy() to immediately after cpu_init() would
hopefully fix that.

Andreas

> 
>> Since commit 65dee38052597b6285eb208125369f01b29ba6c1 (target-i386:
>> move cpu_reset and reset callback to cpu.c) the x86 CPU is reset through
>> cpu_init() but was still reset immediately after in linux-user and
>> bsd-user. Similarly it was reset again in linux-user after cpu_copy(),
>> defeating its very purpose. Clean this up.
>>
>> Fixing the ppc and sparc cases of cpu_copy() and overhauling its
>> implementation is left for another day.
> Before b55a37c98 cpu_reset() was called at the end of cpu_init() and copying
> CPUState/duplicating breakpoints afterwards in cpu_copy() worked, BUT commit
> b4558d7481 breaks it, by positioning cpu_reset() call after copying
> CPUState/duplicating breakpoints. That should break as minimum breakpoints
> handling since they all should be duplicated on all CPUs and cpu_reset()
> deletes them explicitly.
> 
> From my POV patch fixes bug introduced by b4558d7481, Perhaps you should
> update commit message to mention this commit at least and what this patch
> fixes beside cleanups.
> 
> It would be nice though to get confirmation from Blue that all I've said above
> is not total nonsense.
> 
>>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  bsd-user/main.c      |    2 +-
>>  linux-user/main.c    |    2 +-
>>  linux-user/syscall.c |    2 +-
>>  3 Dateien geändert, 3 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-)
>>
>> diff --git a/bsd-user/main.c b/bsd-user/main.c
>> index 1dc0330..ae24723 100644
>> --- a/bsd-user/main.c
>> +++ b/bsd-user/main.c
>> @@ -917,7 +917,7 @@ int main(int argc, char **argv)
>>          fprintf(stderr, "Unable to find CPU definition\n");
>>          exit(1);
>>      }
>> -#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
>> +#if defined(TARGET_SPARC) || defined(TARGET_PPC)
>>      cpu_reset(ENV_GET_CPU(env));
>>  #endif
>>      thread_env = env;
>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index 0181bc2..3df8aa2 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -3540,7 +3540,7 @@ int main(int argc, char **argv, char **envp)
>>          fprintf(stderr, "Unable to find CPU definition\n");
>>          exit(1);
>>      }
>> -#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
>> +#if defined(TARGET_SPARC) || defined(TARGET_PPC)
>>      cpu_reset(ENV_GET_CPU(env));
>>  #endif
>>  
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 693e66f..7be6144 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -4361,7 +4361,7 @@ 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)
>> +#if defined(TARGET_SPARC) || defined(TARGET_PPC)
>>          cpu_reset(ENV_GET_CPU(new_env));
>>  #endif
>>          /* Init regs that differ from the parent.  */
> 
>
Andreas Färber Jan. 30, 2013, 12:38 a.m. UTC | #3 
Am 21.01.2013 16:54, schrieb Igor Mammedov:
> On Sun, 20 Jan 2013 08:39:35 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> Subj could be more specific, something along the lines:
>   Fix broken breakpoints duplication for i386-{bds,linux}-user
> 
>> Since commit 65dee38052597b6285eb208125369f01b29ba6c1 (target-i386:
>> move cpu_reset and reset callback to cpu.c) the x86 CPU is reset through
>> cpu_init() but was still reset immediately after in linux-user and
>> bsd-user. Similarly it was reset again in linux-user after cpu_copy(),
>> defeating its very purpose. Clean this up.
>>
>> Fixing the ppc and sparc cases of cpu_copy() and overhauling its
>> implementation is left for another day.
> Before b55a37c98 cpu_reset() was called at the end of cpu_init() and copying
> CPUState/duplicating breakpoints afterwards in cpu_copy() worked, BUT commit
> b4558d7481 breaks it, by positioning cpu_reset() call after copying
> CPUState/duplicating breakpoints. That should break as minimum breakpoints
> handling since they all should be duplicated on all CPUs and cpu_reset()
> deletes them explicitly.
> 
> From my POV patch fixes bug introduced by b4558d7481, Perhaps you should
> update commit message to mention this commit at least and what this patch
> fixes beside cleanups.
> 
> It would be nice though to get confirmation from Blue that all I've said above
> is not total nonsense.

I believe your analysis wrt breakpoints and watchpoints is incorrect
since the memset() in cpu_reset() handlers only goes up to
offset(CPUArchState, breakpoints), i.e. not including the breakpoints.

But as discussed with Peter I've posted a v2 that first fixes the reset
bug and then cleans up the now-superfluous x86 reset.

Regards,
Andreas
Igor Mammedov Jan. 30, 2013, 7:08 a.m. UTC | #4 
On Wed, 30 Jan 2013 01:38:47 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 21.01.2013 16:54, schrieb Igor Mammedov:
> > On Sun, 20 Jan 2013 08:39:35 +0100
> > Andreas Färber <afaerber@suse.de> wrote:
> > Subj could be more specific, something along the lines:
> >   Fix broken breakpoints duplication for i386-{bds,linux}-user
> > 
> >> Since commit 65dee38052597b6285eb208125369f01b29ba6c1 (target-i386:
> >> move cpu_reset and reset callback to cpu.c) the x86 CPU is reset through
> >> cpu_init() but was still reset immediately after in linux-user and
> >> bsd-user. Similarly it was reset again in linux-user after cpu_copy(),
> >> defeating its very purpose. Clean this up.
> >>
> >> Fixing the ppc and sparc cases of cpu_copy() and overhauling its
> >> implementation is left for another day.
> > Before b55a37c98 cpu_reset() was called at the end of cpu_init() and copying
> > CPUState/duplicating breakpoints afterwards in cpu_copy() worked, BUT commit
> > b4558d7481 breaks it, by positioning cpu_reset() call after copying
> > CPUState/duplicating breakpoints. That should break as minimum breakpoints
> > handling since they all should be duplicated on all CPUs and cpu_reset()
> > deletes them explicitly.
> > 
> > From my POV patch fixes bug introduced by b4558d7481, Perhaps you should
> > update commit message to mention this commit at least and what this patch
> > fixes beside cleanups.
> > 
> > It would be nice though to get confirmation from Blue that all I've said above
> > is not total nonsense.
> 
> I believe your analysis wrt breakpoints and watchpoints is incorrect
> since the memset() in cpu_reset() handlers only goes up to
> offset(CPUArchState, breakpoints), i.e. not including the breakpoints.
memsetting would lead to leak as minimum, x86_cpu_reset() close to the end
calls cpu_breakpoint_remove_all() and cpu_watchpoint_remove_all() instead. 

> 
> But as discussed with Peter I've posted a v2 that first fixes the reset
> bug and then cleans up the now-superfluous x86 reset.
Cool.

> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
diff mbox

Patch

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 1dc0330..ae24723 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -917,7 +917,7 @@  int main(int argc, char **argv)
         fprintf(stderr, "Unable to find CPU definition\n");
         exit(1);
     }
-#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
+#if defined(TARGET_SPARC) || defined(TARGET_PPC)
     cpu_reset(ENV_GET_CPU(env));
 #endif
     thread_env = env;
diff --git a/linux-user/main.c b/linux-user/main.c
index 0181bc2..3df8aa2 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3540,7 +3540,7 @@  int main(int argc, char **argv, char **envp)
         fprintf(stderr, "Unable to find CPU definition\n");
         exit(1);
     }
-#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
+#if defined(TARGET_SPARC) || defined(TARGET_PPC)
     cpu_reset(ENV_GET_CPU(env));
 #endif
 
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 693e66f..7be6144 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4361,7 +4361,7 @@  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)
+#if defined(TARGET_SPARC) || defined(TARGET_PPC)
         cpu_reset(ENV_GET_CPU(new_env));
 #endif
         /* Init regs that differ from the parent.  */