Re: [PATCH v3 07/15] x86: implement set value flow for MBA

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-09-19 10:57:16, Roger Pau Monn� wrote:
> On Tue, Sep 05, 2017 at 05:32:29PM +0800, Yi Sun wrote:
[...]

> > +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]. */
> > +    if ( cbm & (~0ul << cbm_len) )
> > +        return false;
> > +
> > +    /* At least one bit need to be set. */
> > +    if ( cbm == 0 )
> > +        return false;
> 
> You can join both checks into a single if.
> 
Sure.

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

> >  static void do_write_psr_msrs(void *data)
> 
> Why does this function take a 'void *data' instead of 'const struct
> cos_write_info *info'?
> 
Because 'do_write_psr_msrs' is an parameter of 'on_selected_cpus' which is
declared below:

void on_selected_cpus(
    const cpumask_t *selected,
    void (*func) (void *info),
    void *info,
    int wait)

> >  {
> >      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, array_len = info->array_len, 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];
> > +        unsigned int cos_num, j;
> > +
> > +        if ( !feat || !props )
> > +            continue;
> > +
> > +        cos_num = props->cos_num;
> > +        if ( array_len < cos_num )
> 
> Not sure you need array_len, couldn't you use:
> 
> if ( index + cos_num >= info->array_len )
>     return;
> 
> ?
> 
Looks good. Thanks!

> Thanks, Roger.

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