Re: [PATCH v3 13/15] tools: implement new generic get value interface and MBA get value command

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-09-19 12:02:08, Roger Pau Monn� wrote:
> On Tue, Sep 05, 2017 at 05:32:35PM +0800, Yi Sun wrote:
> > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > index c8d2921..78d5bc5 100644
> > --- a/tools/libxl/libxl_psr.c
> > +++ b/tools/libxl/libxl_psr.c
> > @@ -71,16 +71,30 @@ static void libxl__psr_cmt_log_err_msg(libxl__gc *gc, int err)
> >      LOGE(ERROR, "%s", msg);
> >  }
> >  
> > -static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int err)
> > +static void libxl__psr_alloc_log_err_msg(libxl__gc *gc,
> > +                                         int err,
> > +                                         libxl_psr_type type)
> >  {
> > +    /*
> > +     * Index is 'libxl_psr_type' so we set two 'CDP' to correspond to
> > +     * DATA and CODE.
> > +     */
> > +    const char * const feat_name[6] = {
> 
> The explicit '6' is not needed.
> 
> > +        "UNKNOWN",
> > +        "L3 CAT",
> > +        "CDP",
> > +        "CDP",
> > +        "L2 CAT",
> > +        "MBA",
> 
> I'm not sure whether you want to use designated initializers here, in
> case someone decides to change the order of the xc_psr_type enum or
> the order here. ie:
> 
> feat_name[]?= {
>     [XC_PSR_CAT_L3_CBM] = "L3 CAT",
>     [XC_PSR_CAT_L3_CBM_CODE..XC_PSR_CAT_L3_CBM_DATA] = "CDP",
>     ...
> }
> 
Got it. Thanks! One correction, the index is 'libxl_psr_type'.

> > +    };
> >      char *msg;
> >  
> >      switch (err) {
> >      case ENODEV:
> > -        msg = "CAT is not supported in this system";
> > +        msg = "is not supported in this system";
> >          break;
> >      case ENOENT:
> > -        msg = "CAT is not enabled on the socket";
> > +        msg = "is not enabled on the socket";
> >          break;
> >      case EOVERFLOW:
> >          msg = "no free COS available";
> > @@ -106,7 +120,7 @@ static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int err)
> >          return;
> >      }
> >  
> > -    LOGE(ERROR, "%s", msg);
> > +    LOGE(ERROR, "%s: %s", feat_name[type], msg);
> 
> I don't think you should use LOGE here, but rather LOG. LOGE should be
> used when errno is set, which I don't think is the case here.
> 
But two places to call libxl__psr_cat_log_err_msg all set errno in 'xc_'
functions.

> > @@ -347,18 +361,7 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
> >                            libxl_psr_cbm_type type, uint32_t target,
> >                            uint64_t *cbm_r)
> >  {
> > -    GC_INIT(ctx);
> > -    int rc = 0;
> > -    xc_psr_type xc_type = libxl__psr_cbm_type_to_libxc_psr_type(type);
> > -
> > -    if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type,
> > -                                   target, cbm_r)) {
> > -        libxl__psr_cat_log_err_msg(gc, errno);
> > -        rc = ERROR_FAIL;
> > -    }
> > -
> > -    GC_FREE;
> > -    return rc;
> > +    return libxl_psr_get_val(ctx, domid, type, target, cbm_r);
> >  }
> 
> You could even move this to libxl.h as a static function IMHO.
> 
Yes. But I prefer to keep it here with other interfaces together. Is that
acceptable to you?

> > diff --git a/tools/xl/xl_psr.c b/tools/xl/xl_psr.c
> > index 40269b4..46b7788 100644
> > --- a/tools/xl/xl_psr.c
> > +++ b/tools/xl/xl_psr.c
> > -static int psr_cat_print_socket(uint32_t domid, libxl_psr_cat_info *info,
> > -                                unsigned int lvl)
> > +static int psr_print_socket(uint32_t domid,
> > +                            libxl_psr_hw_info *info,
> > +                            libxl_psr_feat_type type,
> > +                            unsigned int lvl)
> >  {
> > -    int rc;
> > -    uint32_t l3_cache_size;
> > -
> >      printf("%-16s: %u\n", "Socket ID", info->id);
> >  
> > -    /* So far, CMT only supports L3 cache. */
> > -    if (lvl == 3) {
> > -        rc = libxl_psr_cmt_get_l3_cache_size(ctx, info->id, &l3_cache_size);
> > -        if (rc) {
> > -            fprintf(stderr, "Failed to get l3 cache size for socket:%d\n",
> > -                    info->id);
> > -            return -1;
> > +    switch (type) {
> > +    case LIBXL_PSR_FEAT_TYPE_CAT:
> > +    {
> > +        int rc;
> > +        uint32_t l3_cache_size;
> > +
> > +        /* So far, CMT only supports L3 cache. */
> > +        if (lvl == 3) {
> 
> Shouldn't you print some kind of error message if lvl != 3? Or is it
> expected that this function will be called with lvl != 3 and it should
> be ignored?
> 
We only get cache size for level 3 cache. So, if input is lvl=2, we print
nothing.

> Thanks, Roger.

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