Re: [PATCH v14 04/10] x86: detect and initialize Platform QoS Monitoring feature

["Jan Beulich" <JBeulich@suse.com>]

>>> On 02.09.14 at 11:05, <chao.p.peng@linux.intel.com> wrote:
> On Mon, Sep 01, 2014 at 12:38:20PM +0100, Jan Beulich wrote:
>> >>> On 28.08.14 at 09:43, <chao.p.peng@linux.intel.com> wrote:
>> > +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_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);
>> 
>> Shouldn't both of these imply opt_pqos = 1, so the user can avoid
>> redundancy like "pqos=yes,pqos_monitor:yes"? I'd even think
>> "pqos=pqos_monitor" should be sufficient to enable PQoS and the
>> monitoring.
> Another sub-option pqos_mbm(memory bandwith monitor) will be added
> in the future, which can coexist with pqos_monitor. So we want both can
> be turn on/off independently. While I agree with you that to keep things
> simple. How about this: pqos=pqos_monitor|pqos_mbm,rmid_max=* ?

Yes - all the extra "yes" or "no" should be accepted, but not required.

>> > +    if ( pqosm->qm_features & QOS_MONITOR_TYPE_L3 )
>> > +    {
>> > +        cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
>> > +        pqosm->l3m.upscaling_factor = ebx;
>> > +        pqosm->l3m.rmid_max = ecx;
>> > +        pqosm->l3m.l3_features = edx;
>> > +    }
>> > +
>> > +    pqosm->rmid_max = min(rmid_max, pqosm->l3m.rmid_max);
>> > +    pqosm->rmid_to_dom = xmalloc_array(domid_t, pqosm->rmid_max + 1);
>> 
>> ... this is what isn't going to do well.
> Nomally the rmid_max comes from hardware should not have chance to
> degenerate to 0xffffffff, do you mean we protect it with rmid_mask?

Then simply BUG_ON() rather then silently doing the wrong thing?

Jan