[Qemu-devel] [PULL 08/25] qht: do not segfault when gathering stats from an uninitialized qht

[Paolo Bonzini]

From: "Emilio G. Cota" <address@hidden>

So far, QHT functions assume that the passed qht has previously been
initialized--otherwise they segfault.

This patch makes an exception for qht_statistics_init, with the goal
of simplifying calling code. For instance, qht_statistics_init is
called from the 'info jit' dump, and given that under KVM the TB qht
is never initialized, we get a segfault. Thus, instead of complicating
the 'info jit' code with additional checks, let's allow passing an
uninitialized qht to qht_statistics_init.

While at it, add a test for this to test-qht.

Before the patch (for $ qemu -enable-kvm [...]):
(qemu) info jit
[...]
direct jump count   0 (0%) (2 jumps=0 0%)
Program received signal SIGSEGV, Segmentation fault.

After the patch the "TB hash buckets", "TB hash occupancy"
and "TB hash avg chain" lines are omitted.
(qemu) info jit
[...]
direct jump count   0 (0%) (2 jumps=0 0%)
TB hash buckets     0/0 (-nan% head buckets used)
TB hash occupancy   nan% avg chain occ. Histogram: (null)
TB hash avg chain   nan buckets. Histogram: (null)
[...]

Reported by: Changlong Xie <address@hidden>
Signed-off-by: Emilio G. Cota <address@hidden>
Message-Id: <address@hidden>
[Extract printing statistics to an entirely separate function. - Paolo]
Signed-off-by: Paolo Bonzini <address@hidden>
---
 tests/test-qht.c |  4 ++++
 translate-all.c  | 70 +++++++++++++++++++++++++++++++-------------------------
 util/qht.c       |  7 +++++-
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/tests/test-qht.c b/tests/test-qht.c
index f1d6283..46a64b6 100644
--- a/tests/test-qht.c
+++ b/tests/test-qht.c
@@ -95,8 +95,12 @@ static void iter_check(unsigned int count)
 
 static void qht_do_test(unsigned int mode, size_t init_entries)
 {
+    /* under KVM we might fetch stats from an uninitialized qht */
+    check_n(0);
+
     qht_init(&ht, 0, mode);
 
+    check_n(0);
     insert(0, N);
     check(0, N, true);
     check_n(N);
diff --git a/translate-all.c b/translate-all.c
index 0d47c1c..efeba29 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1663,15 +1663,50 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong 
addr)
            TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
 }
 
+static void print_qht_statistics(FILE *f, fprintf_function cpu_fprintf,
+                                 struct qht_stats hst)
+{
+    uint32_t hgram_opts;
+    size_t hgram_bins;
+    char *hgram;
+
+    if (!hst.head_buckets) {
+        return;
+    }
+    cpu_fprintf(f, "TB hash buckets     %zu/%zu (%0.2f%% head buckets used)\n",
+                hst.used_head_buckets, hst.head_buckets,
+                (double)hst.used_head_buckets / hst.head_buckets * 100);
+
+    hgram_opts =  QDIST_PR_BORDER | QDIST_PR_LABELS;
+    hgram_opts |= QDIST_PR_100X   | QDIST_PR_PERCENT;
+    if (qdist_xmax(&hst.occupancy) - qdist_xmin(&hst.occupancy) == 1) {
+        hgram_opts |= QDIST_PR_NODECIMAL;
+    }
+    hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
+    cpu_fprintf(f, "TB hash occupancy   %0.2f%% avg chain occ. Histogram: 
%s\n",
+                qdist_avg(&hst.occupancy) * 100, hgram);
+    g_free(hgram);
+
+    hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
+    hgram_bins = qdist_xmax(&hst.chain) - qdist_xmin(&hst.chain);
+    if (hgram_bins > 10) {
+        hgram_bins = 10;
+    } else {
+        hgram_bins = 0;
+        hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
+    }
+    hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
+    cpu_fprintf(f, "TB hash avg chain   %0.3f buckets. Histogram: %s\n",
+                qdist_avg(&hst.chain), hgram);
+    g_free(hgram);
+}
+
 void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
 {
     int i, target_code_size, max_target_code_size;
     int direct_jmp_count, direct_jmp2_count, cross_page;
     TranslationBlock *tb;
     struct qht_stats hst;
-    uint32_t hgram_opts;
-    size_t hgram_bins;
-    char *hgram;
 
     target_code_size = 0;
     max_target_code_size = 0;
@@ -1724,34 +1759,7 @@ void dump_exec_info(FILE *f, fprintf_function 
cpu_fprintf)
                         tcg_ctx.tb_ctx.nb_tbs : 0);
 
     qht_statistics_init(&tcg_ctx.tb_ctx.htable, &hst);
-
-    cpu_fprintf(f, "TB hash buckets     %zu/%zu (%0.2f%% head buckets used)\n",
-                hst.used_head_buckets, hst.head_buckets,
-                (double)hst.used_head_buckets / hst.head_buckets * 100);
-
-    hgram_opts =  QDIST_PR_BORDER | QDIST_PR_LABELS;
-    hgram_opts |= QDIST_PR_100X   | QDIST_PR_PERCENT;
-    if (qdist_xmax(&hst.occupancy) - qdist_xmin(&hst.occupancy) == 1) {
-        hgram_opts |= QDIST_PR_NODECIMAL;
-    }
-    hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
-    cpu_fprintf(f, "TB hash occupancy   %0.2f%% avg chain occ. Histogram: 
%s\n",
-                qdist_avg(&hst.occupancy) * 100, hgram);
-    g_free(hgram);
-
-    hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
-    hgram_bins = qdist_xmax(&hst.chain) - qdist_xmin(&hst.chain);
-    if (hgram_bins > 10) {
-        hgram_bins = 10;
-    } else {
-        hgram_bins = 0;
-        hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
-    }
-    hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
-    cpu_fprintf(f, "TB hash avg chain   %0.3f buckets. Histogram: %s\n",
-                qdist_avg(&hst.chain), hgram);
-    g_free(hgram);
-
+    print_qht_statistics(f, cpu_fprintf, hst);
     qht_statistics_destroy(&hst);
 
     cpu_fprintf(f, "\nStatistics:\n");
diff --git a/util/qht.c b/util/qht.c
index 28ce289..16a8d79 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -789,11 +789,16 @@ void qht_statistics_init(struct qht *ht, struct qht_stats 
*stats)
 
     map = atomic_rcu_read(&ht->map);
 
-    stats->head_buckets = map->n_buckets;
     stats->used_head_buckets = 0;
     stats->entries = 0;
     qdist_init(&stats->chain);
     qdist_init(&stats->occupancy);
+    /* bail out if the qht has not yet been initialized */
+    if (unlikely(map == NULL)) {
+        stats->head_buckets = 0;
+        return;
+    }
+    stats->head_buckets = map->n_buckets;
 
     for (i = 0; i < map->n_buckets; i++) {
         struct qht_bucket *head = &map->buckets[i];
-- 
2.7.4