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

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, February 24, 2014 10:13 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; Ian.Campbell@xxxxxxxxxx;
> Ian.Jackson@xxxxxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> xen-devel@xxxxxxxxxxxxx; konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx;
> keir@xxxxxxx
> Subject: Re: [PATCH v9 1/6] x86: detect and initialize Cache QoS Monitoring
> feature
> 
> >>> On 19.02.14 at 07:32, Dongxiao Xu <dongxiao.xu@xxxxxxxxx> wrote:
> > +struct pqos_cqm __read_mostly *cqm = NULL;
> 
> Misplaced __read_mostly (belongs after the *).

Okay.

> 
> > +static void __init init_cqm(void)
> > +{
> > +    unsigned int rmid;
> > +    unsigned int eax, edx;
> > +    unsigned int cqm_pages;
> > +    unsigned int i;
> > +
> > +    if ( !opt_cqm_max_rmid )
> > +        return;
> > +
> > +    cqm = xzalloc(struct pqos_cqm);
> > +    if ( !cqm )
> > +        return;
> > +
> > +    cpuid_count(0xf, 1, &eax, &cqm->upscaling_factor, &cqm->max_rmid,
> &edx);
> > +    if ( !(edx & QOS_MONITOR_EVTID_L3) )
> > +        goto out;
> > +
> > +    cqm->min_rmid = 1;
> > +    cqm->max_rmid = min(opt_cqm_max_rmid, cqm->max_rmid);
> > +
> > +    cqm->rmid_to_dom = xmalloc_array(domid_t, cqm->max_rmid + 1);
> > +    if ( !cqm->rmid_to_dom )
> > +        goto out;
> > +
> > +    /* Reserve RMID 0 for all domains not being monitored */
> > +    cqm->rmid_to_dom[0] = DOMID_XEN;
> > +    for ( rmid = cqm->min_rmid; rmid <= cqm->max_rmid; rmid++ )
> > +        cqm->rmid_to_dom[rmid] = DOMID_INVALID;
> > +
> > +    /* Allocate CQM buffer size in initialization stage */
> > +    cqm_pages = ((cqm->max_rmid + 1) * sizeof(domid_t) +
> > +                (cqm->max_rmid + 1) * sizeof(uint64_t) * NR_CPUS)/
> 
> Does this really need to be NR_CPUS (rather than nr_cpu_ids)?

Okay.
As you mentioned in later comment, the CQM data is indexed per-socket.
Here we use NR_CPUS or nr_cpu_ids because it is big enough to cover the 
possible socket number in the system (even consider hotplug case).
Is there a better way that we can get the system socket number (including the 
case even there is no CPU in that socket)?

> 
> > +                PAGE_SIZE + 1;
> > +    cqm->buffer_size = cqm_pages * PAGE_SIZE;
> > +
> > +    cqm->buffer =
> alloc_xenheap_pages(get_order_from_pages(cqm_pages), 0);
> 
> And does the allocation really need to be physically contiguous?
> If so - did you calculate how much more memory you allocate
> (due to the rounding up to the next power of 2), to decide
> whether it's worthwhile freeing the unused portion?

The buffer needs to be contiguous since userspace will map it as read-only to 
avoid data copy.

According to my rough calculation, the cqm_pages value is about 29 pages on my 
test machine.


> 
> > +    if ( !cqm->buffer )
> > +    {
> > +        xfree(cqm->rmid_to_dom);
> > +        goto out;
> > +    }
> > +    memset(cqm->buffer, 0, cqm->buffer_size);
> > +
> > +    for ( i = 0; i < cqm_pages; i++ )
> > +        share_xen_page_with_privileged_guests(
> > +            virt_to_page((void *)((unsigned long)cqm->buffer + i *
> PAGE_SIZE)),
> 
> virt_to_page((void *)cqm->buffer + i * PAGE_SIZE)

Okay.

> 
> > +static void __init init_qos_monitor(void)
> > +{
> > +    unsigned int qm_features;
> > +    unsigned int eax, ebx, ecx;
> > +
> > +    if ( !(boot_cpu_has(X86_FEATURE_QOSM)) )
> 
> Pointless pair of parentheses.

Okay.

> 
> > +        return;
> > +
> > +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &qm_features);
> > +
> > +    if ( opt_cqm && (qm_features & QOS_MONITOR_TYPE_L3) )
> > +        init_cqm();
> > +}
> > +
> > +void __init init_platform_qos(void)
> > +{
> > +    if ( !opt_pqos )
> > +        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 b49256d..639528f 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;
> > @@ -1419,6 +1420,8 @@ void __init __start_xen(unsigned long mbi_p)
> >
> >      domain_unpause_by_systemcontroller(dom0);
> >
> > +    init_platform_qos();
> > +
> >      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..0a8065c
> > --- /dev/null
> > +++ b/xen/include/asm-x86/pqos.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * pqos.h: Platform QoS related service for guest.
> > + *
> > + * Copyright (c) 2014, 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.
> > + */
> > +#ifndef ASM_PQOS_H
> > +#define ASM_PQOS_H
> > +
> > +#include <public/xen.h>
> > +#include <xen/spinlock.h>
> > +
> > +/* QoS Resource Type Enumeration */
> > +#define QOS_MONITOR_TYPE_L3            0x2
> > +
> > +/* QoS Monitoring Event ID */
> > +#define QOS_MONITOR_EVTID_L3           0x1
> > +
> > +struct pqos_cqm {
> > +    spinlock_t cqm_lock;
> > +    uint64_t *buffer;
> > +    unsigned int min_rmid;
> > +    unsigned int max_rmid;
> > +    unsigned int used_rmid;
> > +    unsigned int upscaling_factor;
> > +    unsigned int buffer_size;
> > +    domid_t *rmid_to_dom;
> > +};
> > +extern struct pqos_cqm *cqm;
> > +
> > +void init_platform_qos(void);
> > +
> > +#endif
> > --
> > 1.7.9.5
> 


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