Re: [Xen-devel] [PATCH v9 11/25] x86: refactor psr: L3 CAT: set value: implement cos finding flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.03.17 at 05:26, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-27 04:28:23, Jan Beulich wrote:
>> >>> On 16.03.17 at 12:08, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/psr.c
>> > +++ b/xen/arch/x86/psr.c
> [...]
>  
>> > +static int cat_compare_val(const uint32_t val[],
>> > +                           const struct feat_node *feat,
>> > +                           unsigned int cos)
>> > +{
>> > +    /*
>> > +     * Different features' cos_max are different. If cos id of the feature
>> > +     * being set exceeds other feature's cos_max, the val of other feature
>> > +     * must be default value. HW supports such case.
>> > +     */
>> > +    if ( cos > feat->info.cat_info.cos_max )
>> > +    {
>> > +        /* cos_reg_val[0] is the default value. */
>> > +        if ( val[0] != feat->cos_reg_val[0] )
>> > +            return -EINVAL;
>> 
>> As you can see, with cos_max moved into the generic portion of the
>> feature node, this entire check can move into the caller.
>> 
> CDP has different behavior in this callback function. We need to check 
> val[0]
> and val[1] like below:
> static int l3_cdp_compare_val(...)
> {
>     if ( cos > feat->info.cat_info.cos_max )
>     {
>         if ( val[0] != get_cdp_data(feat, 0) ||
>              val[1] != get_cdp_code(feat, 0) )
>             return -EINVAL;
> 
>         /* Find */
>         return 1;
>     }
> 
>     if ( val[0] == get_cdp_data(feat, cos) &&
>          val[1] == get_cdp_code(feat, cos) )
>         /* Find */
>         return 1;
> ......
> }

There's no difference other than there being two values checked
here. Moving this to generic code should be easily possible as long
as the get_val() hook is flexible enough. Once again, please think
thoroughly about where to draw the line between generic code
and feature specific hooks - the latter should be reduced to a
minimum.

>> > +        return 1;
>> > +    }
>> > +
>> > +    if ( val[0] == feat->cos_reg_val[cos] )
>> > +        /* Find */
>> > +        return 1;
>> > +
>> > +    /* Not find */
>> > +    return 0;
>> > +}
>> 
>> Or actually, the entire function then becomes feature independent,
>> as it seems. And I think I did suggest that already during review of
>> an earlier version.
>> 
> Per above explanation, I think we have to keep this callback function.

Per above explanation, I think you don't need to keep this callback
function.

>> > +    int find = 0;
>> 
>> "found" again, or even simply "rc"? Also I think this would better
>> move into the outer for() scope.
>> 
> Ok, will use 'found' and move it.

Please check its use(s): It should be "found" only if that's the
meaning it always has. It's type being int (rather than bool)
suggests otherwise ...

Jan


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