Patchwork [for-1.4,1/2] linux-user: Fix cpu_copy() usage

login
register
mail settings
Submitter Andreas Färber
Date Jan. 30, 2013, 12:34 a.m.
Message ID <1359506059-765-2-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/216738/
State New
Headers show

Comments

Andreas Färber - Jan. 30, 2013, 12:34 a.m.
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(-)
Igor Mammedov - Jan. 30, 2013, 7:18 a.m.
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.
Andreas Färber - Jan. 30, 2013, 7:46 a.m.
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.
Igor Mammedov - Jan. 30, 2013, 8:46 a.m.
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
[...]
>
Andreas Färber - Jan. 30, 2013, 8:53 a.m.
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
Igor Mammedov - Jan. 30, 2013, 10:14 a.m.
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
>

Patch

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;