Re: [Qemu-devel] [PATCH next v2 00/74] QOM CPUState, part 3: CPU reset

["Andreas Färber" <afaerber@suse.de>]

Am 14.05.2012 21:54, schrieb Blue Swirl:
> On Mon, May 14, 2012 at 4:15 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 10.05.2012 02:13, schrieb Andreas Färber:
>>> Andreas Färber (74):
>> [...]
>>>   target-sparc: Let cpu_sparc_init() return SPARCCPU
>>>   sun4m: Use cpu_sparc_init() to obtain SPARCCPU
>>>   sun4m: Pass SPARCCPU to {main,secondary}_cpu_reset()
>>>   sun4u: Use cpu_sparc_init() to obtain SPARCCPU
>>>   sun4u: Let cpu_devinit() return SPARCCPU
>>>   sun4u: Store SPARCCPU in ResetData
>>>   leon3: Use cpu_sparc_init() to obtain SPARCCPU
>>>   leon3: Store SPARCCPU in ResetData
>> [...]

Forgot to mention the bsd-user patch.

>>>   Kill off cpu_state_reset()
>>
>> Ping! Blue, can you ack please?
> 
> What was again the purpose of all these changes, 00/74 only mentions
> killing cpu_state_reset()?

cpu_reset(CPUState *) was renamed to cpu_state_reset(CPUArchState *) to
make room for a QOM cpu_reset(CPUState *). This we have now but we still
unneededly have cpu_state_reset() and in particular we have one instance
(mips) where cpu_state_reset() does not forward to cpu_reset(). It
should on sparc. So the goal of this series is to eliminate all callers
of cpu_state_reset(). The safest and cleanest way is to remove it
completely.
The upcoming followup series, part 4, then starts moving
CPUArchState-duplicated fields from CPU_COMMON to the one CPUState, for
which reset code needs to be added that requires cpu_reset() - rather
than cpu_state_reset() - to be called in order to be executed, e.g.
halted. A preview of that can be seen on my GitHub qom-cpu branch - I
still need a QOM-safe way of calling tlb_flush() for VMState without too
much code duplication before I'll submit.

> For example two CPU types (SPARCCPU vs.
> CPUSPARCState) doesn't look very useful, is that needed?

That is outside the scope of this series but I can explain again:
CPUSPARCState is the problematic struct I'm trying to fix:

* I introduced a CPUState struct for QOM TYPE_CPU. This is compiled
twice, once for system mode, once for user mode. This struct so far is
empty. Here common fields are supposed to go (part 4), in a way that
code can be shared across targets (e.g., 13x -softmmu, 1x qom/cpu.o).

* I introduced a SPARCCPU struct for QOM TYPE_SPARC_CPU. This is a
target-specific subclass of TYPE_CPU and is what 1.1 already uses to
create the CPU state internally. This embeds CPUSPARCState.

The way QOM and most object-oriented type systems work is by placing
common fields in the front and adding new ones in the back. As pointed
out by rth this is bad for TCG because we want small offsets to
target-specific register fields for immediate load/stores. env points to
the middle of the CPU, cpu to the front, if you will. So we still need
CPUSPARCState, just not for everything.

As common or target-specific fields get moved to or added to CPUState /
SPARCPU, it becomes difficult to "jailbreak" CPUSPARCState -
sparc_env_get_cpu() and generalized ENV_GET_CPU() macro are ways to do
so. To avoid their use and the runtime casts they bring along, I am
first propagating the use of the larger type, SPARCCPU, throughout the
parts of code that will use it. CPUSPARCState can then be cheaply
accessed through cpu->env.

The purpose as before is to speed up compilation, by factoring out
common code (using CPUState) that does not need to be compiled for each
CPUxxxState with their varying field offsets, and to allow for mixing
CPU cores (with some contraints) within one executable, by making
CPUState more self-describing.

Cf. http://wiki.qemu.org/Features/QOM/CPU

CPUSPARCState is what still makes having subclasses of SPARCCPU with
additional fields tricky - they would get added to the end after some of
the large CPU_COMMON fields, so accessing them from TCG gets ugly.

And SPARCCPU is the type that gives access to SPARCCPUClass, where
constant data specific to one type can be obtained from, i.e. what is
now sparc_def_t. There we'll (you'll) need to decide whether you want to
keep it declarative, which will lead to field duplication between
sparc_def_t/SPARCCPUInfo and SPARCCPUClass due to late instantiation, or
make it imperative, which needs closer review to not introduce errors
when rewriting all the type definitions.

HTE,

Andreas

> Otherwise the patches look pretty safe ("if it compiles, it works").

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg