Re: [Qemu-devel] [PATCH v2 09/10] trace: [tcg] Add per-vCPU tracing states for events with the 'vcpu' property

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Tue, Nov 24, 2015 at 06:09:36PM +0100, Lluís Vilanova wrote:
>> +    /* Ensure 'tb_phys_idx' can encode event states as a bitmask */
>> +    bool too_many_tcg_vcpu_events[
>> +        TRACE_CPU_EVENT_COUNT > sizeof(unsigned int)*8 ? -1 : 0];

> There is a limit of 32 vcpu tcg events?  That seems low but as long as
> not too many users of this feature are merged it will work...

Well, 'tb_phys_idx' and 'tb_phys_idx_req' (just above this line) are 'unsigned
int'. They could be changed to a 64-bit index if necessary, but more than that
would require a more complex physical TB cache selection.

Fortunately, this is going to be catched at compilation time, making it safe
against crashing QEMU because of too many events of this type.


>> @@ -227,6 +228,17 @@ void cpu_dump_statistics(CPUState *cpu, FILE *f, 
>> fprintf_function cpu_fprintf,
>> void cpu_reset(CPUState *cpu)
>> {
>> CPUClass *klass = CPU_GET_CLASS(cpu);
>> +    TraceEvent *ev = NULL;
>> +
>> +    if (!qemu_initialized) {

> Is there a cleaner place to do this without introducing the
> qemu_initialized global?

> I guess the problem is that tracing itself is initialized before the
> vcpus are set up.  Is qemu_add_machine_init_done_notifier() sufficient
> for this purpose?

Right, tracing must be initialized early, while vCPUs do so much later. Also,
the hook I took for initialization is also called by regular vCPU resets and
hotplugs. The problem with machine_init is that it only works in full-system
(softmmu) mode, so it would require a separate initialization call for the user
mode variants (e.g., linux-user).

It would be much cleaner to add a trace post-initialization routine right before
main_loop/cpu_loop (doing the per-vCPU tracing state initialization). I'll
re-check the code to see if there was any other condition that made me take
'cpu_reset' instead.


Thanks,
  Lluis