Re: [Xen-devel] [PATCH v4 09/24] x86: refactor psr: set value: assemble features value array.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.01.17 at 07:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-01-10 07:34:00, Jan Beulich wrote:
>> >>> On 14.12.16 at 05:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > +static int l3_cat_set_new_val(uint64_t val[],
>> > +                              const struct feat_node *feat,
>> > +                              unsigned int old_cos,
>> > +                              enum cbm_type type,
>> > +                              uint64_t m)
>> > +{
>> > +    if ( type != PSR_CBM_TYPE_L3 )
>> > +        /* L3 CAT uses one COS. Skip it. */
>> > +        return 1;
>> 
>> If this is the wrong type, how can you return 1 (or any positive
>> value) here? This basically suggests that, as recommended for an
>> earlier operation already, that you want the type check done in
>> the caller. Or wait - isn't type not matching an error here, as you
>> iterate the same list twice in the same order? Which then gets us
>> back to me mentioning that the set-new-value should really be
>> done in common code, not in the callbacks; it would really only be
>> the check (psr_check_cbm() in the L3 case above) which would
>> require a callback.
>> 
> Your understanding is right. The value array will be iterated twice. First,
> assemble all features old value into it. Second, set the target value into
> array according to the target feature's position in the array. Because
> different features may have different behaviors, e.g. CDP uses two entries
> of the array, I think it would be better to make 'set_new_val' be a callback
> function.

Bot for common code to do that, all it takes is knowing the number
of registers, which you already have a callback for.

Jan


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