Re: [RFC PATCH v3 3/4] plugins/cache: Enabled cache parameterization

["Alex Bennée" <alex.bennee@linaro.org>]

Mahmoud Mandour <ma.mandourr@gmail.com> writes:

> Made both icache and dcache configurable through plugin arguments.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
>  contrib/plugins/cache.c | 44 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
> index 715e5443b0..d8e8c750b6 100644
> --- a/contrib/plugins/cache.c
> +++ b/contrib/plugins/cache.c
> @@ -104,8 +104,17 @@ static inline uint64_t extract_set(struct Cache *cache, uint64_t addr)
>      return (addr & cache->set_mask) >> cache->blksize_shift;
>  }
>  
> +static bool bad_cache_params(int blksize, int assoc, int cachesize)
> +{
> +    return (cachesize % blksize) != 0 || (cachesize % (blksize * assoc) != 0);
> +}
> +
>  static struct Cache *cache_init(int blksize, int assoc, int cachesize)
>  {
> +    if (bad_cache_params(blksize, assoc, cachesize)) {
> +        return NULL;
> +    }
> +
>      struct Cache *cache;
>      int i;
>      uint64_t blk_mask;
> @@ -403,8 +412,30 @@ 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=")) {
> -            limit = g_ascii_strtoull(opt + 6, NULL, 10);
> +        if (g_str_has_prefix(opt, "I=")) {
> +            gchar **toks = g_strsplit(opt + 2, " ", -1);

I don't think this works because a space will trigger the shell to split
the args - the only way I could get it to work was by quoting the whole
-plugin argument. I know the syntax of optional plugin args is ugly as
hell but this should probably use "," like hwprofile.

> +            if (g_strv_length(toks) != 3) {
> +                g_strfreev(toks);
> +                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);
> +            g_strfreev(toks);
> +        } else if (g_str_has_prefix(opt, "D=")) {
> +            gchar **toks = g_strsplit(opt + 2, " ", -1);
> +            if (g_strv_length(toks) != 3) {
> +                g_strfreev(toks);
> +                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);
> +            g_strfreev(toks);
> +        } else if (g_str_has_prefix(opt, "limit=")) {
> +            limit = g_ascii_strtoll(opt + 6, NULL, 10);
>          } else {
>              fprintf(stderr, "option parsing failed: %s\n", opt);
>              return -1;
> @@ -412,7 +443,16 @@ int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
>      }
>  
>      dcache = cache_init(dblksize, dassoc, dcachesize);
> +    if (!dcache) {
> +        fprintf(stderr, "dcache cannot be constructed from given parameters\n");
> +        return -1;
> +    }
> +

Can we give users more of a hint of what's wrong? I suspect you'll need
to factor out a validate_cache_param and return a string that describes
the failure mode. Otherwise it gets frustrating to the user.

>      icache = cache_init(iblksize, iassoc, icachesize);
> +    if (!icache) {
> +        fprintf(stderr, "icache cannot be constructed from given parameters\n");
> +        return -1;
> +    }

ditto

>  
>      qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
>      qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);


-- 
Alex Bennée