Re: [PATCH v11 12/23] x86: refactor psr: L3 CAT: set value: implement write msr flow.

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

On 17-05-30 09:35:53, Jan Beulich wrote:
> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote:
> > +struct cos_write_info
> > +{
> > +    unsigned int cos;
> > +    struct feat_node *feature;
> > +    uint32_t *val;
> > +    enum psr_feat_type feat_type;
> > +};
> > +
> > +static void do_write_psr_msrs(void *data)
> > +{
> > +    struct cos_write_info *info = data;
> > +    unsigned int cos = info->cos;
> > +    struct feat_node *feat = info->feature;
> > +    const struct feat_props *props = feat_props[info->feat_type];
> > +    unsigned int i;
> > +
> > +    for ( i = 0; i < props->cos_num; i++ )
> > +    {
> > +        if ( feat->cos_reg_val[cos * props->cos_num + i] != info->val[i] )
> > +        {
> > +            feat->cos_reg_val[cos * props->cos_num + i] = info->val[i];
> > +            props->write_msr(cos, info->val[i], props->type[i]);
> > +        }
> > +    }
> > +}
> 
> Again you're passing feat_type here only to get at props. Why
> not pass props right away? Also I think it would make sense to
> pull props->cos_num into a local variable.
> 
Have modified these according to your comments. Thanks!

> >  static int write_psr_msrs(unsigned int socket, unsigned int cos,
> >                            uint32_t val[], unsigned int array_len,
> >                            enum psr_feat_type feat_type)
> >  {
> > -    return -ENOENT;
> > +    unsigned int i;
> > +    struct psr_socket_info *info = get_socket_info(socket);
> > +    struct cos_write_info data =
> > +    {
> > +        .cos = cos,
> > +        .feature = info->features[feat_type],
> > +        .feat_type = feat_type,
> > +    };
> > +
> > +    if ( cos > info->features[feat_type]->cos_max )
> > +        return -EINVAL;
> > +
> > +    /* Skip to the feature's value head. */
> > +    for ( i = 0; i < feat_type; i++ )
> > +    {
> > +        if ( !info->features[i] )
> > +            continue;
> 
> This is inconsistent with checks done elsewhere, where you also
> check feat_props[feat_type] against NULL. I've made a comment
> regarding whether both checks are wanted in a uniform or non-
> uniform way pretty early in the series. Whatever is selected
> should then be used consistently.
> 
Have changed it. Thanks!

> Jan

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