From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v3 2/4] x86: add domctl cmd to set/get CDP code/data CBM Date: Mon, 14 Sep 2015 08:28:20 -0600 Message-ID: <55F6F5A402000078000A29E0@prv-mh.provo.novell.com> References: <1442201227-8610-1-git-send-email-he.chen@linux.intel.com> <1442201227-8610-3-git-send-email-he.chen@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZbUk9-0001zb-EM for xen-devel@lists.xenproject.org; Mon, 14 Sep 2015 14:28:29 +0000 In-Reply-To: <1442201227-8610-3-git-send-email-he.chen@linux.intel.com> 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: He Chen Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xenproject.org, chao.p.peng@linux.intel.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org >>> On 14.09.15 at 05:27, wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -288,14 +288,39 @@ int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len, > return 0; > } > > -int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm) > +int psr_get_l3_cbm(struct domain *d, unsigned int socket, > + uint64_t *cbm, enum cbm_type type) > { > struct psr_cat_socket_info *info = get_cat_socket_info(socket); > + bool_t cdp_enabled = test_bit(socket, cdp_socket_enable); > > if ( IS_ERR(info) ) > return PTR_ERR(info); > > - *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm; > + if ( type == PSR_CBM_TYPE_L3 && cdp_enabled ) > + return -EXDEV; > + > + if ( (type == PSR_CBM_TYPE_L3_CODE || type == PSR_CBM_TYPE_L3_DATA) > + && !cdp_enabled ) > + return -EXDEV; These checks really ought to go into the subsequent switch(). But then I wonder why asking for code or data would be wrong when !cdp_enabled? You could simple return the same mask for all three variants in that case. > + switch ( type ) > + { > + case PSR_CBM_TYPE_L3: > + *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm; > + break; > + > + case PSR_CBM_TYPE_L3_CODE: > + *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.code; > + break; > + > + case PSR_CBM_TYPE_L3_DATA: > + *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.data; > + break; > + > + default: > + return -EINVAL; Considering that this is a helper function and "type" comes from a caller inside the hypervisor, I'd really like to see an ASSERT_UNREACHABLE() here. > @@ -369,10 +394,53 @@ static int write_l3_cbm(unsigned int socket, unsigned int cos, > return 0; > } > > -int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm) > +static int find_cos(struct psr_cat_cbm *map, int cos_max, > + uint64_t cbm_code, uint64_t cbm_data, bool_t cdp_enabled) > { > - unsigned int old_cos, cos; > - struct psr_cat_cbm *map, *found = NULL; > + unsigned int cos; > + > + if ( !cdp_enabled ) > + { > + for ( cos = 0; cos <= cos_max; cos++ ) > + if ( map[cos].ref && map[cos].u.cbm == cbm_code ) > + return cos; > + } > + else > + { > + for ( cos = 0; cos <= cos_max; cos++ ) > + if ( map[cos].ref && map[cos].u.cdp.code == cbm_code && > + map[cos].u.cdp.data == cbm_data ) > + return cos; > + } I think having just a single loop here with a suitable conditional expression inside the if() would both shrink code size and allow the compiler to produce better (smaller) code. > +static int pick_avail_cos(struct psr_cat_cbm *map, int cos_max, int old_cos) > +{ > + int cos; In the function right above "cos" was unsigned int, which I think is the correct type (also for "old_cos" and "cos_max"). > +int psr_set_l3_cbm(struct domain *d, unsigned int socket, > + uint64_t cbm, enum cbm_type type) > +{ > + unsigned int old_cos, cos_max; > + int cos, ret; Whereas here I can see why it should be plain int. > @@ -381,53 +449,79 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm) > if ( !psr_check_cbm(info->cbm_len, cbm) ) > return -EINVAL; > > + if ( !cdp_enabled && (type == PSR_CBM_TYPE_L3_CODE || > + type == PSR_CBM_TYPE_L3_DATA) ) > + return -EINVAL; > + > + cos_max = info->cos_max; > old_cos = d->arch.psr_cos_ids[socket]; > map = info->cos_to_cbm; > > - spin_lock(&info->cbm_lock); > > - for ( cos = 0; cos <= info->cos_max; cos++ ) > + switch( type ) > { > - /* If still not found, then keep unused one. */ > - if ( !found && cos != 0 && map[cos].ref == 0 ) > - found = map + cos; > - else if ( map[cos].u.cbm == cbm ) > - { > - if ( unlikely(cos == old_cos) ) > - { > - ASSERT(cos == 0 || map[cos].ref != 0); > - spin_unlock(&info->cbm_lock); > - return 0; > - } > - found = map + cos; > + case PSR_CBM_TYPE_L3: > + cbm_code = cbm; > + cbm_data = cbm; Wrong indentation. > break; > - } > - } > > - /* If old cos is referred only by the domain, then use it. */ > - if ( !found && map[old_cos].ref == 1 ) > - found = map + old_cos; > + case PSR_CBM_TYPE_L3_CODE: > + cbm_code = cbm; > + cbm_data = map[old_cos].u.cdp.data; > + break; > > - if ( !found ) > - { > - spin_unlock(&info->cbm_lock); > - return -EOVERFLOW; > + case PSR_CBM_TYPE_L3_DATA: > + cbm_code = map[old_cos].u.cdp.code; > + cbm_data = cbm; > + break; > + > + default: > + return -EINVAL; > } > > - cos = found - map; > - if ( found->u.cbm != cbm ) > + spin_lock(&info->cbm_lock); > + cos = find_cos(map, cos_max, cbm_code, cbm_data, cdp_enabled); > + if ( cos >= 0 ) > { > - int ret = write_l3_cbm(socket, cos, cbm, 0, 0); > - > - if ( ret ) > + if ( unlikely(cos == old_cos) ) Are you sure about the unlikely() here? > { > spin_unlock(&info->cbm_lock); > - return ret; > + return 0; > + } > + } > + else > + { > + cos = pick_avail_cos(map, cos_max, old_cos); > + if ( cos < 0 ) > + { > + spin_unlock(&info->cbm_lock); > + return cos; > + } > + > + /* We try to avoid writing MSR */ > + if ( cdp_enabled ) > + { > + if ( map[cos].u.cdp.code == cbm_code && > + map[cos].u.cdp.data == cbm_data ) > + need_write = 0; > + } > + else > + need_write = map[cos].u.cbm == cbm_code ? 0 : 1; Please write such without conditional expression (a comparison on the right side of an assignment is quite fine). Also the scope of "need_write" would better be reduced to the minimal required one. > --- a/xen/include/asm-x86/psr.h > +++ b/xen/include/asm-x86/psr.h > @@ -49,6 +49,12 @@ struct psr_cmt { > struct psr_cmt_l3 l3; > }; > > +enum cbm_type { > + PSR_CBM_TYPE_L3 = 1, > + PSR_CBM_TYPE_L3_CODE = 2, > + PSR_CBM_TYPE_L3_DATA = 3, It doesn't look like you require these values to be 1, 2, and 3 respectively. Please omit explicit values in such cases. Jan