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

[Blue Swirl]

On Mon, May 14, 2012 at 8:59 PM, Andreas Färber <address@hidden> wrote:
> Am 14.05.2012 21:54, schrieb Blue Swirl:
>> On Mon, May 14, 2012 at 4:15 PM, Andreas Färber <address@hidden> 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.

Excellent goals!

>
> 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.

Can't we move the TLB to common code? Why can't SPARCCPU contain
everything that CPUSPARCState had minus common items? Also, TCG
softmmu functions could use pointers to CPUTLB directly, then the slow
path could recover CPUState or others with container_of().

> 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