Re: [Xen-devel] [PATCH v4 07/15] x86: implement set value flow for MBA

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sat, Sep 23, 2017 at 09:48:16AM +0000, Yi Sun wrote:
> @@ -274,29 +277,6 @@ static enum psr_feat_type psr_type_to_feat_type(enum 
> psr_type type)
>      return feat_type;
>  }
>  
> -static bool psr_check_cbm(unsigned int cbm_len, unsigned long cbm)
> -{
> -    unsigned int first_bit, zero_bit;
> -
> -    /* Set bits should only in the range of [0, cbm_len]. */
> -    if ( cbm & (~0ul << cbm_len) )
> -        return false;
> -
> -    /* At least one bit need to be set. */
> -    if ( cbm == 0 )
> -        return false;
> -
> -    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 ( zero_bit < cbm_len &&
> -         find_next_bit(&cbm, cbm_len, zero_bit) < cbm_len )
> -        return false;
> -
> -    return true;
> -}
> -
>  /* Implementation of allocation features' functions. */
>  static bool cat_init_feature(const struct cpuid_leaf *regs,
>                              struct feat_node *feat,
> @@ -426,11 +406,36 @@ static bool cat_get_feat_info(const struct feat_node 
> *feat,
>      return true;
>  }
>  
> +static bool cat_check_cbm(const struct feat_node *feat, unsigned long cbm)
> +{
> +    unsigned int first_bit, zero_bit;
> +    unsigned int cbm_len = feat->cat.cbm_len;
> +
> +    /*
> +     * Set bits should only in the range of [0, cbm_len].
> +     * And, at least one bit need to be set.
> +     */
> +    if ( cbm & (~0ul << cbm_len) || cbm == 0 )
> +        return false;
> +
> +    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 ( zero_bit < cbm_len &&
> +         find_next_bit(&cbm, cbm_len, zero_bit) < cbm_len )
> +        return false;
> +
> +    return true;
> +}

Why do you need to move the code apart from renaming it?

> @@ -1210,25 +1237,39 @@ static unsigned int get_socket_cpu(unsigned int 
> socket)
>  struct cos_write_info
>  {
>      unsigned int cos;
> -    struct feat_node *feature;
> +    struct feat_node **features;
>      const uint32_t *val;
> -    const struct feat_props *props;
> +    unsigned int array_len;
> +    const struct feat_props **props;

Why do you need props here, from the usage below it's just pointing
to feat_props, which is already available in this context.

>  };
>  
>  static void do_write_psr_msrs(void *data)
>  {
>      const struct cos_write_info *info = data;
> -    struct feat_node *feat = info->feature;
> -    const struct feat_props *props = info->props;
> -    unsigned int i, cos = info->cos, cos_num = props->cos_num;
> +    unsigned int i, index = 0, cos = info->cos;
> +    const uint32_t *val_array = info->val;
>  
> -    for ( i = 0; i < cos_num; i++ )
> +    for ( i = 0; i < ARRAY_SIZE(feat_props); i++ )
>      {
> -        if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] )
> +        struct feat_node *feat = info->features[i];
> +        const struct feat_props *props = info->props[i];

If you use ARRAY_SIZE(feat_props), the above should be feat_props[i].
Also I'm worried about the size of the props array, isn't there a
possibility that the props array is smaller than the feature array?


Thanks, Roger.

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