Re: [Xen-devel] [PATCH v2 2/4] x86: add domctl cmd to set/get CDP code/data CBM

2015-09-09 Thread Chao Peng 
On Wed, Sep 09, 2015 at 01:16:46PM +0800, He Chen wrote:
> CDP extends CAT and provides the capacity to control L3 code & data
> cache. With CDP, one COS correspond to two CMBs(code & data). cbm_type
> is added to support distinguish different CBM operation. Besides, new
> domctl cmds are introdunced to support set/get CDP CBM. Some CAT
> functions to operation CBMs are extended to support CDP.
> 
> Signed-off-by: He Chen 
> ---
>  xen/arch/x86/domctl.c   |  32 -
>  xen/arch/x86/psr.c  | 163 
> ++--
>  xen/include/asm-x86/psr.h   |  12 +++-
>  xen/include/public/domctl.h |   4 ++
>  4 files changed, 170 insertions(+), 41 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index bf62a88..734fddb 100644
> --- a/xen/arch/x86/domctl.c
>  
> -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);
>  
>  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 )
> +{
> +if ( info->cdp_enabled )
> +return -EXDEV;
> +*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
> +}
> +else if ( type == PSR_CBM_TYPE_L3_CODE )
> +{
> +if ( !info->cdp_enabled )
> +return -EXDEV;
> +*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.code;
> +}
> +else
> +{
> +if ( !info->cdp_enabled )
> +return -EXDEV;
> +*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.data;
> +}

Could I suggest to use switch? And the check can be done in advance.

>  
>  return 0;
>  }
> @@ -371,65 +389,136 @@ 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 guess the '{' and '}' can be omitted if the body contains only one
sentence.

> +
> +return -ENOENT;
> +}
> +
> +static int pick_avail_cos(struct psr_cat_cbm *map, int cos_max, int old_cos)
> +{
> +int cos;
> +
> +/* If old cos is referred only by the domain, then use it. */
> +if ( map[old_cos].ref == 1 )
> +return old_cos;
> +
> +/* Then we pick an unused one, never pick 0 */

Find an unused one other than cos0.

> +for ( cos = 1; cos <= cos_max; cos++ )
> +if ( map[cos].ref == 0 )
> +return cos;
> +
> +return -ENOENT;
> +}
> +
> +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;
> +uint64_t cbm_data, cbm_code;
> +bool_t need_write = 1;
> +struct psr_cat_cbm *map;
>  struct psr_cat_socket_info *info = get_cat_socket_info(socket);
>  
>  if ( IS_ERR(info) )
>  return PTR_ERR(info);
>  
> +cbm_code = (1ull << info->cbm_len) - 1;
> +cbm_data = (1ull << info->cbm_len) - 1;

Why this is needed, as you will give them the value eventually.

>  if ( !psr_check_cbm(info->cbm_len, cbm) )
>  return -EINVAL;
>  
> +cos_max = info->cos_max;
>  old_cos = d->arch.psr_cos_ids[socket];
>  map = info->cos_to_cbm;
>  
> -spin_lock(>cbm_lock);
> -
> -for ( cos = 0; cos <= info->cos_max; cos++ )
> +switch( type )

Coding style.

Chao
>  {
> -/* 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(>cbm_lock);
> -return 0;
> -}
> -found = map + cos;
> +case PSR_CBM_TYPE_L3:
> +if ( info->cdp_enabled )
> +return -EINVAL;
> +cbm_code = cbm;
> +cbm_data = cbm;
>  break;
> -}
> -}
>  
> -/* If

Re: [Xen-devel] [PATCH v2 2/4] x86: add domctl cmd to set/get CDP code/data CBM

2015-09-09 Thread Jan Beulich 
>>> On 09.09.15 at 09:24,  wrote:
> On Wed, Sep 09, 2015 at 01:16:46PM +0800, He Chen wrote:
>> -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);
>>  
>>  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 )
>> +{
>> +if ( info->cdp_enabled )
>> +return -EXDEV;
>> +*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
>> +}
>> +else if ( type == PSR_CBM_TYPE_L3_CODE )
>> +{
>> +if ( !info->cdp_enabled )
>> +return -EXDEV;
>> +*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.code;
>> +}
>> +else
>> +{
>> +if ( !info->cdp_enabled )
>> +return -EXDEV;
>> +*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.data;
>> +}
> 
> Could I suggest to use switch? And the check can be done in advance.

Indeed. And please consider carefully what the default case ought
to be.

>> @@ -371,65 +389,136 @@ 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 guess the '{' and '}' can be omitted if the body contains only one
> sentence.

Not really: They are required on the outer if(), and hence for
consistency they should be retained on the corresponding else.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2 2/4] x86: add domctl cmd to set/get CDP code/data CBM

2015-09-08 Thread He Chen 
CDP extends CAT and provides the capacity to control L3 code & data
cache. With CDP, one COS correspond to two CMBs(code & data). cbm_type
is added to support distinguish different CBM operation. Besides, new
domctl cmds are introdunced to support set/get CDP CBM. Some CAT
functions to operation CBMs are extended to support CDP.

Signed-off-by: He Chen 
---
 xen/arch/x86/domctl.c   |  32 -
 xen/arch/x86/psr.c  | 163 ++--
 xen/include/asm-x86/psr.h   |  12 +++-
 xen/include/public/domctl.h |   4 ++
 4 files changed, 170 insertions(+), 41 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index bf62a88..734fddb 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1167,12 +1167,40 @@ long arch_do_domctl(
 {
 case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM:
 ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
- domctl->u.psr_cat_op.data);
+ domctl->u.psr_cat_op.data,
+ PSR_CBM_TYPE_L3);
+break;
+
+case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE:
+ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
+ domctl->u.psr_cat_op.data,
+ PSR_CBM_TYPE_L3_CODE);
+break;
+
+case XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA:
+ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
+ domctl->u.psr_cat_op.data,
+ PSR_CBM_TYPE_L3_DATA);
 break;
 
 case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
 ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
- >u.psr_cat_op.data);
+ >u.psr_cat_op.data,
+ PSR_CBM_TYPE_L3);
+copyback = 1;
+break;
+
+case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE:
+ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
+ >u.psr_cat_op.data,
+ PSR_CBM_TYPE_L3_CODE);
+copyback = 1;
+break;
+
+case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA:
+ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
+ >u.psr_cat_op.data,
+ PSR_CBM_TYPE_L3_DATA);
 copyback = 1;
 break;
 
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index ba0f726..c7d8356 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -290,14 +290,32 @@ 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);
 
 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 )
+{
+if ( info->cdp_enabled )
+return -EXDEV;
+*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+}
+else if ( type == PSR_CBM_TYPE_L3_CODE )
+{
+if ( !info->cdp_enabled )
+return -EXDEV;
+*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.code;
+}
+else
+{
+if ( !info->cdp_enabled )
+return -EXDEV;
+*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.data;
+}
 
 return 0;
 }
@@ -371,65 +389,136 @@ 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;
+}
+
+return -ENOENT;
+}
+
+static int pick_avail_cos(struct psr_cat_cbm *map, int cos_max, int old_cos)
+{
+int cos;
+
+/* If old cos is referred only by the domain, then use it. */
+if ( map[old_cos].ref == 1 )
+return old_cos;
+
+/* Then we pick an unused one, never pick 0 */
+for ( cos = 1; cos <= cos_max; cos++ )
+if ( map[cos].ref == 0 )
+return cos;
+
+return -ENOENT;
+}
+
+int