|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 2/4] x86: add domctl cmd to set/get CDP code/data CBM
>>> On 14.09.15 at 05:27, <he.chen@xxxxxxxxxxxxxxx> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |