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

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

On Thu, Sep 25, 2014 at 10:14:35PM +0100, Andrew Cooper wrote:
> On 25/09/2014 21:33, Konrad Rzeszutek Wilk wrote:
> > 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@intel.com>
> >> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> >> ---
> >>  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`
> >> +
> 
> Personally, I think these first 5 lines of context are fine.  They
> follow the standard layout of all other parameters in this document wrt
> names, valid values and default values.
> 
> >> +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.
> 
> I feel that the wording here could be improved for extra clarity.  How
> about:
> 
> Platform Shared Resource Services.  Intel Haswell and later server
> platforms offer information about the sharing of resources.
> 
> The following resources are available:
> * Cache Monitoring Technology (Haswell and later).  Information
> regarding the L3 cache occupancy.
This is good.
> 
> (I seem to remember another one about L3 data bandwidth to local and
> non-local memory controllers, but cant remember its name offhand)
> 
Yeah, there are local/total L3 data bandwidth monitoring. They are
planed in another patchset, so here I may not mention them.
> > 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'? 
> 
> The hardware has a maximum supported RMID, but instead of allocating
> memory based on a u32 out of cpuid, I insisted on a command line max
> parameter to provide a sane upper bound.
> 
> I would however agree that a sentence or two describing what an RMID is
> would be useful here, although being strictly the command line
> documentation, I don't think it warrants a full whitepapers worth of detail.
> 
Sure.
> >
> > 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@intel.com>
> >> + *
> >> + * 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'.
> 
> It is a value provided on the command line, which likely differs from
> CPUID's max reported RMID.  opt_$FOO is the correct variable name.
> 
> >> +
> >> +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 |= ?
> 
> Agreed - this appears to disable cmt if specified.
> 
> >
> >> +        } 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;
> >
> > ?
> 
> Good catch, as "psr_cmt == NULL" is the check for psr being enabled.
Thanks Konrad.
> ~Andrew
> 
> >> +        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?
ok
> >
> >> +    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@intel.com>
> >> + *
> >> + * 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@lists.xen.org
> >> http://lists.xen.org/xen-devel
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel