Re: [Xen-devel] [PATCH v12 09/23] x86: refactor psr: L3 CAT: set value: assemble features value array.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/14/17 3:25 AM >>>
> @@ -592,7 +615,14 @@ int psr_get_val(struct domain *d, unsigned int socket,
>  /* Set value functions */
>  static unsigned int get_cos_num(const struct psr_socket_info *info)
>  {
> -    return 0;
> +    unsigned int num = 0, i;
> +
> +    /* Get all features total amount. */
> +    for ( i = 0; i < ARRAY_SIZE(feat_props); i++ )
> +        if ( feat_props[i] )
> +            num += feat_props[i]->cos_num;
> +
> +    return num;
>  }

The function parameter isn't being used afaics (neither by later patches), so
please drop it (in the earlier patch introducing the function).

> @@ -600,7 +630,47 @@ static int gather_val_array(uint32_t val[],
>                              const struct psr_socket_info *info,
>                              unsigned int old_cos)
>  {
> -    return -EINVAL;
> +    unsigned int i;
> +
> +    if ( !val )
> +        return -EINVAL;
> +
> +    /* Get all features current values according to old_cos. */
> +    for ( i = 0; i < ARRAY_SIZE(info->features); i++ )
> +    {
> +        unsigned int cos = old_cos, j;
> +        const struct feat_node *feat = info->features[i];
> +        const struct feat_props *props = feat_props[i];
> +
> +        if ( !feat )
> +            continue;
> +
> +        if ( !props )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return -ENOENT;
> +        }
> +
> +        if ( array_len < props->cos_num )
> +            return -ENOSPC;
> +
> +        /*
> +         * If old_cos exceeds current feature's cos_max, we should get
> +         * default value. So assign cos to 0 which stores default value.
> +         */
> +        if ( cos > feat->cos_max )
> +            cos = 0;
> +
> +        /* Value getting order is same as feature array. */
> +        for ( j = 0; j < props->cos_num; j++ )
> +            val[j] = feat->cos_reg_val[cos * props->cos_num + j];
> +
> +        array_len -= props->cos_num;
> +
> +        val += props->cos_num;

I don't think there should be a blank line between these two ones.

> @@ -610,6 +680,60 @@ static int insert_val_into_array(uint32_t val[],
>                                   enum cbm_type type,
>                                   uint32_t new_val)
>  {
> +    const struct feat_node *feat;
> +    const struct feat_props *props;
> +    unsigned int i;
> +
> +    ASSERT(feat_type < PSR_SOCKET_FEAT_NUM);
> +
> +    /* Insert new value into array according to feature's position in array. 
> */
> +    for ( i = 0; i < feat_type; i++ )
> +    {
> +        if ( !info->features[i] )
> +            continue;
> +
> +        props = feat_props[i];
> +        if ( !props )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return -ENOENT;

How about you use "continue" here and drop the info->features[i] check above?

In any event
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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