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

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Thursday, November 21, 2013 8:14 PM
> To: Xu, Dongxiao
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2 1/8] x86: detect and initialize Cache QoS
> Monitoring feature
> 
> On 21/11/13 07:20, dongxiao.xu@xxxxxxxxx wrote:
> > From: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxx>
> > + * 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.
> > + *
> > + * 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.

OK.

> 
> > +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

That's a good point.
I will add a range check here.

> 
> > +    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?

In this design, we monitored per domain basis, including all the L3 cache usage 
within both guest and hypervisor mode.
Dom0 can also be monitored by "xl pqos-attach cqm 0".

However for specific Xen's L3 usage, it is not covered in this design, since we 
can only write one RMID into the hardware at a time.

> 
> >      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
> 
> > @@ -0,0 +1,37 @@
> > +/*
> > + * pqos.h: Platform QoS related service for guest.
> > + *
> > + * Copyright (c) 2013, Intel Corporation
> > + * Author: Jiongxi Li  <jiongxi.li@xxxxxxxxx>
> > + * 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.
> > + *
> > + * 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 ?

OK.

> 
> > +    domid_t  domain_id;
> 
> You will need to include one of the types headers.

Sorry maybe I didn't quite understand your point here... But, domid_t's 
definition header file will be included in the arch/x86/pqos.c file before 
pqos.h is included.

> 
> > +    bool_t   inuse;
> > +};
> > +
> > +void init_platform_qos(void);
> > +
> > +#endif


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