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....@linux.intel.com> 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@lists.xen.org
https://lists.xen.org/xen-devel
