Re: [Xen-devel] [PATCH v2 09/15] tools: implement the new libxc get hw info interface

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 24, 2017 at 09:14:43AM +0800, Yi Sun wrote:
> This patch implements a new libxc get hw info interface and corresponding
> data structures. It also changes libxl_psr.c to call this new interface.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> ---
> v2:
>     - split this patch out from a big patch in v1.
>       (suggested by Wei Liu)
>     - change 'CAT_INFO' and 'MBA_INFO' to 'CAT' and 'MBA'.
>       (suggested by Chao Peng)
> ---
>  tools/libxc/include/xenctrl.h | 30 +++++++++++++++++++++++++---
>  tools/libxc/xc_psr.c          | 46 
> ++++++++++++++++++++++++++++++++-----------
>  tools/libxl/libxl_psr.c       | 34 ++++++++++++++++++++++++++++++--
>  3 files changed, 93 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index c7710b8..81a6f48 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2458,6 +2458,31 @@ enum xc_psr_cat_type {
>  };
>  typedef enum xc_psr_cat_type xc_psr_cat_type;
>  
> +enum xc_psr_feat_type {
> +    XC_PSR_FEAT_UNKNOWN,
> +    XC_PSR_FEAT_CAT_L3,
> +    XC_PSR_FEAT_CAT_L2,
> +    XC_PSR_FEAT_MBA,
> +};
> +typedef enum xc_psr_feat_type xc_psr_feat_type;
> +
> +struct xc_psr_hw_info {
> +    union {
> +        struct {
> +            uint32_t cos_max;
> +            uint32_t cbm_len;
> +            bool     cdp_enabled;
> +        } xc_cat_info;
> +
> +        struct {
> +            uint32_t cos_max;
> +            uint32_t thrtl_max;
> +            bool     linear;
> +        } xc_mba_info;

No need for the _info suffix. The type itself already has info in it's
name.

> +    } u;
> +};
> +typedef struct xc_psr_hw_info xc_psr_hw_info;
> +
>  int xc_psr_cmt_attach(xc_interface *xch, uint32_t domid);
>  int xc_psr_cmt_detach(xc_interface *xch, uint32_t domid);
>  int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
> @@ -2479,9 +2504,8 @@ int xc_psr_cat_set_domain_data(xc_interface *xch, 
> uint32_t domid,
>  int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
>                                 xc_psr_cat_type type, uint32_t target,
>                                 uint64_t *data);
> -int xc_psr_cat_get_info(xc_interface *xch, uint32_t socket, unsigned int lvl,
> -                        uint32_t *cos_max, uint32_t *cbm_len,
> -                        bool *cdp_enabled);
> +int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
> +                       xc_psr_feat_type type, xc_psr_hw_info *hw_info);
>  
>  int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
>  int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
> diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
> index 73d05f2..ba412e4 100644
> --- a/tools/libxc/xc_psr.c
> +++ b/tools/libxc/xc_psr.c
> @@ -323,36 +323,58 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, 
> uint32_t domid,
>      return rc;
>  }
>  
> -int xc_psr_cat_get_info(xc_interface *xch, uint32_t socket, unsigned int lvl,
> -                        uint32_t *cos_max, uint32_t *cbm_len, bool 
> *cdp_enabled)
> +int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
> +                       xc_psr_feat_type type, xc_psr_hw_info *hw_info)
>  {
>      int rc = -1;
>      DECLARE_SYSCTL;
>  
> +    if ( !hw_info )
> +        return rc;

You should set errno = EINVAL here.

> +
>      sysctl.cmd = XEN_SYSCTL_psr_alloc_op;
>      sysctl.u.psr_alloc_op.target = socket;
>  
> -    switch ( lvl )
> +    switch ( type )
>      {
> -    case 2:
> +    case XC_PSR_FEAT_CAT_L2:
>          sysctl.u.psr_alloc_op.cmd = XEN_SYSCTL_PSR_CAT_get_l2_info;
>          rc = xc_sysctl(xch, &sysctl);
>          if ( !rc )
>          {
> -            *cos_max = sysctl.u.psr_alloc_op.u.cat_info.cos_max;
> -            *cbm_len = sysctl.u.psr_alloc_op.u.cat_info.cbm_len;
> -            *cdp_enabled = false;
> +            hw_info->u.xc_cat_info.cos_max =
> +                        sysctl.u.psr_alloc_op.u.cat_info.cos_max;
> +            hw_info->u.xc_cat_info.cbm_len =
> +                        sysctl.u.psr_alloc_op.u.cat_info.cbm_len;
> +            hw_info->u.xc_cat_info.cdp_enabled = false;
>          }
>          break;
> -    case 3:
> +    case XC_PSR_FEAT_CAT_L3:
>          sysctl.u.psr_alloc_op.cmd = XEN_SYSCTL_PSR_CAT_get_l3_info;
>          rc = xc_sysctl(xch, &sysctl);
>          if ( !rc )
>          {
> -            *cos_max = sysctl.u.psr_alloc_op.u.cat_info.cos_max;
> -            *cbm_len = sysctl.u.psr_alloc_op.u.cat_info.cbm_len;
> -            *cdp_enabled = sysctl.u.psr_alloc_op.u.cat_info.flags &
> -                           XEN_SYSCTL_PSR_CAT_L3_CDP;
> +            hw_info->u.xc_cat_info.cos_max =
> +                        sysctl.u.psr_alloc_op.u.cat_info.cos_max;
> +            hw_info->u.xc_cat_info.cbm_len =
> +                        sysctl.u.psr_alloc_op.u.cat_info.cbm_len;
> +            hw_info->u.xc_cat_info.cdp_enabled =
> +                        sysctl.u.psr_alloc_op.u.cat_info.flags &
> +                        XEN_SYSCTL_PSR_CAT_L3_CDP;
> +        }
> +        break;
> +    case XC_PSR_FEAT_MBA:
> +        sysctl.u.psr_alloc_op.cmd = XEN_SYSCTL_PSR_MBA_get_info;
> +        rc = xc_sysctl(xch, &sysctl);
> +        if ( !rc )
> +        {
> +            hw_info->u.xc_mba_info.cos_max =
> +                        sysctl.u.psr_alloc_op.u.mba_info.cos_max;
> +            hw_info->u.xc_mba_info.thrtl_max =
> +                        sysctl.u.psr_alloc_op.u.mba_info.thrtl_max;
> +            hw_info->u.xc_mba_info.linear =
> +                        sysctl.u.psr_alloc_op.u.mba_info.flags &
> +                        XEN_SYSCTL_PSR_MBA_LINEAR;
>          }
>          break;
>      default:
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index cf368ba..b183305 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -361,6 +361,27 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
>      return rc;
>  }
>  
> +static inline xc_psr_feat_type libxl__psr_feat_type_to_libxc_psr_feat_type(

No inline please. In any case this is not performance critical code, so
let the compiler decide.

And the function name could be shorter, maybe:

libxl__psr_type_to_libxc_type

Or is this going to clash with some other translation function?

> +                                   libxl_psr_feat_type type, unsigned int 
> lvl)
> +{
> +    xc_psr_feat_type xc_type = XC_PSR_FEAT_UNKNOWN;
> +
> +    switch (type) {
> +    case LIBXL_PSR_FEAT_TYPE_CAT:
> +        if (lvl == 3)
> +            xc_type = XC_PSR_FEAT_CAT_L3;
> +        if (lvl == 2)
> +            xc_type = XC_PSR_FEAT_CAT_L2;
> +        break;
> +    case LIBXL_PSR_FEAT_TYPE_MBA:
> +        xc_type = XC_PSR_FEAT_MBA;
> +    default:
> +        break;
> +    }
> +
> +    return xc_type;
> +}
> +
>  int libxl_psr_cat_get_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
>                             int *nr, unsigned int lvl)
>  {
> @@ -369,6 +390,8 @@ int libxl_psr_cat_get_info(libxl_ctx *ctx, 
> libxl_psr_cat_info **info,
>      int i = 0, socketid, nr_sockets;
>      libxl_bitmap socketmap;
>      libxl_psr_cat_info *ptr;
> +    xc_psr_hw_info hw_info;
> +    xc_psr_feat_type xc_type;
>  
>      libxl_bitmap_init(&socketmap);
>  
> @@ -385,16 +408,23 @@ int libxl_psr_cat_get_info(libxl_ctx *ctx, 
> libxl_psr_cat_info **info,
>          goto out;
>      }
>  
> +    xc_type = libxl__psr_feat_type_to_libxc_psr_feat_type(
> +                  LIBXL_PSR_FEAT_TYPE_CAT, lvl);

Shouldn't you check that xc_type != XC_PSR_FEAT_UNKNOWN here?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel