Re: [Xen-devel] [PATCH v8 09/24] x86: refactor psr: set value: assemble features value array.

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Feb 15, 2017 at 04:49:24PM +0800, Yi Sun wrote:
[...]
>  
> +static unsigned int l3_cat_get_cos_num(const struct feat_node *feat)
> +{
> +    return 1;
> +}
> +
> +static int l3_cat_get_old_val(uint64_t val[],

And the length of val is? How can you bound-check the access?

But I *think* this is just a pointer to uint64_t, you can just use
uint64_t *val here and *val = x; in code?

Same comment applies to the set_new_val handler as well.

> +                              const struct feat_node *feat,
> +                              unsigned int old_cos)
> +{
> +    if ( old_cos > feat->info.l3_cat_info.cos_max )
> +        /* Use default value. */
> +        old_cos = 0;
> +
> +    /* CAT */
> +    val[0] =  feat->cos_reg_val[old_cos];
> +
> +    return 0;
> +}
> +
> +static int l3_cat_set_new_val(uint64_t val[],
> +                              const struct feat_node *feat,
> +                              enum cbm_type type,
> +                              uint64_t m)
> +{
> +    if ( !psr_check_cbm(feat->info.l3_cat_info.cbm_len, m) )
> +        return -EINVAL;
> +
> +    val[0] = m;
> +
> +    return 0;
> +}
> +
>  static const struct feat_ops l3_cat_ops = {
>      .get_cos_max = l3_cat_get_cos_max,
>      .get_feat_info = l3_cat_get_feat_info,
>      .get_val = l3_cat_get_val,
> +    .get_cos_num = l3_cat_get_cos_num,
> +    .get_old_val = l3_cat_get_old_val,
> +    .set_new_val = l3_cat_set_new_val,
>  };
>  
>  static void __init parse_psr_bool(char *s, char *value, char *feature,
> @@ -549,15 +633,42 @@ 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;
> +    const struct feat_node *feat_tmp;
> +    unsigned int num = 0;
> +
> +    /* Get all features total amount. */
> +    list_for_each_entry(feat_tmp, &info->feat_list, list)
> +        num += feat_tmp->ops.get_cos_num(feat_tmp);
> +
> +    return num;
>  }
>  
> -static int assemble_val_array(uint64_t *val,
> +static int combine_val_array(uint64_t *val,
>                                uint32_t array_len,
>                                const struct psr_socket_info *info,
>                                unsigned int old_cos)

Please just name this function combine_val_array in your previous patch
instead of trying to rename it here. Or just don't change the name at
all -- I don't see why changing name is necessary.

Wei.

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