Re: [PATCH v6 3/3] tools & docs: add tools and docs support for Intel CDP

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

On Thu, 2015-10-08 at 11:23 +0800, He Chen wrote:
> This is the xl/xc changes to support Intel Code/Data Prioritization.
> CAT xl commands to set/get CBMs are extended to support CDP.
> Add new CDP options with CAT commands in xl interface man page.
> Add description of CDP in xl-psr.markdown.
> 
> Signed-off-by: He Chen <he.chen@linux.intel.com>
> ---
> Changes in v6:
> * separate CBM headings in the output of xl psr-cat-show
> * revert the numbers of SDM chapter in xl-psr.markdown
> * XC_PSR_CAT_L3_CODE (DATA) => XC_PSR_CAT_L3_CBM_CODE (DATA)

In this comment on v5 I mentioned that it was more important for libxl than
libxc, I'm afraid that what I was trying to say was that the libxl names
needed changing as well. Sorry for not clearly saying so.

> +For more detailed information please refer to Intel SDM chapter
> +"17.15 - Platform Shared Resource Control: Cache Allocation Technology".

Looks like you missed deleting this section number.

> @@ -8454,20 +8457,30 @@ 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("%#16"PRIx64, cbm);

If this libxl_psr_cat_get_cbm(CODE) fails for some reason then this prints
nothing, meaning that the following libxl_psr_cat_get_cbm(DATA) will
actually print into the code column.

I think you should add an:
    else 
        printf("%16s", "error")

(or perhaps "%-16s" for alignment, please check which looks best and
decide)

Please also refactor all three of these nearly identical:

    if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_???,       
                                 socketid, &cbm))
                 printf("%#16"PRIx64, cbm);

blocks into a helper which takes the PSR_CBM_TYPE as an argument and prints
either the result or the error string with the appropriate width.

> +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_DATA,
> +                                   socketid, &cbm))
> +            printf("%#16"PRIx64, cbm);
> +    }
>