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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-08-30 09:42:56, Roger Pau Monn� wrote:
> 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.
> 
This patch only introduces the generic interfaces without any MBA stuff.

The 'LIBXL_HAVE_PSR_MBA' is used to indicate the interfaces change here.
Per my understand, we should add a macro to indicate libxl interfaces
change, right?

> > --- 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.
> 
Oh, ok. So the interfaces declaration does not need be included by macro. I see
some other interfaces are done so. So, I follow the convention.

> > +/*
> > + * 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.
> 
Ok, I will send a patch to remoev this '#ifdef LIBXL_HAVE_PSR_CAT' in this file.

[...]

> > 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?
> 
This is not CBM type. The 'libxl_psr_cbm_type' name is not good enough. But I
have to introduce a new generic interface here if we want to make the name be
generic. I think it is not so valuable. So, I reuse the 'libxl_psr_cbm_type'
to cover MBA. How do you think?

> Roger.

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