|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v16 04/10] x86: detect and initialize Cache Monitoring Technology feature
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@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`
>> +
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.
(I seem to remember another one about L3 data bandwidth to local and
non-local memory controllers, but cant remember its name offhand)
> 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.
>
> 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'.
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.
~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?
>
>> + 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |