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

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

On 17-08-30 09:58:03, Roger Pau Monn� wrote:
> On Thu, Aug 24, 2017 at 09:14:43AM +0800, Yi Sun wrote:
> > 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.
> 
Ok, got it.

> > +    } u;
> > +};
> > +typedef struct xc_psr_hw_info xc_psr_hw_info;

[...]

> > 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.
> 
Sorry for missing it. Thanks!

[...]

> > 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.
> 
Ok, will remove it.

> 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?
>
Yes, there is another similar function which name is:
  'libxl__psr_cbm_type_to_libxc_psr_val_type'

I think I can remove the last '_psr' to make the name be shorter.
 
[...]

> > @@ -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?
> 
Yes, I should. Thanks!

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

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