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

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

On 17-09-19 11:15:11, Roger Pau Monn� wrote:
> On Tue, Sep 05, 2017 at 05:32:31PM +0800, Yi Sun wrote:
> > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > index c7710b8..bbdf8e2 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,
> 
> You don't seem to have such an unknown type in the libxl layer, any
> reason you need it at the libxc layer?
> 
It will be used in libxl_psr.c to check if libxl type convertion to libxc type
is correct.

But per your suggestion in libxl_psr.c, I may drop UNKNOWN.

> > +    XC_PSR_FEAT_CAT_L3,
> > +    XC_PSR_FEAT_CAT_L2,
> > +    XC_PSR_FEAT_MBA,
> 
> I think you can drop the _FEAT_ from the enum names.
> 
Ok.

> > +};
> > +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;
> 
> No need for the 'xc_cat', just 'cat' please (and 'mba' below).
> 
Ok.

> > +
> > +        struct {
> > +            uint32_t cos_max;
> > +            uint32_t thrtl_max;
> > +            bool     linear;
> > +        } xc_mba;
> > +    } u;
> > +};
> > +typedef struct xc_psr_hw_info xc_psr_hw_info;
> > +
[...]

> > -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)
> >  {
[...]

> > +    case XC_PSR_FEAT_MBA:
> > +        sysctl.u.psr_alloc.cmd = XEN_SYSCTL_PSR_ALLOC_get_mba_info;
> > +        rc = xc_sysctl(xch, &sysctl);
> > +        if ( !rc )
> > +        {
> > +            hw_info->u.xc_mba.cos_max =
> > +                        sysctl.u.psr_alloc.u.mba_info.cos_max;
> > +            hw_info->u.xc_mba.thrtl_max =
> > +                        sysctl.u.psr_alloc.u.mba_info.thrtl_max;
> > +            hw_info->u.xc_mba.linear =
> > +                        sysctl.u.psr_alloc.u.mba_info.flags &
> > +                        XEN_SYSCTL_PSR_ALLOC_MBA_LINEAR;
> >          }
> 
> Would it help to prevent line breaks to change the indentation above
> to:
> 
> rc = xc_sysctl(...);
> if ( rc )
>     break;
> 
> hw_info->u....
> 
> ?
Let me try.

> 
> >          break;
> >      default:
> > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > index 4a6978e..dd412cc 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 xc_psr_feat_type libxl__feat_type_to_libxc_feat_type(
> > +                            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;
> 
> assert?
> 
> libxl_psr_feat_type cannot have any other value apart from the two
> that you list above.
> 
Ok, may consider it.

> > +    }
> > +
> > +    return xc_type;
> > +}
> > +
> > @@ -385,16 +408,27 @@ int libxl_psr_cat_get_info(libxl_ctx *ctx, 
> > libxl_psr_cat_info **info,
> >          goto out;
> >      }
> >  
> > +    xc_type = libxl__feat_type_to_libxc_feat_type(LIBXL_PSR_FEAT_TYPE_CAT, 
> > lvl);
> > +    if (xc_type == XC_PSR_FEAT_UNKNOWN) {
> > +        LOG(ERROR, "feature type or lvl is wrong");
> > +        rc = ERROR_FAIL;
> > +        goto out;
> > +    }
> > +
> >      ptr = libxl__malloc(NOGC, nr_sockets * sizeof(libxl_psr_cat_info));
> >  
> >      libxl_for_each_set_bit(socketid, socketmap) {
> >          ptr[i].id = socketid;
> > -        if (xc_psr_cat_get_info(ctx->xch, socketid, lvl, &ptr[i].cos_max,
> > -                                &ptr[i].cbm_len, &ptr[i].cdp_enabled)) {
> > +        if (xc_psr_get_hw_info(ctx->xch, socketid, xc_type, &hw_info)) {
> 
> It might be nice to LOG the errno here, or else you lose it AFAICT.
> 
Ok.

> Thanks, Roger.

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