Re: [Xen-devel] [PATCH v8 10/24] x86: refactor psr: set value: implement cos finding flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.17 at 06:35, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-08 10:03:10, Jan Beulich wrote:
>> >>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > @@ -356,6 +369,34 @@ static int l3_cat_set_new_val(uint64_t val[],
>> >      return 0;
>> >  }
>> >  
>> > +static int l3_cat_compare_val(const uint64_t val[],
>> > +                              const struct feat_node *feat,
>> > +                              unsigned int cos, bool *found)
>> > +{
>> > +    uint64_t l3_def_cbm;
>> > +
>> > +    l3_def_cbm = (1ull << feat->info.l3_cat_info.cbm_len) - 1;
>> 
>> Please only calculate the value on the path you need it. Also this
>> will again degenerate of cbm_len == 64.
>> 
> Sorry, what do you mean? I need get the default value of L3 CAT here
> to check if input val equals the default one if cos exceeds cos_max.

No, you don't need it at function scope. You only need it ...

>> > +    /*
>> > +     * 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.l3_cat_info.cos_max )
>> > +    {
>> > +        if ( val[0] != l3_def_cbm )

... in this more narrow one. Specifically no code outside the outer
if() needs the variable.

>> > +        {
>> > +            *found = false;
>> > +            return -ENOENT;
>> 
>> What is the difference between this "not found" and ...
>> 
> This is an error case. If input cos exceeds the cos_max and the input val is
> not default value, that means the input parameters exceed HW ability. We 
> should
> report error back.
> 
>> > +        }
>> > +        *found = true;
>> > +    }
>> > +    else
>> > +        *found = (val[0] == feat->cos_reg_val[cos]);
>> > +
>> > +    return 0;
>> 
>> ... the possible one here? I.e. why once -ENOENT and once 0 as
>> return value?
>> 
> 0 means success. '*found' means if the val is found or not.

I still don't understand the difference, but perhaps this will all sort
itself out once the cos_max checks get done in common code and
the default values (suitably stored, as suggested elsewhere) can
also be checked by common code. In fact with that I'm not even
sure you will continue to require this feature dependent actor
anymore.

> Per Roger's
> suggestion, I will refine this function to use return value to check
> if it is found or not.

Well, let's see how this ends up being.

Jan


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