Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/01/15 11:12, Chao Peng wrote:
> This is the tools side wrapper for XEN_SYSCTL_PSR_CMT_get_l3_event_mask
> of XEN_SYSCTL_psr_cmt_op.
>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
>  tools/libxc/include/xenctrl.h |    1 +
>  tools/libxc/xc_psr.c          |   24 ++++++++++++++++++++++++
>  tools/libxl/libxl.h           |    1 +
>  tools/libxl/libxl_psr.c       |   18 ++++++++++++++++++
>  4 files changed, 44 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 0ad8b8d..96b357c 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2697,6 +2697,7 @@ int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
>  int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid);
>  int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
>      uint32_t *upscaling_factor);
> +int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask);
>  int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
>      uint32_t *l3_cache_size);
>  int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
> diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
> index 872e6dc..e76a0f9 100644
> --- a/tools/libxc/xc_psr.c
> +++ b/tools/libxc/xc_psr.c
> @@ -112,6 +112,30 @@ int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
>      return rc;
>  }
>  
> +int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask)
> +{
> +    static int val = 0;

This should be uint32_t rather than int.

I am somewhat concerned about multithreaded use of libxc, but this is
not the first issue in libxc, and probably shouldn't be held against
this patch.  As the result of the hypercall is going to be the same, the
worse that a race could achieve is a wasted hypercall.

> +    int rc;
> +    DECLARE_SYSCTL;
> +
> +    if ( val )
> +    {
> +        *event_mask = val;
> +        return 0;
> +    }
> +
> +    sysctl.cmd = XEN_SYSCTL_psr_cmt_op;
> +    sysctl.u.psr_cmt_op.cmd =
> +        XEN_SYSCTL_PSR_CMT_get_l3_event_mask;
> +    sysctl.u.psr_cmt_op.flags = 0;
> +
> +    rc = xc_sysctl(xch, &sysctl);
> +    if ( !rc )
> +        val = *event_mask = sysctl.u.psr_cmt_op.u.data;
> +
> +    return rc;
> +}
> +
>  int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
>                                        uint32_t *l3_cache_size)
>  {
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 0a123f1..42ace76 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1453,6 +1453,7 @@ int libxl_psr_cmt_attach(libxl_ctx *ctx, uint32_t domid);
>  int libxl_psr_cmt_detach(libxl_ctx *ctx, uint32_t domid);
>  int libxl_psr_cmt_domain_attached(libxl_ctx *ctx, uint32_t domid);
>  int libxl_psr_cmt_enabled(libxl_ctx *ctx);
> +int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type);
>  int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid);
>  int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx, uint32_t socketid,
>      uint32_t *l3_cache_size);
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 0437465..3018a0d 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -120,6 +120,24 @@ int libxl_psr_cmt_enabled(libxl_ctx *ctx)
>      return xc_psr_cmt_enabled(ctx->xch);
>  }
>  
> +int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type)
> +{
> +    GC_INIT(ctx);
> +    uint32_t event_mask;
> +    int ret;

The libxl CODING_SYTLE states that this "ret" should be "rc"

~Andrew

> +
> +    ret = xc_psr_cmt_get_l3_event_mask(ctx->xch, &event_mask);
> +    if (ret < 0) {
> +        libxl__psr_cmt_log_err_msg(gc, errno);
> +        ret = 0;
> +    } else {
> +        ret = event_mask & (1 << (type - 1));
> +    }
> +
> +    GC_FREE;
> +    return ret;
> +}
> +
>  int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid)
>  {
>      GC_INIT(ctx);