Re: [PATCH v2 1/8] x86: detect and initialize Cache QoS Monitoring feature

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/11/13 12:14, Andrew Cooper wrote:
> On 21/11/13 07:20, dongxiao.xu@intel.com wrote:
>> From: Dongxiao Xu <dongxiao.xu@intel.com>
>>
>> Detect platform QoS feature status and enumerate the resource types,
>> one of which is to monitor the L3 cache occupancy.
>>
>> Also introduce a Xen grub command line parameter to control the
>> QoS feature status globally.
>>
>> Signed-off-by: Jiongxi Li <jiongxi.li@intel.com>
>> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>> ---
>>  xen/arch/x86/Makefile            |    1 +
>>  xen/arch/x86/cpu/intel.c         |    6 +++
>>  xen/arch/x86/pqos.c              |   89 ++++++++++++++++++++++++++++++++++++++
>>  xen/arch/x86/setup.c             |    3 ++
>>  xen/include/asm-x86/cpufeature.h |    1 +
>>  xen/include/asm-x86/pqos.h       |   37 ++++++++++++++++
>>  6 files changed, 137 insertions(+)
>>  create mode 100644 xen/arch/x86/pqos.c
>>  create mode 100644 xen/include/asm-x86/pqos.h
>>
>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>> index d502bdf..54962e0 100644
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -58,6 +58,7 @@ obj-y += crash.o
>>  obj-y += tboot.o
>>  obj-y += hpet.o
>>  obj-y += xstate.o
>> +obj-y += pqos.o
>>  
>>  obj-$(crash_debug) += gdbstub.o
>>  
>> diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
>> index 27fe762..f0d83ea 100644
>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -230,6 +230,12 @@ static void __devinit init_intel(struct cpuinfo_x86 *c)
>>  	     ( c->cpuid_level >= 0x00000006 ) &&
>>  	     ( cpuid_eax(0x00000006) & (1u<<2) ) )
>>  		set_bit(X86_FEATURE_ARAT, c->x86_capability);
>> +
>> +	/* Check platform QoS monitoring capability */
>> +	if ((c->cpuid_level >= 0x00000007) &&
>> +	    (cpuid_ebx(0x00000007) & (1u<<12)))
>> +		set_bit(X86_FEATURE_QOSM, c->x86_capability);
>> +
>>  }
>>  
>>  static struct cpu_dev intel_cpu_dev __cpuinitdata = {
>> diff --git a/xen/arch/x86/pqos.c b/xen/arch/x86/pqos.c
>> new file mode 100644
>> index 0000000..e6ab416
>> --- /dev/null
>> +++ b/xen/arch/x86/pqos.c
>> @@ -0,0 +1,89 @@
>> +/*
>> + * pqos.c: Platform QoS related service for guest.
>> + *
>> + * Copyright (c) 2013, Intel Corporation
>> + * Author: Jiongxi Li  <jiongxi.li@intel.com>
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
>> + * Place - Suite 330, Boston, MA 02111-1307 USA.
>> + */
>> +#include <asm/processor.h>
>> +#include <xen/init.h>
>> +#include <asm/pqos.h>
>> +
>> +static bool_t pqos_enabled = 1;
> I cant see anywhere in the series where this is used outside of an init
> function, in which case it should be __initdata.
>
>> +boolean_param("pqos", pqos_enabled);
>> +
>> +unsigned int cqm_res_count = 0;
>> +unsigned int cqm_upscaling_factor = 0;
>> +bool_t cqm_enabled = 0;
>> +struct cqm_res_struct *cqm_res_array = NULL;
>> +
>> +static void __init init_cqm(void)
>> +{
>> +    unsigned int eax, edx;
>> +    unsigned int max_cqm_rmid;
>> +
>> +    cpuid_count(0xf, 1, &eax, &cqm_upscaling_factor, &max_cqm_rmid, &edx);
>> +    if ( !(edx & QOS_MONITOR_EVTID_L3) )
>> +        return;
>> +
>> +    cqm_res_count = max_cqm_rmid + 1;
>> +
> Range check on cqm_res_count ?  If max_cqm_rmid ends up as -1 from the
> cpuid, we will allocate a 0 length array and crash later when reserving
> RMID 0
>
>> +    cqm_res_array = xzalloc_array(struct cqm_res_struct, cqm_res_count);
>> +    if ( !cqm_res_array )
>> +    {
>> +        cqm_res_count = 0;
>> +        return;
>> +    }
>> +
>> +    cqm_enabled = 1;
>> +
>> +    /* Reserve RMID 0 for all domains not being monitored */
>> +    cqm_res_array[0].inuse = 1;
>> +
>> +    printk(XENLOG_INFO "Cache QoS Monitoring Enabled.\n");
>> +}
>> +
>> +static void __init init_qos_monitor(void)
>> +{
>> +    unsigned int qm_features;
>> +    unsigned int eax, ebx, ecx;
>> +
>> +    if ( !(boot_cpu_has(X86_FEATURE_QOSM)) )
>> +        return;
>> +
>> +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &qm_features);
>> +
>> +    if ( qm_features & QOS_MONITOR_TYPE_L3 )
>> +        init_cqm();
>> +}
>> +
>> +void __init init_platform_qos(void)
>> +{
>> +    if ( !pqos_enabled )
>> +        return;
>> +
>> +    init_qos_monitor();
>> +}
>> +
>> +/*
>> + * 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 5bf4ee0..95418e4 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -48,6 +48,7 @@
>>  #include <asm/setup.h>
>>  #include <xen/cpu.h>
>>  #include <asm/nmi.h>
>> +#include <asm/pqos.h>
>>  
>>  /* opt_nosmp: If true, secondary processors are ignored. */
>>  static bool_t __initdata opt_nosmp;
>> @@ -1402,6 +1403,8 @@ void __init __start_xen(unsigned long mbi_p)
>>  
>>      domain_unpause_by_systemcontroller(dom0);
>>  
>> +    init_platform_qos();
>> +
> Does this want to be slightly earlier during startup? Does it make sense
> to have dom0 monitored?  Is it possible to monitor Xen's L3 usage as well?
>
>>      reset_stack_and_jump(init_done);
>>  }
>>  
>> diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
>> index 1cfaf94..ca59668 100644
>> --- a/xen/include/asm-x86/cpufeature.h
>> +++ b/xen/include/asm-x86/cpufeature.h
>> @@ -147,6 +147,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_QOSM	(7*32+12) /* Platform QoS monitoring capability */
>>  #define X86_FEATURE_NO_FPU_SEL 	(7*32+13) /* FPU CS/DS stored as zero */
>>  #define X86_FEATURE_SMAP	(7*32+20) /* Supervisor Mode Access Prevention */
>>  
>> diff --git a/xen/include/asm-x86/pqos.h b/xen/include/asm-x86/pqos.h
>> new file mode 100644
>> index 0000000..934d68a
>> --- /dev/null
>> +++ b/xen/include/asm-x86/pqos.h
> It might make sense to split this file.  The QOS_* defines could be
> argued as asm-x86, (perhaps asm-x86/qpos-defines.h)
>
> However, the cqm_res_struct and function declaration are very certainly
> not asm code, and would probably better live in arch/x86/qpos.h

Ignore this - I got my include hierarchy confused.  Sorry.

~Andrew

>
>> @@ -0,0 +1,37 @@
>> +/*
>> + * pqos.h: Platform QoS related service for guest.
>> + *
>> + * Copyright (c) 2013, Intel Corporation
>> + * Author: Jiongxi Li  <jiongxi.li@intel.com>
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
>> + * Place - Suite 330, Boston, MA 02111-1307 USA.
>> + */
>> +#ifndef ASM_PQOS_H
>> +#define ASM_PQOS_H
>> +
>> +/* QoS Resource Type Enumeration */
>> +#define QOS_MONITOR_TYPE_L3            0x2
>> +
>> +/* QoS Monitoring Event ID */
>> +#define QOS_MONITOR_EVTID_L3           0x1
>> +
>> +struct cqm_res_struct {
> This name is redundant with the extra "_struct" at the end. How about
> cqm_res or cqm_resource ?
>
>> +    domid_t  domain_id;
> You will need to include one of the types headers.
>
>> +    bool_t   inuse;
>> +};
>> +
>> +void init_platform_qos(void);
>> +
>> +#endif
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel