From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chao Peng <chao.p.peng@linux.intel.com>
Subject: Re: [PATCH v14 04/10] x86: detect and initialize
 Platform QoS Monitoring feature
Date: Tue, 2 Sep 2014 17:05:44 +0800
Message-ID: <20140902090544.GD15872@pengc-linux>
References: <1409211839-21718-1-git-send-email-chao.p.peng@linux.intel.com>
	<1409211839-21718-5-git-send-email-chao.p.peng@linux.intel.com>
	<540476CC020000780002F6D4@mail.emea.novell.com>
Reply-To: Chao Peng <chao.p.peng@linux.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <540476CC020000780002F6D4@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, dgdegra@tycho.nsa.gov
List-Id: xen-devel@lists.xenproject.org

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