Re: [PATCH v5 12/13] tools: add tools support for Intel CAT

[Chao Peng <chao.p.peng@linux.intel.com>]

On Tue, Apr 21, 2015 at 05:15:15PM +0200, Dario Faggioli wrote:
> On Tue, 2015-04-21 at 17:49 +0800, Chao Peng wrote:
> > On Tue, Apr 21, 2015 at 03:24:37AM +0200, Dario Faggioli wrote:
> > > On Fri, 2015-04-17 at 22:33 +0800, Chao Peng wrote:
> > > > This is the xc/xl changes to support Intel Cache Allocation
> > > > Technology(CAT). Two commands are introduced:
> > > > - xl psr-cat-hwinfo
> > > >   Show CAT hardware information.
> > > 
> > > > Examples:
> > > > [root@vmm-psr vmm]# xl psr-cat-hwinfo
> > > > Cache Allocation Technology (CAT):
> > > > Socket ID       : 0
> > > > L3 Cache        : 12288KB
> > > > Maximum COS     : 15
> > > > CBM length      : 12
> > > > Default CBM     : 0xfff
> > > > 
> > > Or, you can rename the psr-cmt-hwinfo command, added in the previous
> > > patch, to 'psr-hwinfo' and make it accept options, e.g.,
> > > 
> > >  "-m, --cmt   show Cache Monitoring Technology (CMT) hardware info"
> > >  "-c, --cat   show Cache Allocation Technology (CAT) hardware info"
> > > 
> > > By default (i.e., no options provided), it can just print all the hw
> > > info.
> > > 
> > > Not a big deal, but I think that would make a better command line
> > > interface. Tools' maintainers' call, I guess.
> > 
> > Thanks for suggestion.
> > > 
> > > > --- a/tools/libxc/include/xenctrl.h
> > > > +++ b/tools/libxc/include/xenctrl.h
> > > 
> > > > +
> > > > +int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
> > > > +                               xc_psr_cat_type type, uint32_t target,
> > > > +                               uint64_t data);
> > > > +int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
> > > > +                               xc_psr_cat_type type, uint32_t target,
> > > > +                               uint64_t *data);
> > > >
> > > So, for this twos, 'target' is the socket you want to act on.
> > > 
> > > > +int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
> > > > +                           uint32_t *cos_max, uint32_t *cbm_len);
> > > >
> > > While here you use 'socket', to mean the same thing.
> > > 
> > > That looks rather inconsistent. Since it's a socket we are talking
> > > about, why not 'socket' everywhere?
> > 
> > 
> > The idea behind here is: All the places that appear as 'target' imply 
> > there are possible values other than just socket (e.g. considering L2
> > Cache Allocation in the future). So 'target' is always paired with a
> > 'psr_cat_type' parameter. 
> >
> Mmm... I understand your concerns. So, sticking to the future L2 CAT
> support example, what would 'target' mean in that case, a pCPU? I'll
> have to be something that makes it possible to 'identify' an L2, as much
> as socket identifies an L3... Is this the case?

For L2, it's likely to be the cpu or core id, but not socket.

> 
> I'm not sure. My own concerns are that, if I look at the prototypes of
> this functions, it's not that evident what values should I use for the
> type and target parameters, whether there are constraints/relationships
> among them, etc. That applies to both the xc_* functions above and the
> libxl_* functions below, IMO.
> 
> Libxc is not a stable interface, so we could even just forget about
> this, design this very interface _only_ for what we have now and deal
> with different CBM types when we'll be introducing them. However, libxl
> *does* have a stable API, so we still need to solve the issue at that
> level.
> 
> > For routines that only work for L3 (e.g.
> > xc_psr_cat_get_l3_info) then 'socket' is used. I admit that it looks
> > inconsistent, perhaps rename all 'socket' to 'target'?
> > 
> No, IMO, that is one "good inconsistency", as it allows, at least for
> that function, to easily figure out what one should pass to the function
> by means of that parameter! :-)
> 
> I really am not sure, and probably would have to know in what way(s)
> 'target' would change its meaning, depending on the value of 'type' (as
> asked above)... Probably what I'd do is leave parameters names as they
> are, but write a few doc-comments to explain how to use them, especially
> at the libxl level (libxc is a lot less critical, from this respect, I
> think).

OK, I can add some comments.

Thanks,
Chao