Re: [PATCH v2] x86: psr: support co-exist features' values setting

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

On 17-10-09 15:03:25, Roger Pau Monn� wrote:
> On Sun, Oct 08, 2017 at 04:22:00AM +0000, Yi Sun wrote:
[...]

> >  static void do_write_psr_msrs(void *data)
> 
> Should this be "static int do_write_psr_msrs"...
>
This function is a parameter of 'on_selected_cpus()' which requires it to be
'void'.
 
> >  {
> >      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;
> >  
> > -    for ( i = 0; i < cos_num; i++ )
> > +    /*
> > +     * Iterate all featuers to write different value (not same as MSR) for
> > +     * each feature.
> > +     */
> > +    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 = feat_props[i];
> > +        unsigned int cos_num, j;
> > +
> > +        if ( !feat || !props )
> > +            continue;
> > +
> > +        cos_num = props->cos_num;
> > +        if ( info->array_len < index + cos_num )
> > +            return;
> 
> So that you can return -ENOSPC here (inline with what was previously
> done in write_psr_msrs)?
> 
> > +
> > +        for ( j = 0; j < cos_num; j++ )
> >          {
> > -            feat->cos_reg_val[cos * cos_num + i] = info->val[i];
> > -            props->write_msr(cos, info->val[i], props->type[i]);
> > +            if ( feat->cos_reg_val[cos * cos_num + j] != info->val[index + j] )
> > +            {
> > +                feat->cos_reg_val[cos * cos_num + j] = info->val[index + j];
> > +                props->write_msr(cos, info->val[index + j], props->type[j]);
> > +            }
> >          }
> > +
> > +        index += cos_num;
> >      }
> >  }
> >  
> > @@ -1137,30 +1155,18 @@ static int write_psr_msrs(unsigned int socket, unsigned int cos,
> >                            const uint32_t val[], unsigned int array_len,
> >                            enum psr_feat_type feat_type)
> >  {
> > -    int ret;
> >      struct psr_socket_info *info = get_socket_info(socket);
> >      struct cos_write_info data =
> >      {
> >          .cos = cos,
> > -        .feature = info->features[feat_type],
> > -        .props = feat_props[feat_type],
> > +        .features = info->features,
> > +        .val = val,
> > +        .array_len = array_len,
> >      };
> >  
> >      if ( cos > info->features[feat_type]->cos_max )
> >          return -EINVAL;
> >  
> > -    /* Skip to the feature's value head. */
> > -    ret = skip_prior_features(&array_len, feat_type);
> > -    if ( ret < 0 )
> > -        return ret;
> > -
> > -    val += ret;
> > -
> > -    if ( array_len < feat_props[feat_type]->cos_num )
> 
> When moved inside of do_write_psr_msrs this becomes:
> 
> info->array_len < index + cos_num
> 
> Where cos_num == feat_props[feat_type]->cos_num. Is this correct?
> 
> I'm asking because the check used to be array_len < cos_num.
> 
The removed old codes here only check one feature cos_num. Because the old
codes can only handle one feature. That is the reason I submit this patch
to support multiple co-exist features' values setting.

> Thanks, Roger.

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