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-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!

> Jan

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