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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-09-26 10:39:31, Roger Pau Monn� wrote:
> 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?
> 
Because it is CAT specific function now. I moved it into CAT section.
    '/* Implementation of allocation features' functions. */'

> > @@ -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.
> 
I may drop it.

> >  };
> >  
> >  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].

Ok.

> 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?
> 
No, every member is inserted into props array if the feature is initialized
successfully and inserted into feature array. So, they are 1:1.

> 
> Thanks, Roger.

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