Re: [Xen-devel] [PATCH v2 08/15] tools: create general interfaces to support psr allocation features

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

On Thu, Aug 24, 2017 at 09:14:42AM +0800, Yi Sun wrote:
> This patch creates general interfaces in libxl to support all psr
> allocation features.
> 
> Add 'LIBXL_HAVE_PSR_MBA' to indicate interface change.

I'm not sure this is enough to cover the changes you are doing here:
you are introducing some MBA stuff, plus a kind of generic interface
for PSR.

I think this should be split into two patches, the first one adding
the generic interface, and the second one adding the MBA stuff.

> Please note, the functionality cannot work until later patches
> are applied.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> ---
> v2:
>     - remove '_INFO' in 'libxl_psr_feat_type' and make corresponding
>       changes in 'libxl_psr_hw_info'.
>       (suggested by Chao Peng)
> ---
>  tools/libxl/libxl.h         | 35 ++++++++++++++++++++++++++++++++++-
>  tools/libxl/libxl_psr.c     | 25 +++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl | 22 ++++++++++++++++++++++
>  3 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 229e289..c1d804c 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -931,6 +931,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> const libxl_mac *src);
>  #define LIBXL_HAVE_PSR_L2_CAT 1
>  
>  /*
> + * LIBXL_HAVE_PSR_MBA
> + *
> + * If this is defined, the Memory Bandwidth Allocation feature is supported.
> + */
> +#define LIBXL_HAVE_PSR_MBA 1
> +
> +/*
>   * LIBXL_HAVE_MCA_CAPS
>   *
>   * If this is defined, setting MCA capabilities for HVM domain is supported.
> @@ -2219,7 +2226,33 @@ int libxl_psr_cat_get_info(libxl_ctx *ctx, 
> libxl_psr_cat_info **info,
>  int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
>                                int *nr);
>  void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr);
> -#endif
> +
> +#ifdef LIBXL_HAVE_PSR_MBA

You don't need this, this is only for consumers of libxl. It is
perfectly fine to have the prototypes of the functions, even if
consumers don't use them.

> +/*
> + * Function to set a domain's value. It operates on a single or multiple
> + * target(s) defined in 'target_map'. 'target_map' specifies all the sockets
> + * to be operated on.
> + */
> +int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid,
> +                      libxl_psr_cbm_type type, libxl_bitmap *target_map,
> +                      uint64_t val);
> +/*
> + * Function to get a domain's cbm. It operates on a single 'target'.
> + * 'target' specifies which socket to be operated on.
> + */
> +int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid,
> +                      libxl_psr_cbm_type type, unsigned int target,
> +                      uint64_t *val);
> +/*
> + * On success, the function returns an array of elements in 'info',
> + * and the length in 'nr'.
> + */
> +int libxl_psr_get_hw_info(libxl_ctx *ctx, libxl_psr_hw_info **info,
> +                          unsigned int *nr, libxl_psr_feat_type type,
> +                          unsigned int lvl);
> +void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr);
> +#endif /* LIBXL_HAVE_PSR_MBA */
> +#endif /* LIBXL_HAVE_PSR_CAT */

Please send a patch to remove the already existing ifdef PSR
pollution.

>  
>  /* misc */
>  
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index f55ba1e..cf368ba 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -425,6 +425,31 @@ void libxl_psr_cat_info_list_free(libxl_psr_cat_info 
> *list, int nr)
>      free(list);
>  }
>  
> +int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid,
> +                      libxl_psr_cbm_type type, libxl_bitmap *target_map,
> +                      uint64_t val)
> +{
> +    return ERROR_FAIL;
> +}
> +
> +int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid,
> +                      libxl_psr_cbm_type type, unsigned int target,
> +                      uint64_t *val)
> +{
> +    return ERROR_FAIL;
> +}
> +
> +int libxl_psr_get_hw_info(libxl_ctx *ctx, libxl_psr_hw_info **info,
> +                          unsigned int *nr, libxl_psr_feat_type type,
> +                          unsigned int lvl)
> +{
> +    return ERROR_FAIL;
> +}
> +
> +void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr)
> +{
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 6e80d36..ab847f8 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -977,6 +977,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [
>      (2, "L3_CBM_CODE"),
>      (3, "L3_CBM_DATA"),
>      (4, "L2_CBM"),
> +    (5, "MBA_THRTL"),

Is this really a CBM type?

Roger.

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