Re: [Xen-devel] [PATCH v12 4/9] x86: detect and initialize Platform QoS Monitoring feature

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, July 04, 2014 7:57 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; Ian.Campbell@xxxxxxxxxx;
> George.Dunlap@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: Re: [PATCH v12 4/9] x86: detect and initialize Platform QoS 
> Monitoring
> feature
> 
> >>> On 04.07.14 at 10:34, <dongxiao.xu@xxxxxxxxx> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -863,6 +863,20 @@ This option can be specified more than once (up to 8
> times at present).
> >  ### ple\_window
> >  > `= <integer>`
> >
> > +### pqos (Intel)
> > +> `= List of ( <boolean> | pqos_monitor:<boolean> | rmid_max:<integer> )`
> > +
> > +> Default: `pqos=1,pqos_monitor:1,rmid_max:255`
> 
> Please keep this in sync with the actual code.
> 
> > +struct pqos_monitor *__read_mostly pqosm = NULL;
> > +static bool_t __initdata opt_pqos = 0;
> > +static bool_t __initdata opt_pqos_monitor = 0;
> 
> So as said in response to the overview mail already - I think it's fine to
> have the sub-option enabled by default, and just keeping the master
> one off for now. In any event you don't need explicit 0 initializers for
> static data.
> 
> > +static unsigned int __initdata opt_rmid_max = 255;
> > +
> > +static void __init parse_pqos_param(char *s)
> > +{
> > +    char *ss, *val_str;
> > +    int val;
> > +
> > +    do {
> > +        ss = strchr(s, ',');
> > +        if ( ss )
> > +            *ss = '\0';
> > +
> > +        val = parse_bool(s);
> > +        if ( val >= 0 )
> > +            opt_pqos = val;
> > +        else
> > +        {
> > +            val = !!strncmp(s, "no-", 3);
> > +            if ( !val )
> > +                s += 3;
> > +
> > +            val_str = strchr(s, ':');
> > +            if ( val_str )
> > +                *val_str++ = '\0';
> > +
> > +            if ( val_str && !strcmp(s, "pqos_monitor") &&
> > +                 (val = parse_bool(val_str)) >= 0 )
> > +                opt_pqos_monitor = val;
> > +            else if ( val_str && !strcmp(s, "rmid_max") )
> > +                opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> > +        }
> > +
> > +        s = ss + 1;
> > +    } while ( ss );
> > +}
> > +
> > +custom_param("pqos", parse_pqos_param);
> > +static void __init init_pqos_monitor(unsigned int rmid_max)
> 
> The blank line should be below custom_param() rather than above it.
> 
> > +{
> > +    unsigned int eax, ebx, ecx, edx;
> > +    unsigned int rmid;
> > +
> > +    if ( !boot_cpu_has(X86_FEATURE_QOSM) )
> > +        return;
> > +
> > +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &edx);
> > +    if ( !edx )
> > +        return;
> > +
> > +    pqosm = xzalloc(struct pqos_monitor);
> > +    if ( !pqosm )
> > +        return;
> > +
> > +    pqosm->qm_features = edx;
> > +    pqosm->rmid_mask = ~(~0ull << get_count_order(ebx));
> > +    pqosm->rmid_inuse = 0;
> > +    pqosm->rmid_min = 1;
> > +    pqosm->rmid_max = min(rmid_max, ebx);
> > +
> > +    if ( pqosm->qm_features & QOS_MONITOR_TYPE_L3 ) {
> 
> Coding style (also further down).
> 
> > --- /dev/null
> > +++ b/xen/include/asm-x86/pqos.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * pqos.h: Platform QoS 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_PQOS_H__
> > +#define __ASM_PQOS_H__
> > +
> > +#include <public/xen.h>
> > +
> > +/* QoS Resource Type Enumeration */
> > +#define QOS_MONITOR_TYPE_L3            0x2
> > +
> > +/* L3 Monitoring Features */
> > +#define L3_FEATURE_OCCUPANCY           0x1
> > +
> > +struct pqos_monitor_l3 {
> > +    unsigned int l3_features;
> > +    unsigned int upscaling_factor;
> > +    unsigned int rmid_max;
> > +};
> > +
> > +struct pqos_monitor {
> > +    unsigned long rmid_mask;
> > +    unsigned int rmid_min;
> > +    unsigned int rmid_max;
> > +    unsigned int rmid_inuse;
> > +    unsigned int qm_features;
> > +    domid_t *rmid_to_dom;
> > +    struct pqos_monitor_l3 l3m;
> > +};
> 
> Does any of the above get used outside of pqos.c? Anything used only
> locally (until the end of the series of course) should be moved there.

The later patch 6/9 will reference the structure in sysctl.c.

Thanks,
Dongxiao

> 
> > +extern struct pqos_monitor *pqosm;
> > +
> > +void __init init_platform_qos(void);
> 
> No __init annotaion on declarations please (these only belong on
> definitions), even more so without including xen/init.h.
> 
> Jan

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