Re: [Qemu-devel] [PATCH v4 12/17] trace: dynamically allocate trace_dstate in CPUState

["Lluís Vilanova" <vilanova@ac.upc.edu>]

Daniel P Berrange writes:

> The CPUState struct has a bitmap tracking which VCPU
> events are currently active. This is indexed based on
> the event ID values, and sized according the maximum
> TraceEventVCPUID enum value.

> When we start dynamically assigning IDs at runtime,
> we can't statically declare a bitmap without making
> an assumption about the max event count. This problem
> can be solved by dynamically allocating the per-CPU
> dstate bitmap.

> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qom/cpu.h | 8 +++++---
>  qom/cpu.c         | 6 +++++-
>  trace/control.c   | 5 +++++
>  trace/control.h   | 7 +++++++
>  4 files changed, 22 insertions(+), 4 deletions(-)

> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index ce0c406..ec5ad86 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -27,7 +27,6 @@
>  #include "qemu/bitmap.h"
>  #include "qemu/queue.h"
>  #include "qemu/thread.h"
> -#include "trace/generated-events.h"
 
>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,
>                                       void *opaque);
> @@ -350,8 +349,11 @@ struct CPUState {
>      struct KVMState *kvm_state;
>      struct kvm_run *kvm_run;
 
> -    /* Used for events with 'vcpu' and *without* the 'disabled' properties */
> -    DECLARE_BITMAP(trace_dstate, TRACE_VCPU_EVENT_COUNT);
> +    /* Used for events with 'vcpu' and *without* the 'disabled' properties
> +     * Dynamically allocated based on bitmap requried to hold upto

upto -> up to

For more than one line, comments are usually:

  /*
   * text...
   */


> +     * trace_get_vcpu_event_count() entries.
> +     */
> +    unsigned long *trace_dstate;
 
>      /* TODO Move common fields from CPUArchState here. */
>      int cpu_index; /* used by alpha TCG */
> diff --git a/qom/cpu.c b/qom/cpu.c
> index f783b5a..48bbf67 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -29,6 +29,7 @@
>  #include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/qdev-properties.h"
> +#include "trace/control.h"
 
>  bool cpu_exists(int64_t id)
>  {
> @@ -350,12 +351,15 @@ static void cpu_common_initfn(Object *obj)
>      qemu_mutex_init(&cpu->work_mutex);
>      QTAILQ_INIT(&cpu->breakpoints);
>      QTAILQ_INIT(&cpu->watchpoints);
> -    bitmap_zero(cpu->trace_dstate, TRACE_VCPU_EVENT_COUNT);
> +
> +    cpu->trace_dstate = bitmap_new(trace_get_vcpu_event_count());
>  }
 
>  static void cpu_common_finalize(Object *obj)
>  {
> +    CPUState *cpu = CPU(obj);
>      cpu_exec_exit(CPU(obj));
> +    g_free(cpu->trace_dstate);

Now that you factored this into the cpu var, use it to call cpu_exec_exit too.


>  }
 
>  static int64_t cpu_common_get_arch_id(CPUState *cpu)
> diff --git a/trace/control.c b/trace/control.c
> index 7be654d..9704f4d 100644
> --- a/trace/control.c
> +++ b/trace/control.c
> @@ -310,3 +310,8 @@ void trace_init_vcpu_events(void)
>          }
>      }
>  }
> +
> +uint32_t trace_get_vcpu_event_count(void)
> +{
> +    return TRACE_VCPU_EVENT_COUNT;
> +}
> diff --git a/trace/control.h b/trace/control.h
> index 45ee4fb..feb5397 100644
> --- a/trace/control.h
> +++ b/trace/control.h
> @@ -235,6 +235,13 @@ char *trace_opt_parse(const char *optarg);
>  void trace_init_vcpu_events(void);
 
 
> +/**
> + * trace_get_vcpu_event_count:
> + *
> + * Return the number of known vcpu-specific events
> + */
> +uint32_t trace_get_vcpu_event_count(void);
> +
>  #include "trace/control-internal.h"
 
>  #endif /* TRACE__CONTROL_H */
> -- 
> 2.7.4