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-12 02:40:41, Jan Beulich wrote:
> >>> On 12.01.17 at 02:22, <yi.y.sun@linux.intel.com> wrote:
> > On 17-01-11 07:01:23, Jan Beulich wrote:
> >> >>> On 11.01.17 at 07:22, <yi.y.sun@linux.intel.com> wrote:
> >> > 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?
> >> 
> >> Well, my general reservation here is that this way you require about
> >> half a dozen functions to all return the same value. If the value
> >> changes (or if somebody clones the set), there's the risk of one not
> >> getting properly updated. Therefore I'd much prefer for just one
> >> function to return the count. And I'm relatively certain that with the
> >> type checks moved out, this will actually end up being the more
> >> natural way.
> >> 
> > I imagine the way as your suggestion. It might be below flow for this 
> > write_msr.
> > 
> > list_for_each_entry(feat...) {
> >     feat->write_msr(..., val_array);
> >     val_array += feat->get_cos_num();
> >     ......
> > }
> > 
> > Is that what you think? Thanks!
> 
> Yes, something along these lines.
> 
Got it, thanks!

> Jan

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