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

[Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>]

On Mon, Sep 14, 2015 at 11:27:05AM +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
                                    ^s
> is added to support distinguish different CBM operation. Besides, new
              ^'support' can be ommitted.                ^s
> 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 <he.chen@xxxxxxxxxxxxxxx>

> +
> +    /* If old cos is referred only by the domain, then use it. */
> +    if ( map[old_cos].ref == 1 )
> +        return old_cos;
> +
> +    /* Find an unused one other than cos0 */

Missing stop.

> +    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;
> +    bool_t cdp_enabled = test_bit(socket, cdp_socket_enable);
> +    struct psr_cat_cbm *map;
>      struct psr_cat_socket_info *info = get_cat_socket_info(socket);
>  
>      if ( IS_ERR(info) )
> @@ -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) )

Nice code can be made by aligning PSR_CBM_TYPE_L3_DATA 
with 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 )

Coding style.

> +    {
> +        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 */

Missing stop.


Overall it looks only some minor issues, with these being fixed, then

Reviewed-by: Chao peng <chao.p.peng@xxxxxxxxxxxxxxx>

Chao

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