Re: [PATCH v4 12/24] x86: refactor psr: set value: implement write msr flow.

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

On 17-01-10 08:15:15, Jan Beulich wrote:
> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -186,6 +186,9 @@ struct feat_ops {
> >      unsigned int (*exceeds_cos_max)(const uint64_t val[],
> >                                      const struct feat_node *feat,
> >                                      unsigned int cos);
> > +    /* write_msr is used to write out feature MSR register. */
> > +    int (*write_msr)(unsigned int cos, const uint64_t val[],
> > +                     struct feat_node *feat);
> 
> Looks like this function again returns number-of-values, yet this time
> without a comment saying so. While you don't need to replicate
> that description multiple time, please at least has a brief reference.
> That said, with the type checks moved out I think this return value
> model won't be needed anymore - the caller, having checked the
> type, could then simply call the get-num-val (or however it was
> named) hook to know how many array entries to skip.
> 
For write msr, we may need iterate the whole feature list to write values for
every feature if the input value is not same as old on the COS ID. So, I prefer
to keep current return value, the number-of-values handled. That would be clear
and easy to implement. Of course, we can call get_cos_num to get the returen
value or define a macro to replace the digit. How do you think?

> > @@ -889,9 +909,67 @@ static int alloc_new_cos(const struct psr_socket_info *info,
> >      return -ENOENT;
> >  }
> >  
> > +static unsigned int get_socket_cpu(unsigned int socket)
> > +{
> > +    if ( likely(socket < nr_sockets) )
> > +        return cpumask_any(socket_cpumask[socket]);
> > +
> > +    return nr_cpu_ids;
> > +}
> > +
> > +struct cos_write_info
> > +{
> > +    unsigned int cos;
> > +    struct list_head *feat_list;
> > +    const uint64_t *val;
> > +};
> > +
> > +static void do_write_psr_msr(void *data)
> > +{
> > +    struct cos_write_info *info = (struct cos_write_info *)data;
> > +    unsigned int cos           = info->cos;
> > +    struct list_head *feat_list= info->feat_list;
> > +    const uint64_t *val        = info->val;
> > +    struct feat_node *feat_tmp;
> > +    int ret;
> > +
> > +    if ( !feat_list )
> > +        return;
> > +
> > +    /* We need set all features values into MSRs. */
> > +    list_for_each_entry(feat_tmp, feat_list, list)
> > +    {
> > +        ret = feat_tmp->ops.write_msr(cos, val, feat_tmp);
> > +        if ( ret <= 0)
> 
> Missing blank.
> 
Sorry.

> > +            return;
> > +
> > +        val += ret;
> > +    }
> > +}
> > +
> >  static int write_psr_msr(unsigned int socket, unsigned int cos,
> >                           const uint64_t *val)
> >  {
> > +    struct psr_socket_info *info = get_socket_info(socket);
> > +
> > +    struct cos_write_info data =
> 
> No blank lines between declarations please (unless there are
> extraordinarily many).
> 
Ok, thanks!

> Jan

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