Re: [Qemu-devel] [PATCH for-1.4] *-user: Don't reset X86CPU again

[Igor Mammedov]

On Sun, 20 Jan 2013 08:39:35 +0100
Andreas Färber <address@hidden> 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 <address@hidden>
> Signed-off-by: Andreas Färber <address@hidden>
> Cc: Peter Maydell <address@hidden>
> ---
>  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.  */