Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Robert Foley]

Hi,

On Tue, 2 Jun 2020 at 11:46, Alex Bennée <alex.bennee@linaro.org> wrote:
<snip>
> diff --git a/tests/plugin/hwprofile.c b/tests/plugin/hwprofile.c
> new file mode 100644
> index 00000000000..f5e0639e762
> --- /dev/null
> +++ b/tests/plugin/hwprofile.c
<snip>
> +static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
> +                       uint64_t vaddr, void *udata)
> +{
> +    struct qemu_plugin_hwaddr *hwaddr = qemu_plugin_get_hwaddr(meminfo, 
> vaddr);
> +
> +    if (!hwaddr || !qemu_plugin_hwaddr_is_io(hwaddr)) {
> +        return;
> +    } else {
> +        char *name = qemu_plugin_hwaddr_device_name(hwaddr);
> +        DeviceCounts *counts;
> +
> +        g_mutex_lock(&lock);
> +        counts = (DeviceCounts *) g_hash_table_lookup(devices, name);
> +        if (!counts) {
> +            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
> +            uint64_t base = vaddr - off;
> +            counts = new_count(name, base);
> +        } else {
> +            g_free(name);
> +        }
> +
> +        if (detail) {
> +            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
> +            IOLocationCounts *io_count = 
> g_hash_table_lookup(counts->access_pattern, &off);
> +            if (!io_count) {
> +                io_count = new_location(off);
> +                g_hash_table_insert(counts->access_pattern, &off, io_count);
> +            }
> +            if (qemu_plugin_mem_is_store(meminfo)) {
> +                io_count->writes++;
> +                io_count->cpu_write |= (1 << cpu_index);
> +            } else {
> +                io_count->reads++;
> +                io_count->cpu_read |= (1 << cpu_index);
> +            }
> +        } else {
> +            if (qemu_plugin_mem_is_store(meminfo)) {
> +                counts->total_writes++;
> +                counts->cpu_write |= (1 << cpu_index);
> +            } else {
> +                counts->total_reads++;
> +                counts->cpu_read |= (1 << cpu_index);

The bitmasks cpu_read and cpu_write are ints.  Maybe to account for
larger core counts
> 32, we could assert if the cpu_index is >= 32?

> +            }
> +        }
> +        g_mutex_unlock(&lock);
> +    }
> +}
> +
> +static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> +{
> +    size_t n = qemu_plugin_tb_n_insns(tb);
> +    size_t i;
> +
> +    for (i = 0; i < n; i++) {
> +        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
> +        qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
> +                                         QEMU_PLUGIN_CB_NO_REGS,
> +                                         rw, NULL);
> +    }
> +}
> +
> +QEMU_PLUGIN_EXPORT
> +int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
> +                        int argc, char **argv)
> +{
> +    int i;
> +
> +    for (i = 0; i < argc; i++) {
> +        char *opt = argv[i];
> +        if (g_strcmp0(opt, "read") == 0) {
> +            rw = QEMU_PLUGIN_MEM_R;
> +        } else if (g_strcmp0(opt, "write") == 0) {
> +            rw = QEMU_PLUGIN_MEM_W;
> +        } else if (g_strcmp0(opt, "detail") == 0) {

When testing out the options, I noticed that
if we supply arguments of "read", and "write", then we will only get
the last one set, "write", since rw gets overwritten.
One option would be to error out if more than one of these read/write
args is supplied.

Reviewed-by: Robert Foley <robert.foley@linaro.org>
Tested-by: Robert Foley <robert.foley@linaro.org>

> +            detail = true;
> +        } else {
> +            fprintf(stderr, "option parsing failed: %s\n", opt);
> +            return -1;
> +        }
> +    }
> +
> +    plugin_init();
> +
> +    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> +    return 0;
> +}
> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
> index b3250e2504c..d87b8d40699 100644
> --- a/tests/plugin/Makefile
> +++ b/tests/plugin/Makefile
> @@ -14,6 +14,7 @@ NAMES += hotblocks
>  NAMES += howvec
>  NAMES += hotpages
>  NAMES += lockstep
> +NAMES += hwprofile
>
>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>
> --
> 2.20.1
>