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

[Igor Mammedov]

On 05/16/2012 03:01 PM, Andreas Färber 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.

1. I'd like to have cpu_reset in realizefn - which is kind of equivalent
of cpu power-on, after which cpu should be in known state
(i.e. the state after reset, at least for target-i386).

2. cpu_reset in realizefn as well will free us from calling cpu_reset
in device_add when implementing hotplug and will allow to remove it from
hw/pc.c.

3. regarding exec.c:cpu_copy and do_fork on target-i386
 cpu_copy first creates cpu then copies all over it CPUArchState of original
 cpu perserving only cpu_index. It's beyond my understanding why
 anyone would/need do this.
 So I'll not touch cpu_reset in do_fork since I haven't a clue what's
 going on. Albeit adding cpu_reset in realizefn shouldn't break anything here.

> Actually, all we'd need is ObjectClass::realize field, so I'm
> considering extracting the intersection between Paolo and me, stick a
> CPU-specific wrapper method on top as requested by Anthony and then we
> can move ahead here...
>
> Andreas

--
-----
 Igor