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

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

On 23/12/2014 08:54, Chao Peng wrote:
> This is the xc side wrapper for XEN_SYSCTL_PSR_CMT_get_l3_event_mask
> of XEN_SYSCTL_psr_cmt_op. Additional check for event id against value
> got from this routine is also added.
>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
>   tools/libxc/include/xenctrl.h |    1 +
>   tools/libxc/xc_psr.c          |   32 ++++++++++++++++++++++++++++++++
>   2 files changed, 33 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..94c8698 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;
> +    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)
>   {
> @@ -144,6 +168,7 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
>       xc_resource_op_t op;
>       xc_resource_entry_t entries[2];
>       uint32_t evtid;
> +    uint32_t event_mask;
>       int rc;
>   
>       switch ( type )
> @@ -155,6 +180,13 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
>           return -1;
>       }
>   
> +    rc = xc_psr_cmt_get_l3_event_mask(xch, &event_mask);
> +    if ( rc < 0 )
> +        return rc;
> +
> +    if ( !(event_mask & (1 << (evtid - 1))) )
> +        return -1;
> +

This adds an extra hypercall on a common path to return a constant. As 
libxc is mostly a set of basic hypercall wrappers, I don't it should be 
validating its parameters like this.

The caller of xc_psr_cmt_get_data() must be aware of all the details, 
and whether certain events are supported or not.  I woud simply let the 
xc_resourse_op() fail (faulting on the wrmsr) if the caller passes a bad 
event id.

~Andrew

>       entries[0].u.cmd = XEN_RESOURCE_OP_MSR_WRITE;
>       entries[0].idx = MSR_IA32_CMT_EVTSEL;
>       entries[0].val = (uint64_t)rmid << 32 | evtid;