Re: [Xen-devel] [PATCH v16 04/10] x86: detect and initialize Cache Monitoring Technology feature

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Sep 25, 2014 at 06:19:04PM +0800, Chao Peng wrote:
> Detect Cache Monitoring Technology(CMT) feature and enumerate the
> resource types, one of which is to monitor the L3 cache occupancy.
> 
> Also introduce a Xen command line parameter to control the Platform
> Shared Resource such as CMT.
> 
> Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> Signed-off-by: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> ---
>  docs/misc/xen-command-line.markdown |   12 ++++
>  xen/arch/x86/Makefile               |    1 +
>  xen/arch/x86/psr.c                  |  107 
> +++++++++++++++++++++++++++++++++++
>  xen/arch/x86/setup.c                |    3 +
>  xen/include/asm-x86/cpufeature.h    |    1 +
>  xen/include/asm-x86/psr.h           |   53 +++++++++++++++++
>  6 files changed, 177 insertions(+)
>  create mode 100644 xen/arch/x86/psr.c
>  create mode 100644 xen/include/asm-x86/psr.h
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index af93e17..b106a46 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1005,6 +1005,18 @@ This option can be specified more than once (up to 8 
> times at present).
>  ### ple\_window
>  > `= <integer>`
>  
> +### psr (Intel)
> +> `= List of ( cmt:<boolean> | rmid_max:<integer> )`

Please explain what 'psr' is (the full name) and why one would want
to use it.

> +
> +> Default: `psr=cmt:0,rmid_max:255`
> +
> +Configure platform shared resource services, which are available on Intel
> +Haswell Server family and future platforms.
> +
> +`cmt` instructs Xen to enable/disable Cache Monitoring Technology.

Please include the default value.

> +
> +`rmid_max` indicates the max value for rmid.

Couple of issues:
 - It reads as not optional (from the documentation) - so what are the values
   that can used? What are the ranges?
 - What is the default value?
 - What is 'RMID'? 

Please please expand more on this. You want users to able to easily
read it and understand it right away without having to search for an
whitepaper on it.
> +
>  ### reboot
>  > `= t[riple] | k[bd] | a[cpi] | p[ci] | n[o] [, [w]arm | [c]old]`
>  
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index c1e244d..cf137fd 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -59,6 +59,7 @@ obj-y += crash.o
>  obj-y += tboot.o
>  obj-y += hpet.o
>  obj-y += xstate.o
> +obj-y += psr.o
>  
>  obj-$(crash_debug) += gdbstub.o
>  
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> new file mode 100644
> index 0000000..9025aeb
> --- /dev/null
> +++ b/xen/arch/x86/psr.c
> @@ -0,0 +1,107 @@
> +/*
> + * pqos.c: Platform Shared Resource related service for guest.
> + *
> + * Copyright (c) 2014, Intel Corporation
> + * Author: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +#include <xen/init.h>
> +#include <xen/cpu.h>
> +#include <asm/psr.h>
> +
> +#define PSR_CMT        (1<<0)
> +
> +struct psr_cmt *__read_mostly  psr_cmt = NULL;

Extra space. No need for the NULL assigment (as this is in .rodata section).

> +static bool_t __initdata opt_psr = 0;

Ditto. No need to assign 0.

> +static unsigned int __initdata opt_rmid_max = 255;

It is not really an 'optional' value as the default is '255'.

I would just call it 'rmid_max'.
> +
> +static void __init parse_psr_param(char *s)
> +{
> +    char *ss, *val_str;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        val_str = strchr(s, ':');
> +        if ( val_str )
> +            *val_str++ = '\0';
> +
> +        if ( !strcmp(s, "cmt")
> +             && ( !val_str || parse_bool(val_str) == 1 )) {
> +            opt_psr &= PSR_CMT;

&= ?

Not opt_psr |= ?

> +        } else if ( val_str && !strcmp(s, "rmid_max") )
> +            opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> +
> +        s = ss + 1;
> +    } while ( ss );
> +}
> +custom_param("psr", parse_psr_param);
> +
> +static void __init init_psr_cmt(unsigned int rmid_max)
> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    unsigned int rmid;
> +
> +    if ( !boot_cpu_has(X86_FEATURE_CMT) )
> +        return;
> +
> +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &edx);
> +    if ( !edx )
> +        return;
> +
> +    psr_cmt = xzalloc(struct psr_cmt);
> +    if ( !psr_cmt )
> +        return;
> +
> +    psr_cmt->features = edx;
> +    psr_cmt->rmid_mask = ~(~0ull << get_count_order(ebx));
> +    psr_cmt->rmid_max = min(rmid_max, ebx);
> +
> +    if ( psr_cmt->features & PSR_RESOURCE_TYPE_L3 )
> +    {
> +        cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
> +        psr_cmt->l3.upscaling_factor = ebx;
> +        psr_cmt->l3.rmid_max = ecx;
> +        psr_cmt->l3.features = edx;
> +    }
> +
> +    psr_cmt->rmid_max = min(rmid_max, psr_cmt->l3.rmid_max);
> +    psr_cmt->rmid_to_dom = xmalloc_array(domid_t, psr_cmt->rmid_max + 1);
> +    if ( !psr_cmt->rmid_to_dom )
> +    {
> +        xfree(psr_cmt);

And:
        psr_cmt = NULL;

?
> +        return;
> +    }
> +    /* Reserve RMID 0 for all domains not being monitored */

