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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.01.17 at 02:22, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-01-11 07:01:23, Jan Beulich wrote:
>> >>> On 11.01.17 at 07:22, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-01-10 08:15:15, Jan Beulich wrote:
>> >> >>> On 14.12.16 at 05:07, <yi.y.sun@xxxxxxxxxxxxxxx> 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.

Jan


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