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-20 09:57:59, Roger Pau Monn� wrote:
> On Wed, Sep 20, 2017 at 02:46:24PM +0800, Yi Sun wrote:
> > On 17-09-19 12:02:08, Roger Pau Monn� wrote:
> > > On Tue, Sep 05, 2017 at 05:32:35PM +0800, Yi Sun wrote:
> > > > +    };
> > > >      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.
> 
> But you already translate the error into a custom message ('msg' in the
> above context), and the error variable in this case is 'err' not
> 'errno', so LOGE is not appropriate here IMHO.
> 
Ok.

[...]

> > > > 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.
> 
> My question would be, is it expected that this function is called with
> lvl == 2 as part of the normal operation with valid input values?
> 
Yes, lvl == 2 is a normal operation.

> Roger.

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