Re: [Qemu-devel] [PATCH RFC qom-cpu 39/41] target-xtensa: Introduce XtensaCPU subclasses

[Max Filippov]

On Sat, Jul 6, 2013 at 11:12 PM, Andreas Färber <address@hidden> wrote:
> Am 06.07.2013 20:39, schrieb Max Filippov:
>> On Sat, Jul 6, 2013 at 10:01 PM, Max Filippov <address@hidden> wrote:
>>> On Sat, Jul 6, 2013 at 4:55 PM, Andreas Färber <address@hidden> wrote:
>>>> Am 29.06.2013 22:01, schrieb Andreas Färber:
>>>>> Register a CPU type per core registered. Save the XtensaConfig in
>>>>> XtensaCPUClass instead of CPUXtensaState.
>>>>>
>>>>> Prepares for storing per-class GDB register count.
>>>>>
>>>>> Signed-off-by: Andreas Färber <address@hidden>
>>>>
>>>> Ping! Can you ack? (It did not seem to break your test image.)
>>>
>>> Hi Andreas,
>>>
>>> I tried make check -C tests/tcg/xtensa with the branch you recommended
>>> and it segfaults on elf loading
>>
>> ...and maybe a stupid question, but why moving configuration pointer away
>> from env and then changing every place that used to access it?
>
> Xtensa is the only target trying to implicitly access an "env" variable
> through a macro to obtain the number of registers for gdbstub. That's
> what I'm trying to fix with 40/41 in order to get rid of the #ifdeffery.
>
> The number of registers is not accessed by TCG, so it could go into
> XtensaCPU instead of CPUXtensaState.
> Further it does not change during vCPU runtime, so it no longer belongs
> in CPUXtensaState nor XtensaCPU but in XtensaCPUClass, which is shared
> among CPU cores and can be accessed statically.

I'm concerned about two things here: adding boilerplate code to access
CPU class and doing more work at runtime than just following a pointer.
I mean that env->config is almost always used together with env itself,
could we consider env->config to be a cache for xcc->config?

> However we only had one XtensaCPUClass but multiple XtensaConfigs.
> Therefore this patch registers one XtensaCPUClass per XtensaConfig.
>
> You might remember that I once tried to place the XtensaConfig fields
> directly into XtensaCPUClass but that didn't work out nicely back then.
> This is by comparison a slim/minimal conversion to subclasses, leaving
> XtensaConfig and your custom registration mechanisms in place. Cleaning
> that up to use type_init() and type_register_static() in the respective
> source files and changing -cpu ? to iterate QOM types would be a nice
> follow-up, but not needed for this gdbstub refactoring series.
>
> Another background is that I'm trying to get rid of cpu_init() and
> anything that blocks using -device or device_add with QOM CPUs.


-- 
Thanks.
-- Max