Re: [Xen-devel] [PATCH v13 04/10] x86: detect and initialize Platform QoS Monitoring feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.08.14 at 04:17, <dongxiao.xu@xxxxxxxxx> wrote:
> +struct pqos_monitor *__read_mostly pqosm = NULL;
> +static bool_t __initdata opt_pqos = 0;

Pointless initializer (at least on statics you should really avoid it; I can
see reasons to leave it on the non-static one line earlier even though
it's mostly pointless there too).

> +static bool_t __initdata opt_pqos_monitor = 1;
> +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;

Beyond this point you don't use "val" anymore, i.e. other than for
skipping the "no-" prefix you don't really use it, which can't be
correct.

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

Also - namely in the context of the generic command line parser
changes to this effect - be careful to not permit
"no-rmid_max=<value>".

> +static void __init init_pqos_monitor(unsigned int rmid_max)
> +{
> +    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);

Perhaps guard against this degenerating to 0xffffffff, making the
operations below not what you intend, but also not fail?

> +struct pqos_monitor {
> +    unsigned long rmid_mask;
> +    unsigned int rmid_min;

Do you really need this field? It seems to only ever be 1.

Jan


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