From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH 2/6] x86: add support for COS/CBM manangement
Date: Mon, 16 Mar 2015 17:10:32 +0000
Message-ID: <55071C98020000780006A70A@mail.emea.novell.com>
References: <1426241605-4114-1-git-send-email-chao.p.peng@linux.intel.com>
	<1426241605-4114-3-git-send-email-chao.p.peng@linux.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1426241605-4114-3-git-send-email-chao.p.peng@linux.intel.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Chao Peng <chao.p.peng@linux.intel.com>
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.xen.org, will.auld@intel.com, keir@xen.org, dgdegra@tycho.nsa.gov
List-Id: xen-devel@lists.xenproject.org

>>> On 13.03.15 at 11:13, <chao.p.peng@linux.intel.com> 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