Re: [Xen-devel] [PATCH 2/6] x86: add support for COS/CBM manangement

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.15 at 11:13, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1324,6 +1324,24 @@ long arch_do_domctl(
>          }
>          break;
>  
> +    case XEN_DOMCTL_psr_cat_op:
> +        switch ( domctl->u.psr_cat_op.cmd )
> +        {
> +        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);
> +            break;
> +        case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
> +            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> +                                 &domctl->u.psr_cat_op.data);
> +            copyback = 1;
> +            break;
> +        default:
> +            ret = -ENOSYS;
> +            break;

Please leave -ENOSYS to unimplemented hypercalls. -EOPNOTSUPP
or -EINVAL seem to be better candidates here.

> +static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm)
> +{
> +    unsigned int first_bit, zero_bit;
> +
> +    /* Set bits should only in the range of [0, cbm_len) */
> +    if ( cbm & (~0ull << cbm_len) )
> +        return 0;
> +
> +    /* At least two contiguous bits need to be set */
> +    if ( hweight_long(cbm) < 2 )
> +        return 0;
> +
> +    first_bit = find_first_bit(&cbm, cbm_len);
> +    zero_bit = find_next_zero_bit(&cbm, cbm_len, first_bit);
> +
> +    /* Set bits should be contiguous */
> +    if ( find_next_bit(&cbm, cbm_len, zero_bit) < cbm_len )

You can't reliably invoke this without having checked that
zero_bit is less than cbm_len (undefined behavior may result).

> +static void write_l3_cbm(unsigned int socket, unsigned int cos, uint64_t cbm)
> +{
> +    struct cos_cbm_info info = {.cos = cos, .cbm = cbm };
> +
> +    if ( socket == cpu_to_socket(smp_processor_id()) )
> +        do_write_l3_cbm(&info);
> +    else
> +    {
> +        unsigned int cpu = cpumask_check(get_socket_cpu(socket));

Isn't this going to trigger an assertion when the socket count got
specified on the command line?

> +int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
> +{
> +    unsigned int old_cos, cos;
> +    struct psr_cat_cbm *map, *find;
> +    struct psr_cat_socket_info *info;
> +    int ret = get_cat_socket_info(socket, &info);
> +
> +    if ( ret )
> +        return ret;
> +
> +    if ( !psr_check_cbm(info->cbm_len, cbm) )
> +        return -EINVAL;
> +
> +    old_cos = d->arch.psr_cos_ids[socket];
> +    map = info->cos_cbm_map;
> +    find = NULL;
> +
> +    for ( cos = 0; cos <= info->cos_max; cos++ )
> +    {
> +        /* If still no found, then keep this unused one */
> +        if ( !find && cos != 0 && map[cos].ref == 0 )
> +            find = map + cos;
> +        else if ( map[cos].cbm == cbm )
> +        {
> +            if ( unlikely(cos == old_cos) )
> +                return -EEXIST;
> +            find = map + cos;
> +            break;
> +        }
> +    }

Please add a comment explaining that this is implicitly serialized
via the domctl lock.

> +static void psr_free_cos(struct domain *d)
> +{
> +    unsigned int socket;
> +    unsigned int cos;
> +    struct psr_cat_cbm *map;
> +
> +    if( !d->arch.psr_cos_ids )
> +        return;

Considering this check ...

> +    for ( socket = 0; socket < opt_socket_num; socket++)
> +    {
> +        cos = d->arch.psr_cos_ids[socket];
> +        if ( cos == 0 )
> +            continue;
> +
> +        map = cat_socket_info[socket].cos_cbm_map;
> +        if ( map )
> +            map[cos].ref--;
> +    }
> +
> +    xfree(d->arch.psr_cos_ids);

... I think you want to clear the pointer here.

> @@ -222,6 +422,17 @@ static void do_cat_cpu_init(void* data)
>          info->cbm_len = (eax & 0x1f) + 1;

This means cbm_len <= 32. Why is cos_cbm_map[].cbm then a
uint64_t?

Jan


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