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

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-09-25 at 16:43 +0800, He Chen wrote:
> 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?

Quoting the relevant bits of code for clarity:
     libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
    ...
    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)
        type = LIBXL_PSR_CBM_TYPE_L3_CBM;

So the behaviour if -d and -c are given is exactly the same as if neither
of them were given, i.e. type = LIBXL_PSR_CBM_TYPE_L3_CBM? Is that correct
and intended?

If so then I think it would be clearer to only set opt_* during option
parsing and then to figure out the correct LIBXL_PSR_CBM_TYPE_* explicitly
afterwards, rather than have the code cycle through data->code->cbm.

Or just outlaw passing both -d and -c together since it is confusing and
equivalent to passing neither anyway.

Ian.