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

[Chao Peng <chao.p.peng@linux.intel.com>]

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=* ?

chao
> 
> > +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_max = min(rmid_max, ebx);
> 
> Quoting my comment on v13: "Perhaps guard against this
> degenerating to 0xffffffff, making the operations below not what
> you intend, but also not fail?" In particular ...
> 
> > +    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?

Chao
> 
> > +    if ( !pqosm->rmid_to_dom )
> > +    {
> > +        xfree(pqosm);
> > +        return;
> > +    }
> > +    /* Reserve RMID 0 for all domains not being monitored */
> > +    pqosm->rmid_to_dom[0] = DOMID_XEN;
> > +    for ( rmid = 1; rmid <= pqosm->rmid_max; rmid++ )
> > +        pqosm->rmid_to_dom[rmid] = DOMID_INVALID;
> > +
> > +    printk(XENLOG_INFO "Platform QoS Monitoring Enabled.\n");
> > +}
> > +
> > +void __init init_platform_qos(void)
> > +{
> > +    if ( opt_pqos && opt_pqos_monitor && opt_rmid_max )
> 
> Actually - what's the purpose of the pqos_monitor sub-option
> on the command line with it being possible to disable monitoring
> with "rmid_max=0"? Was it that this is solely in preparation of
> future other QoS things?

So yes, it is meaningful for future features. We can't bind that to pqos_monitor.

Chao
> 
> Jan