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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

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;
......
}

> > +        /* Find */
> 
> Found (also below)
> 
> > +        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.

> > @@ -752,7 +793,61 @@ static int find_cos(const uint32_t val[], uint32_t 
> > array_len,
> >                      enum psr_feat_type feat_type,
> >                      const struct psr_socket_info *info)
> >  {
> > +    unsigned int cos, i;
> > +    const unsigned int *ref = info->cos_ref;
> > +    const struct feat_node *feat;
> > +    const uint32_t *val_array = val;
> 
> The name doesn't match the purpose - as you increment the pointer,
> its name should rather be "val_ptr" or some such.
> 
Got it, thanks!

> > +    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.

> > +    unsigned int cos_max;
> > +
> >      ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
> > +
> > +    /* cos_max is the one of the feature which is being set. */
> > +    feat = info->features[feat_type];
> > +    if ( !feat )
> > +        return -ENOENT;
> > +
> > +    cos_max = feat->ops.get_cos_max(feat);
> > +
> > +    for ( cos = 0; cos <= cos_max; cos++ )
> > +    {
> > +        if ( cos && !ref[cos] )
> > +            continue;
> > +
> > +        /*
> > +         * If fail to find cos in below loop, need find whole feature array
> > +         * again from beginning.
> > +         */
> > +        val_array = val;
> 
> You wouldn't need to re-do this here if you moved the variable
> declaration (with initializer) into this scope. This then also
> eliminates the need for the comment, which otherwise would
> need its wording corrected.
> 
Ok, thanks!

> > +        for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> > +        {
> > +            if ( !info->features[i] )
> > +                continue;
> > +
> > +            feat = info->features[i];
> 
> Please swap if() and assignment, utilizing the local variable in the
> if().
> 
Ok, thanks!

> Jan

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