[v4,11/20] instrument: Track vCPUs

Commit Message

Lluís Vilanova Sept. 6, 2017, 6:07 p.m. UTC

Keep a translation between instrumentation's QICPU and CPUState objects to avoid
exposing QEMU's internals to instrumentation clients.

Signed-off-by: Lluís Vilanova <[e-mail]>
---
 cpus-common.c            |    9 +++++++++
 instrument/control.c     |   22 ++++++++++++++++++++++
 instrument/control.h     |   32 ++++++++++++++++++++++++++++++++
 instrument/control.inc.h |   23 +++++++++++++++++++++++
 4 files changed, 86 insertions(+)

Comments

Emilio Cota Sept. 6, 2017, 9:36 p.m. UTC | #1

On Wed, Sep 06, 2017 at 21:07:06 +0300, Lluís Vilanova wrote:
> Keep a translation between instrumentation's QICPU and CPUState objects to avoid
> exposing QEMU's internals to instrumentation clients.
> 
> Signed-off-by: Lluís Vilanova <[e-mail]>
(snip)
> diff --git a/instrument/control.c b/instrument/control.c
> index 2c2781beeb..83453ea561 100644
> --- a/instrument/control.c
> +++ b/instrument/control.c
> @@ -13,10 +13,32 @@
>  #include "instrument/load.h"
>  #include "instrument/qemu-instr/control.h"
>  #include "instrument/qemu-instr/visibility.h"
> +#include "qom/cpu.h"
> +
>  
>  __thread InstrState instr_cur_state;
>  
>  
> +unsigned int instr_cpus_count;
> +CPUState **instr_cpus;
> +
> +void instr_cpu_add(CPUState *vcpu)
> +{
> +    unsigned int idx = vcpu->cpu_index;
> +    if (idx >= instr_cpus_count) {
> +        instr_cpus_count = idx + 1;
> +        instr_cpus = realloc(instr_cpus, sizeof(*instr_cpus) * instr_cpus_count);
> +    }
> +    instr_cpus[idx] = vcpu;
> +}
> +
> +void instr_cpu_remove(CPUState *vcpu)
> +{
> +    unsigned int idx = vcpu->cpu_index;
> +    instr_cpus[idx] = NULL;
> +}

instr_cpus_count and instr_cpus are both modified with cpu_list_lock.
However, other readers do not acquire this same lock. This gets messy
when you try to implement something like "vcpu_for_each", which is
very useful when you load an instrumentation tool once the program
is running (otherwise the tool cannot know for sure what the vCPUs
are). This vcpu_for_each would also have to take cpu_list_lock, for
otherwise it'd miss new threads being added. As you can imagine this
gets messy pretty quickly, given that you have your own lock here.
I went with having my own hash table (a list would be fine too),
protected with the same "instr_lock" you have (i.e. the recursive mutex).

An unrelated comment: your usage of get/set/put confuses me. I'd expect those
to work on refcounted items; instead you use them for instance to hide a cast
(cpu_set) or to create/destroy (e.g. handle_get/put).

		E.

Patch

diff --git a/cpus-common.c b/cpus-common.c
index 59f751ecf9..ec5f46cc3d 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -22,6 +22,9 @@ 
 #include "exec/cpu-common.h"
 #include "qom/cpu.h"
 #include "sysemu/cpus.h"
+#if defined(CONFIG_INSTRUMENT)
+#include "instrument/control.h"
+#endif
 
 static QemuMutex qemu_cpu_list_lock;
 static QemuCond exclusive_cond;
@@ -84,6 +87,9 @@  void cpu_list_add(CPUState *cpu)
     } else {
         assert(!cpu_index_auto_assigned);
     }
+#if defined(CONFIG_INSTRUMENT)
+    instr_cpu_add(cpu);
+#endif
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
     qemu_mutex_unlock(&qemu_cpu_list_lock);
 
@@ -102,6 +108,9 @@  void cpu_list_remove(CPUState *cpu)
     assert(!(cpu_index_auto_assigned && cpu != QTAILQ_LAST(&cpus, CPUTailQ)));
 
     QTAILQ_REMOVE(&cpus, cpu, node);
