From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v14 04/10] x86: detect and initialize Platform QoS Monitoring feature Date: Tue, 02 Sep 2014 10:30:24 +0100 Message-ID: <5405AA50020000780002FC63@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> <540476CC020000780002F6D4@mail.emea.novell.com> <20140902090544.GD15872@pengc-linux> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140902090544.GD15872@pengc-linux> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chao Peng 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 02.09.14 at 11:05, wrote: > On Mon, Sep 01, 2014 at 12:38:20PM +0100, Jan Beulich wrote: >> >>> On 28.08.14 at 09:43, 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