Robert Foley <robert.fo...@linaro.org> writes:
> Hi,
>
> On Tue, 2 Jun 2020 at 11:46, Alex Bennée <alex.ben...@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
I could make them uint64_t and then just warn if we exceed that on start-up.
>> 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.
Yeah the option parsing is a little clunky although given the way you
pass them from the QEMU command line perhaps not too worth finessing.
The default is rw so you make a conscious decision to only care about one
or the other.
All you can really do is fail to initialise the plugin. Hopefully the
output should be enough clue.
>
> Reviewed-by: Robert Foley <robert.fo...@linaro.org>
> Tested-by: Robert Foley <robert.fo...@linaro.org>
Thanks.
Out of interest what did you measure? Are there any useful use cases you can
think of?
>
>> + 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
>>
--
Alex Bennée
