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

[Chao Peng <chao.p.peng@linux.intel.com>]

On Tue, Dec 23, 2014 at 03:46:41PM +0000, Andrew Cooper wrote:
> 
> 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.
> 
Sounds reasonable. I'd move the validation to the caller, that's, the xl side.
Thanks 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;
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel