On Tue, Jun 1, 2021 at 1:29 PM Alex Bennée wrote: > > (Stefan CC'ed for tracing discussion) > > Mahmoud Mandour writes: > > > Made both icache and dcache configurable through plugin arguments > > and added memory trace printing in a separate file. > > Please keep the commits discreet and single topic. The memory trace is > an extra feature so should be in it's own commit. > > > > > Signed-off-by: Mahmoud Mandour > > --- > > contrib/plugins/cache.c | 68 +++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 66 insertions(+), 2 deletions(-) > > > > diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c > > index 8c9d1dd538..fa0bf1dd40 100644 > > --- a/contrib/plugins/cache.c > > +++ b/contrib/plugins/cache.c > > @@ -22,7 +22,7 @@ static GRand *rng; > > static GHashTable *dmiss_ht; > > static GHashTable *imiss_ht; > > > > -static GMutex dmtx, imtx; > > +static GMutex dmtx, imtx, fmtx; > > > > static int limit; > > static bool sys; > > @@ -33,6 +33,8 @@ static uint64_t dmisses; > > static uint64_t imem_accesses; > > static uint64_t imisses; > > > > +FILE *tracefile; > > + > > static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW; > > > > enum AccessResult { > > @@ -205,6 +207,16 @@ static void vcpu_mem_access(unsigned int cpu_index, > qemu_plugin_meminfo_t info, > > insn_addr = ((struct InsnData *) userdata)->addr; > > effective_addr = hwaddr ? qemu_plugin_hwaddr_phys_addr(hwaddr) : > vaddr; > > > > + if (tracefile) { > > + g_mutex_lock(&fmtx); > > + g_autoptr(GString) rep = g_string_new(""); > > + bool is_store = qemu_plugin_mem_is_store(info); > > + g_string_append_printf(rep, "%c: 0x%" PRIx64, > > + is_store ? 'S' : 'L', effective_addr); > > + fprintf(tracefile, "%s\n", rep->str); > > + g_mutex_unlock(&fmtx); > > + } > > I can see this would be useful for debugging but I'm wary of adding > ad-hoc tracing formats when QEMU already has support for a wide range of > tracing formats. We discussed this a bit in: > > Subject: trace_FOO_tcg bit-rotted? > Date: Tue, 06 Apr 2021 17:00:20 +0100 > Message-ID: <87eefnwd0l.fsf@linaro.org> > > However I don't know how easy it would be to leverage the existing > tracing infrastructure from inside a plugin. As I understand it QEMU > currently builds a static list of trace points during the build so maybe > we would need additional infrastructure for a plugin to register a trace > point and for the final output to be use-able. For example the binary > trace output I think still needs to reference the source trace-events > file? > > So that's not a NACK but maybe we could spend a little time working out > if we can come up with a cleaner solution? Alright then, I will have it removed for now and maybe add it if there's a better solution for it. > > Stefan, any thoughts? > > > if (access_cache(dcache, effective_addr) == MISS) { > > struct InsnData *insn = get_or_create(dmiss_ht, userdata, > insn_addr); > > insn->misses++; > > @@ -221,11 +233,20 @@ static void vcpu_insn_exec(unsigned int > vcpu_index, void *userdata) > > g_mutex_lock(&imtx); > > addr = ((struct InsnData *) userdata)->addr; > > > > + if (tracefile) { > > + g_mutex_lock(&fmtx); > > + g_autoptr(GString) rep = g_string_new(""); > > + g_string_append_printf(rep, "I: 0x%" PRIx64, addr); > > + fprintf(tracefile, "%s\n", rep->str); > > + g_mutex_unlock(&fmtx); > > + } > > + > > if (access_cache(icache, addr) == MISS) { > > struct InsnData *insn = get_or_create(imiss_ht, userdata, addr); > > insn->misses++; > > imisses++; > > } > > + > > imem_accesses++; > > g_mutex_unlock(&imtx); > > } > > @@ -352,6 +373,15 @@ static void plugin_exit() > > > > g_mutex_unlock(&dmtx); > > g_mutex_unlock(&imtx); > > + > > + if (tracefile) { > > + fclose(tracefile); > > + } > > +} > > + > > +static bool bad_cache_params(int blksize, int assoc, int cachesize) > > +{ > > + return (cachesize % blksize) != 0 || (cachesize % (blksize * assoc) > != 0); > > } > > > > QEMU_PLUGIN_EXPORT > > @@ -377,14 +407,48 @@ int qemu_plugin_install(qemu_plugin_id_t id, const > qemu_info_t *info, > > > > for (i = 0; i < argc; i++) { > > char *opt = argv[i]; > > - if (g_str_has_prefix(opt, "limit=")) { > > + if (g_str_has_prefix(opt, "I=")) { > > + gchar **toks = g_strsplit(opt + 2, " ", -1); > > + if (g_strv_length(toks) != 3) { > > + fprintf(stderr, "option parsing failed: %s\n", opt); > > + return -1; > > + } > > + icachesize = g_ascii_strtoull(toks[0], NULL, 10); > > + iassoc = g_ascii_strtoull(toks[1], NULL, 10); > > + iblksize = g_ascii_strtoull(toks[2], NULL, 10); > > + } else if (g_str_has_prefix(opt, "D=")) { > > + gchar **toks = g_strsplit(opt + 2, " ", -1); > > + if (g_strv_length(toks) != 3) { > > + fprintf(stderr, "option parsing failed: %s\n", opt); > > + return -1; > > + } > > + dcachesize = g_ascii_strtoull(toks[0], NULL, 10); > > + dassoc = g_ascii_strtoull(toks[1], NULL, 10); > > + dblksize = g_ascii_strtoull(toks[2], NULL, 10); > > + } else if (g_str_has_prefix(opt, "limit=")) { > > limit = g_ascii_strtoull(opt + 6, NULL, 10); > > + } else if (g_str_has_prefix(opt, "tracefile=")) { > > + char *file_name = opt + 10; > > + tracefile = fopen(file_name, "w"); > > + if (!tracefile) { > > + fprintf(stderr, "could not open: %s for writing\n", > file_name); > > + } > > } else { > > fprintf(stderr, "option parsing failed: %s\n", opt); > > return -1; > > } > > } > > > > + if (bad_cache_params(iblksize, iassoc, icachesize)) { > > + fprintf(stderr, "icache cannot be constructed from given > parameters\n"); > > + return -1; > > + } > > + > > + if (bad_cache_params(dblksize, dassoc, dcachesize)) { > > + fprintf(stderr, "dcache cannot be constructed from given > parameters\n"); > > + return -1; > > + } > > + > > Perhaps roll bad_cache_params into cache_init and return NULL if it > fails, so: > > dcache = cache_init(dblksize, dassoc, dcachesize); > if (!dcache) { > fprintf(stderr, "dcache cannot be constructed from given > parameters\n"); > return -1; > } Applied, thanks > > > dcache = cache_init(dblksize, dassoc, dcachesize); > > icache = cache_init(iblksize, iassoc, icachesize); > > > -- > Alex Bennée >