Re: [Qemu-devel] [PATCH for-1.4 1/2] linux-user: Fix cpu_copy() usage

[Igor Mammedov]

On Wed, 30 Jan 2013 09:53:29 +0100
Andreas Färber <address@hidden> wrote:

> Am 30.01.2013 09:46, schrieb Igor Mammedov:
> > On Wed, 30 Jan 2013 08:46:34 +0100
> > Andreas Färber <address@hidden> wrote:
> > 
> >> Am 30.01.2013 08:18, schrieb Igor Mammedov:
> >>> On Wed, 30 Jan 2013 01:34:18 +0100
> >>> Andreas Färber <address@hidden> 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 <address@hidden>
> >>>> Signed-off-by: Andreas Färber <address@hidden>
> >>>> Cc: Peter Maydell <address@hidden>
> >>>> ---
> >>>>  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 <address@hidden>

> 
> Nothing breaks any more than it is by not applying this.
> 
> Andreas
>