Re: [Qemu-devel] [PATCH 19/24] qdev: move reset handler list from vl.c to qdev.c

[Peter Maydell]

On 2 December 2012 05:44, Andreas Färber <address@hidden> wrote:
> Am 01.12.2012 12:26, schrieb Peter Maydell:
>> On 30 November 2012 21:38, Eduardo Habkost <address@hidden> wrote:
>>> cpu_reset() is not that well-defined, otherwise we wouldn't have this on
>>> linux-user:
>>>
>>> #if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
>>>     cpu_reset(ENV_GET_CPU(env));
>>> #endif
>>>
>>> (I have no idea why we have that #ifdef).
>>
>> I think this is because the different targets disagree about whether
>> the CPU should be reset on initial construction or whether it needs
>> a specific reset call. (The current setup with #ifdefs is among other
>> things a historical effect as a result of various refactorings in
>> the past; you can trace the git history if you're interested.)
>
> Peter and me had long IRC discussions about how to fix this in the past:
>
> * On my qom-cpu-copy branch I have a patch queued that drops the
>   #ifdef above, accepting that CPUs may get reset twice then.
>   => Dispels doubt for target authors; doubts about correctness though.

So on its own, this will break things, because the cpu_reset() happens
after the cpu_copy, so we'll end up not in fact copying most of the
registers across from the original thread. In fact this looks broken
for i386/sparc/ppc at the moment, and is probably only working to the
extent that user programs don't actually care about the register state
across the clone() syscall. [and i386 threads are busted anyhow.]
If we want to retain cpu_copy for the moment, the correct place for
the cpu_reset() is in cpu_copy() itself, after the cpu_init but before
the memcpy. [I see actually you mention this later in your email.]

> * PMM suggested to move cpu_clone_regs() from target-*/cpu.h to *-user/.
>   => Would lead to duplication between linux-user and bsd-user; ABI?

clone_regs should definitely be in *-user/, because it is in fact
ABI dependent -- which register the 0 return value from the clone()
syscall ends up in is up to the ABI. Also, there won't in fact be
any duplication problem here because bsd-user doesn't implement
threads and doesn't call cpu_clone_regs().

> * PMM suggested to replace cpu_copy() with ABI-specific code in *-user/.
>   Unfortunately I don't quite remember the details of how... ;)

I don't know that I got as far as the details, but the general idea
is that the CPU state that needs to be propagated to the CPU in the
new thread is only the user-space-facing bits, which *-user/ already
needs to know about (and fish directly in CPUState for) to get things
like signal handlers right. So rather than doing a huge memcpy of
all the CPU's internal state, we just create and reset a CPU as normal,
and then have the *-user/ code copy across those bits of register
state which matter to userspace. I think this is much cleaner because
it means the CPU emulation itself has a simple interface ("create
cpu" and "reset cpu") and the details of copying thread state across
are restricted to the user-mode code.

-- PMM