Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[He Chen <he.chen@linux.intel.com>]

On Thu, Sep 24, 2015 at 12:07:27PM +0100, Ian Campbell wrote:
> On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> > @@ -8410,20 +8415,29 @@ static void psr_cat_print_one_domain_cbm(uint32_t
> > domid, uint32_t socketid)
> >      printf("%5d%25s", domid, domain_name);
> >      free(domain_name);
> >  
> > -    if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
> > -                               socketid, &cbm))
> > -         printf("%#16"PRIx64, cbm);
> > -
> > +    if (!cdp_enabled) {
> > +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
> > +                                   socketid, &cbm))
> > +            printf("%#16"PRIx64, cbm);
> > +    } else {
> > +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CODE,
> > +                                   socketid, &cbm))
> > +            printf("%10s%#8"PRIx64, "code:", cbm);
> > +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_DATA,
> > +                                   socketid, &cbm))
> > +            printf("%10s%#8"PRIx64, "data:", cbm);
> > +    }
> 
> Does cdp being enabled mean that the original L3_CBM functionality is no
> longer available then?
> 
> Please could you give an example of the new output format for this command
> in the commit message.
> 

For the get side, the answer is Yes. But for the set side, L3_CBM means that
setting the same code CBM and data CBM when CDP is enabled.

> >  static int psr_cat_show(uint32_t domid)
> > @@ -8489,6 +8503,8 @@ int main_psr_cat_cbm_set(int argc, char **argv)
> >      libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> >      uint64_t cbm;
> >      int ret, opt = 0;
> > +    int opt_data = 0;
> > +    int opt_code = 0;
> >      libxl_bitmap target_map;
> >      char *value;
> >      libxl_string_list socket_list;
> > 
> > [...]
> 
> > @@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char **argv)
> >          libxl_string_list_dispose(&socket_list);
> >          free(value);
> >          break;
> > +    case 'd':
> > +        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
> > +        opt_data = 1;
> > +        break;
> > +    case 'c':
> > +        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
> > +        opt_code = 1;
> > +        break;
> >      }
> >  
> > +    if (opt_data && opt_code)
> 
> Do you not mean !opt_data && !opt_code?
> 
> But also, isn't this assignment unnecessary since type is initialised to
> the same value when it is declared?
> 
> In fact, because of that initialisation, aren't opt_data and opt_code
> unnecessary, since you set type appropriately elsewhere.
> 
> Are -d and -c mutually exclusive, or is it expected that both can be given?
> 

-d and -c can be both given.

The initialisation of type is L3_CBM, which corresponds to the situation
that neither -d nor -c is given.

I add `if (opt_data && opt_code)` to address the situation that -d and -c
are both given.
If user gives both -d and -c options, image that without if() statement,
-d will be overwritten by the latter -c in switch, and type will be
L3_CODE instead of L3_CBM (means set both and what user wants).

I hope I had made myself clear, or is there something wrong with my
understanding?

Thanks.

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