Full stop missing.

Why do you reserve RMID 0? Can you include the explanation
in the comment please?

> +    psr_cmt->rmid_to_dom[0] = DOMID_XEN;
> +    for ( rmid = 1; rmid <= psr_cmt->rmid_max; rmid++ )
> +        psr_cmt->rmid_to_dom[rmid] = DOMID_INVALID;
> +
> +    printk(XENLOG_INFO "Cache Monitoring Technology Enabled.\n");
> +}
> +
> +void __init init_psr(void)


> +{
> +    if ( opt_psr & PSR_CMT && opt_rmid_max )
> +        init_psr_cmt(opt_rmid_max);
> +}

__initcall(init_psr);

> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 8c8b91f..ca4785e 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -49,6 +49,7 @@
>  #include <xen/cpu.h>
>  #include <asm/nmi.h>
>  #include <asm/alternative.h>
> +#include <asm/psr.h>
>  
>  /* opt_nosmp: If true, secondary processors are ignored. */
>  static bool_t __initdata opt_nosmp;
> @@ -1430,6 +1431,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      domain_unpause_by_systemcontroller(dom0);
>  
> +    init_psr();
> +

And then you can remove this.

>      reset_stack_and_jump(init_done);
>  }
>  
> diff --git a/xen/include/asm-x86/cpufeature.h 
> b/xen/include/asm-x86/cpufeature.h
> index 8014241..137d75c 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -148,6 +148,7 @@
>  #define X86_FEATURE_ERMS     (7*32+ 9) /* Enhanced REP MOVSB/STOSB */
>  #define X86_FEATURE_INVPCID  (7*32+10) /* Invalidate Process Context ID */
>  #define X86_FEATURE_RTM      (7*32+11) /* Restricted Transactional Memory */
> +#define X86_FEATURE_CMT      (7*32+12) /* Cache Monitoring Technology */
>  #define X86_FEATURE_NO_FPU_SEL       (7*32+13) /* FPU CS/DS stored as zero */
>  #define X86_FEATURE_MPX              (7*32+14) /* Memory Protection 
> Extensions */
>  #define X86_FEATURE_RDSEED   (7*32+18) /* RDSEED instruction */
> diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
> new file mode 100644
> index 0000000..e321890
> --- /dev/null
> +++ b/xen/include/asm-x86/psr.h
> @@ -0,0 +1,53 @@
> +/*
> + * psr.h: Platform Shared Resource related service for guest.
> + *
> + * Copyright (c) 2014, Intel Corporation
> + * Author: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +#ifndef __ASM_PSR_H__
> +#define __ASM_PSR_H__
> +
> +/* Resource Type Enumeration */
> +#define PSR_RESOURCE_TYPE_L3            0x2
> +
> +/* L3 Monitoring Features */
> +#define PSR_CMT_L3_OCCUPANCY           0x1
> +
> +struct psr_cmt_l3 {
> +    unsigned int features;
> +    unsigned int upscaling_factor;
> +    unsigned int rmid_max;
> +};
> +
> +struct psr_cmt {
> +    unsigned long rmid_mask;
> +    unsigned int rmid_max;
> +    unsigned int features;
> +    domid_t *rmid_to_dom;
> +    struct psr_cmt_l3 l3;
> +};
> +
> +extern struct psr_cmt *psr_cmt;
> +
> +void init_psr(void);
> +
> +#endif /* __ASM_PSR_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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