Re: [PATCH v6 13/14] tools: add tools support for Intel CAT

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

On Thu, 2015-04-23 at 17:55 +0800, Chao Peng wrote:
> This is the xc/xl changes to support Intel Cache Allocation
> Technology(CAT).
> 
> 'xl psr-hwinfo' is updated to show CAT info and two new commands
> for CAT are introduced:
> - xl psr-cat-cbm-set [-s socket] <domain> <cbm>
>   Set cache capacity bitmasks(CBM) for a domain.
> - xl psr-cat-show <domain>
>   Show CAT domain information.
> 
> Examples:
> [root@vmm-psr vmm]# xl psr-hwinfo --cat
> Cache Allocation Technology (CAT):
> Socket ID       : 0
> L3 Cache        : 12288KB
> Maximum COS     : 15
> CBM length      : 12
> Default CBM     : 0xfff
> 
> [root@vmm-psr vmm]# xl psr-cat-cbm-set 0 0xff
> 
> [root@vmm-psr vmm]# xl psr-cat-show
> Socket ID       : 0
> L3 Cache        : 12288KB
> Default CBM     : 0xfff
>    ID                     NAME             CBM
>     0                 Domain-0            0xff
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
> Changes in v6:
> * Merge xl psr-cmt/cat-hwinfo => xl psr-hwinfo.
> * Add function header to explain the 'target' parameter.
> * Use bitmap instead of TARGETS_ALL.
> * Remove the need to store the return value form libxc.
> * Minor document/commit msg adjustment.
> Changes in v5:
> * Add psr-cat-hwinfo.
> * Add libxl_psr_cat_info_list_free.
> * malloc => libxl__malloc
> * Other comments from Ian/Wei.
> Changes in v4:
> * Add example output in commit message.
> * Make libxl__count_physical_sockets private to libxl_psr.c.
> * Set errno in several error cases.
> * Change libxl_psr_cat_get_l3_info to return all sockets information.
> * Remove unused libxl_domain_info call.
> Changes in v3:
> * Add manpage.
> * libxl_psr_cat_set/get_domain_data => libxl_psr_cat_set/get_cbm.
> * Move libxl_count_physical_sockets into seperate patch.
> * Support LIBXL_PSR_TARGET_ALL for libxl_psr_cat_set_cbm.
> * Clean up the print codes.
> ---
>  docs/man/xl.pod.1             |  57 +++++++++-
>  tools/libxc/include/xenctrl.h |  15 +++
>  tools/libxc/xc_psr.c          |  76 ++++++++++++++
>  tools/libxl/libxl.h           |  35 +++++++
>  tools/libxl/libxl_psr.c       | 143 +++++++++++++++++++++++--
>  tools/libxl/libxl_types.idl   |  10 ++
>  tools/libxl/xl.h              |   4 +
>  tools/libxl/xl_cmdimpl.c      | 237 ++++++++++++++++++++++++++++++++++++++++--
>  tools/libxl/xl_cmdtable.c     |  18 +++-
>  9 files changed, 575 insertions(+), 20 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 7fd9bff..c5044b3 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1495,10 +1495,20 @@ for any of these monitoring types.
>  
>  =over 4
>  
> -=item B<psr-hwinfo>
> +=item B<psr-hwinfo> [I<OPTIONS>]
>  
>  Show CMT hardware information.
>  
> +B<OPTIONS>
> +
> +=over 4
> +
> +=item B<-m>, B<--cmt>
> +
> +Show Cache Monitoring Technology (CMT) hardware information.

Duplicating the psr-hwinfo case for CMT and CAT is confusing.

I suggest to add =head1 <WHATEVER PSR STANDS FOR> and to push the
existing "=head1 CACHE MONITORING TECHNOLOGY" to a =head2, and the same
for the new CACHE ALLOCATION TECHNOLOGY section.

i.e. have an overall PSR section and then subsections for CMT and CAT.

psr-hwinfo would then be described at the top level and the CMD/CAT
specifics in the relevant section.

Does that sound ok?

> +=head1 CACHE ALLOCATION TECHNOLOGY
> +
> +Intel Broadwell and later server platforms offer capabilities to configure and
> +make use of the Cache Allocation Technology (CAT) mechanisms, which enable more
> +cache resources (i.e. L3 cache) to be made available for high priority
> +applications. In Xen implementation, CAT is used to control cache allocation
                   ^the

> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index b81b3ff..eede5f1 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -527,7 +527,9 @@ struct cmd_spec cmd_table[] = {
>      { "psr-hwinfo",
>        &main_psr_hwinfo, 0, 1,
>        "Show hardware information for Platform Shared Resource",
> -      "",
> +      "[options]",
> +      "-m <cmt>       Show Cache Monitoring Technology (CMT) hardware info\n"
> +      "-a <cat>       Show Cache Allocation Technology (CAT) hardware info\n"

<cmt> and <cat> seem out of place here, since -m and -a do not take
options AFAICT.

Apart from those minor comments it looks good to me, thanks.

Ian.