Re: [Qemu-devel] [PATCH next v2 73/74] linux-user: Use cpu_reset() after cpu_init() / cpu_copy()

[Peter Maydell]

On 16 May 2012 14:01, Andreas Färber <address@hidden> wrote:
> Am 16.05.2012 14:36, schrieb Igor Mammedov:
>> On 05/11/2012 01:26 PM, Andreas Färber wrote:
>>> Am 11.05.2012 13:22, schrieb Peter Maydell:
>>>> On 10 May 2012 01:14, Andreas Färber<address@hidden>  wrote:
>>>>> Eliminates cpu_state_reset() usage.
>>>>>
>>>>> Signed-off-by: Andreas Färber<address@hidden>
>>>>> ---
>>>>>   linux-user/main.c    |    2 +-
>>>>>   linux-user/syscall.c |    2 +-
>>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/linux-user/main.c b/linux-user/main.c
>>>>> index 191b750..49108b8 100644
>>>>> --- a/linux-user/main.c
>>>>> +++ b/linux-user/main.c
>>>>> @@ -3405,7 +3405,7 @@ int main(int argc, char **argv, char **envp)
>>>>>          exit(1);
>>>>>      }
>>>>>   #if defined(TARGET_I386) || defined(TARGET_SPARC) ||
>>>>> defined(TARGET_PPC)
>>>>> -    cpu_state_reset(env);
>>>>> +    cpu_reset(ENV_GET_CPU(env));
>>>>>   #endif
>>>>>
>>>>>      thread_env = env;
>>>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>>>> index 20d2a74..539af3f 100644
>>>>> --- a/linux-user/syscall.c
>>>>> +++ b/linux-user/syscall.c
>>>>> @@ -4262,7 +4262,7 @@ static int do_fork(CPUArchState *env, unsigned
>>>>> int flags, abi_ulong newsp,
>>>>>          /* we create a new CPU instance. */
>>>>>          new_env = cpu_copy(env);
>>>>>   #if defined(TARGET_I386) || defined(TARGET_SPARC) ||
>>>>> defined(TARGET_PPC)
>>>>> -        cpu_state_reset(new_env);
>>>>> +        cpu_reset(ENV_GET_CPU(new_env));
>>>>>   #endif
>>>>>          /* Init regs that differ from the parent.  */
>>>>>          cpu_clone_regs(new_env, newsp);
>>>>> --
>>>>
>>>> Do you have any plans to try to rationalise the handling of reset
>>>> so that we consistently either do or don't reset the cpu here,
>>>> rather than having it done based on a TARGET_* ifdef ?
>>>
>>> Igor had an RFC for x86; sparc and ppc reset I haven't looked into yet.
>>> Cc'ing Alex and Blue.
>> I'll rebase RFC for x86 and post patches today and will remove it from here
>> by the last patch in patchset so that when this patch applied we could
>> remove
>> unnecessary call.
>> So ACK for target-i386 here.
>
> Since back then Peter and I have discussed whether we can rather just
> remove the #ifdef here and reset for all targets.
>
> Unfortunately I'm still not clear about some patches that stand in the
> way of ObjectClass::realize - if cpu_reset() is moved to realizefn for
> all targets then we can just call realize here.

I don't think we need to tangle this up with realize. What I think
we should do is:
 (1) remove the #ifdefs on the cpu reset in linux-user/main.c, so
we reset for all target CPU types
 (2) remove the cpu reset from the do_fork code in linux-user and
instead do a cpu reset inside exec.c:cpu_copy(), just after we call
cpu_init(). (This is actually fixing a bug -- the thread clone syscall
is supposed to leave register values alone, which is why we have
cpu_copy in the first place, and calling cpu_reset breaks this.)
 (3) remove any cpu reset calls from inside target cpu_init functions

This cleans things up so all targets work the same (cpu_init never
does a reset) and linux-user works the same as system mode (it's
the responsibility of the code which creates the cpu to reset it
at some point before use).

-- PMM