+#if defined(CONFIG_INSTRUMENT)
+    instr_cpu_remove(cpu);
+#endif
     cpu->cpu_index = UNASSIGNED_CPU_INDEX;
     qemu_mutex_unlock(&qemu_cpu_list_lock);
 }
diff --git a/instrument/control.c b/instrument/control.c
index 2c2781beeb..83453ea561 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -13,10 +13,32 @@ 
 #include "instrument/load.h"
 #include "instrument/qemu-instr/control.h"
 #include "instrument/qemu-instr/visibility.h"
+#include "qom/cpu.h"
+
 
 __thread InstrState instr_cur_state;
 
 
+unsigned int instr_cpus_count;
+CPUState **instr_cpus;
+
+void instr_cpu_add(CPUState *vcpu)
+{
+    unsigned int idx = vcpu->cpu_index;
+    if (idx >= instr_cpus_count) {
+        instr_cpus_count = idx + 1;
+        instr_cpus = realloc(instr_cpus, sizeof(*instr_cpus) * instr_cpus_count);
+    }
+    instr_cpus[idx] = vcpu;
+}
+
+void instr_cpu_remove(CPUState *vcpu)
+{
+    unsigned int idx = vcpu->cpu_index;
+    instr_cpus[idx] = NULL;
+}
+
+
 qi_fini_fn instr_event__fini_fn;
 void *instr_event__fini_data;
 
diff --git a/instrument/control.h b/instrument/control.h
index f2b085f69b..0c37692465 100644
--- a/instrument/control.h
+++ b/instrument/control.h
@@ -10,6 +10,38 @@ 
 #ifndef INSTRUMENT__CONTROL_H
 #define INSTRUMENT__CONTROL_H
 
+#include "qemu/typedefs.h"
+#include "instrument/qemu-instr/types.h"
+
+
+/**
+ * instr_cpu_add:
+ *
+ * Make @vcpu available to instrumentation clients.
+ */
+void instr_cpu_add(CPUState *vcpu);
+
+/**
+ * instr_cpu_remove:
+ *
+ * Make @vcpu unavailable to instrumentation clients.
+ */
+void instr_cpu_remove(CPUState *vcpu);
+
+/**
+ * instr_cpu_get:
+ *
+ * Get the #CPUState corresponding to the given #QICPU.
+ */
+static inline CPUState *instr_cpu_get(QICPU vcpu);
+
+/**
+ * instr_cpu_set:
+ *
+ * Get the #QICPU corresponding to the given #CPUState.
+ */
+static inline QICPU instr_cpu_set(CPUState *vcpu);
+
 
 /**
  * InstrState:
diff --git a/instrument/control.inc.h b/instrument/control.inc.h
index 0f649f4caa..18ae6a34cc 100644
--- a/instrument/control.inc.h
+++ b/instrument/control.inc.h
@@ -7,9 +7,12 @@ 
  * See the COPYING file in the top-level directory.
  */
 
+#include "qemu/osdep.h"
 #include "qemu/atomic.h"
 #include "qemu/compiler.h"
+#include "qom/cpu.h"
 #include <stdbool.h>
+#include <stdint.h>
 
 
 extern __thread InstrState instr_cur_state;
@@ -23,3 +26,23 @@  static inline InstrState instr_get_state(void)
 {
     return atomic_load_acquire(&instr_cur_state);
 }
+
+
+extern unsigned int instr_cpus_count;
+extern CPUState **instr_cpus;
+
+static inline CPUState *instr_cpu_get(QICPU vcpu)
+{
+    unsigned int idx = (uintptr_t)vcpu;
+    if (idx >= instr_cpus_count) {
+        return NULL;
+    } else {
+        return instr_cpus[idx];
+    }
+}
+
+static inline QICPU instr_cpu_set(CPUState *vcpu)
+{
+    uintptr_t idx = vcpu->cpu_index;
+    return (QICPU )idx;
+}