From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH v14 04/10] x86: detect and initialize
 Platform QoS Monitoring feature
Date: Mon, 01 Sep 2014 12:38:20 +0100
Message-ID: <540476CC020000780002F6D4@mail.emea.novell.com>
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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1409211839-21718-5-git-send-email-chao.p.peng@linux.intel.com>
Content-Disposition: inline
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: Chao Peng <chao.p.peng@linux.intel.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 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.

> +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.

> +    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?

Jan