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

[Blue Swirl]

On Mon, May 21, 2012 at 9:09 AM, Andreas Färber <address@hidden> wrote:
> Am 14.05.2012 23:22, schrieb 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()?
> [...]
>>> 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?
>
> The TLB depends on both target_ulong and target_phys_addr_t, so we get a
> matrix of 32/32, 32/64, 64/64 at least, for which we don't have a
> common-code compilation mechanism to date.

Blah.

>> Why can't SPARCCPU contain
>> everything that CPUSPARCState had minus common items?
>
> The common items I'm working on and it became clearer that cpu_copy() is
> broken as it is, so I won't hold back that series any longer.
>
> If we move all fields from CPUSPARCState to SPARCCPU, what pointer would
> you pass as AREG0?
>
> In C code a pointer to the start of the CPU would certainly be handier
> but it was brought up that any large, rarely needed common fields at the
> start (CPUState) would contribute to imm overflows for, e.g., SPARCCPU
> fields such as GPRs etc.
>
>> Also, TCG
>> softmmu functions could use pointers to CPUTLB directly, then the slow
>> path could recover CPUState or others with container_of().
>
> Having messed in TCG code not too long ago, I would appreciate your
> help. The current ppc/ppc64 code at least relies on indexed TLB loads
> from AREG0 aka env. Pointer dereferences in generated code to get to an
> external TLB object might impact performance...

I was thinking about the reverse, TLB accesses in the fast (hit) path
would get the most direct pointer. Only in the slow path, perform
container_of(CPUXState, TLB, tlb) (or what was the syntax again).

>
>>>> Otherwise the patches look pretty safe ("if it compiles, it works").
>
> Was I supposed to interpret that as an Acked-by? ;)

No, but you may do so if it compiles. ;-)

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