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

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

On Sat, Sep 23, 2017 at 09:48:17AM +0000, Yi Sun wrote:
> This patch creates general interfaces in libxl to support all psr
> allocation features.
> 
> Add 'LIBXL_HAVE_PSR_GENERIC' to indicate interface change.
> 
> Please note, the functionality cannot work until later patches
> are applied.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> ---
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> 
> v4:
>     - add description for LIBXL_HAVE_PSR_GENERIC to mention newly added
>       public functions.
>       (suggested by Roger Pau Monné)
> v3:
>     - change 'LIBXL_HAVE_PSR_MBA' to 'LIBXL_HAVE_PSR_GENERIC'.
>       (suggested by Roger Pau Monné)
>     - 'typedef enum libxl_psr_cbm_type libxl_psr_type;' in libxl.h.
>       (suggested by Roger Pau Monné and Wei Liu)
>     - change 'libxl_psr_cbm_type' to 'libxl_psr_type' in newly defined
>       interfaces.
>       (suggested by Roger Pau Monné)
> 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         | 37 +++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_psr.c     | 25 +++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl | 22 ++++++++++++++++++++++
>  3 files changed, 84 insertions(+)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 91408b4..569c331 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -936,6 +936,17 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> const libxl_mac *src);
>  #define LIBXL_HAVE_PSR_L2_CAT 1
>  
>  /*
> + * LIBXL_HAVE_PSR_GENERIC
> + *
> + * If this is defined, the Memory Bandwidth Allocation feature is supported.
> + * The following public functions are available:
> + *   libxl_psr_{set/get}_val
> + *   libxl_psr_get_hw_info
> + *   libxl_psr_hw_info_list_free
> + */
> +#define LIBXL_HAVE_PSR_GENERIC 1
> +
> +/*
>   * LIBXL_HAVE_MCA_CAPS
>   *
>   * If this is defined, setting MCA capabilities for HVM domain is supported.
> @@ -2228,6 +2239,32 @@ 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);
> +
> +typedef enum libxl_psr_cbm_type libxl_psr_type;
> +
> +/*
> + * 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_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_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);

I would place the returned parameters at the end of the list, like you
do in libxl_psr_get_val.

> +void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr);
>  #endif
>  
>  /* misc */
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 197505a..4a6978e 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -428,6 +428,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_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_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 173d70a..cfe8367 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"),
>      ])
>  
>  libxl_psr_cat_info = Struct("psr_cat_info", [
> @@ -985,3 +986,24 @@ libxl_psr_cat_info = Struct("psr_cat_info", [
>      ("cbm_len", uint32),
>      ("cdp_enabled", bool),
>      ])
> +
> +libxl_psr_feat_type = Enumeration("psr_feat_type", [
> +    (1, "CAT"),
> +    (2, "MBA"),
> +    ])

So we have a libxl_psr_type and a libxl_psr_feat_type. This seems
quite unnecessary, but I guess now it's too late to fix this anyway.

Also I see that one is needed for libxl_psr_{set/get}_val while the
other is used for libxl_psr_get_hw_info.

Thanks, Roger.

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