Re: [Xen-devel] [PATCH v19 04/10] x86: detect and initialize Cache Monitoring Technology feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.10.14 at 13:35, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:

Again only cosmetic comments (which can be taken care of while
committing):

> +static void __init parse_psr_param(char *s)
> +{
> +    char *ss, *val_str;
> +    int val_int;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        val_str = strchr(s, ':');
> +        if ( val_str )
> +            *val_str++ = '\0';
> +
> +        if ( !strcmp(s, "cmt") )
> +        {
> +            if ( !val_str )
> +                opt_psr |= PSR_CMT;
> +            else
> +            {
> +                val_int = parse_bool(val_str);

This could be the declaration (with initializer).

> +                if ( val_int == 1 )
> +                    opt_psr |= PSR_CMT;
> +                else if ( val_int == -1 )

val_int != 0 (for forward compatibility).

> +                    printk("PSR: unknown cmt value: %s - CMT disabled!\n",
> +                                    val_str);
> +            }
> +        }
> +        else if ( val_str && !strcmp(s, "rmid_max") )
> +            opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> +
> +        s = ss + 1;
> +    } while ( ss );
> +}
> +custom_param("psr", parse_psr_param);
> +
> +static void __init init_psr_cmt(unsigned int rmid_max)
> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    unsigned int rmid;
> +
> +    if ( !boot_cpu_has(X86_FEATURE_CMT) )
> +        return;
> +
> +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &edx);
> +    if ( !edx )
> +        return;
> +
> +    psr_cmt = xzalloc(struct psr_cmt);
> +    if ( !psr_cmt )
> +        return;
> +
> +    psr_cmt->features = edx;
> +    psr_cmt->rmid_max = min(rmid_max, ebx);
> +    rmid_mask = ~(~0ull << get_count_order(ebx));
> +
> +    if ( psr_cmt->features & PSR_RESOURCE_TYPE_L3 )
> +    {
> +        cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
> +        psr_cmt->l3.upscaling_factor = ebx;
> +        psr_cmt->l3.rmid_max = ecx;
> +        psr_cmt->l3.features = edx;
> +    }
> +
> +    psr_cmt->rmid_max = min(psr_cmt->rmid_max, psr_cmt->l3.rmid_max);
> +
> +    BUG_ON(psr_cmt->rmid_max == 0xffffffff);

This would stop working as intended if rmid_max's type got changed.
All you want to avoid the array size expression in the allocation to
degenerate to zero, i.e. checking exactly that condition would be
better:

    BUG_ON(!(psr_cmt->rmid_max + 1));

> +    psr_cmt->rmid_to_dom = xmalloc_array(domid_t, psr_cmt->rmid_max + 1);

But anyway, using 1UL here still would seem the preferred way of
dealing with this over using BUG_ON().

Jan


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