Re: [Xen-devel] [PATCH v5] x86: psr: support co-exist features' values setting

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

Many thanks for the changes! The changes look good to me and pass the test.

On 17-10-11 06:06:49, Jan Beulich wrote:
> >>> On 11.10.17 at 09:20, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -1111,25 +1111,43 @@ static unsigned int get_socket_cpu(unsigned int 
> > socket)
> >  struct cos_write_info
> >  {
> >      unsigned int cos;
> > -    struct feat_node *feature;
> >      const uint32_t *val;
> > -    const struct feat_props *props;
> > +    unsigned int array_len;
> >  };
> 
> The addition wants to go into the hole after "cos".
> 
> >  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;
> > +    struct cos_write_info *info = data;
> 
> const
> 
> > +    unsigned int i, index = 0, cos = info->cos;
> > +    struct psr_socket_info *socket_info =
> 
> const
> 
> > +                            
> > get_socket_info(cpu_to_socket(smp_processor_id()));
> >  
> > -    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 = socket_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;
> > +        ASSERT(info->array_len >= index + cos_num);
> 
> While this transformation from the original -ENOSPC return looks to
> be correct, but not obviously so, it would have been a good idea
> to mention this in the commit message. After all the above can be
> correct only if the original -ENOSPC return path could have been
> an ASSERT() as well.
> 
> > +        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;
> 
> Looks like I only meant to comment on the uses of index above:
> If you incremented it alongside j, you could use just index in the
> respective array accesses, and you'd avoid the last statement
> above altogether.
> 
> In the interest of getting the patch in I'll see to make the
> adjustments myself. Please double check the result in case I end
> up committing what I've come up with.
> 
> Jan

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