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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.17 at 04:21, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-08 09:54:04, Jan Beulich wrote:
>> >>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > @@ -207,6 +233,29 @@ static enum psr_feat_type 
>> > psr_cbm_type_to_feat_type(enum cbm_type type)
>> >      return feat_type;
>> >  }
>> >  
>> > +static bool psr_check_cbm(unsigned int cbm_len, uint64_t cbm)
>> > +{
>> > +    unsigned int first_bit, zero_bit;
>> > +
>> > +    /* Set bits should only in the range of [0, cbm_len). */
>> > +    if ( cbm & (~0ull << cbm_len) )
>> 
>> This will not do what you intend for cbm_len == 64.
>> 
> cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3
> CAT, it may be 11 bits. For L2 CAT, it may be 8 bits.

And there is an _architectural_ guarantee for them to never
reach 64 bits?

>> > +static int l3_cat_get_old_val(uint64_t val[],
>> > +                              const struct feat_node *feat,
>> > +                              unsigned int old_cos)
>> > +{
>> > +    if ( old_cos > feat->info.l3_cat_info.cos_max )
>> 
>> Afaics this condition is the only L3 CAT specific thing in this function.
>> Should more of it be moved into common code? Same below for
>> set_new_val.
>> 
> Sorry, I may not understand your intention. For different features, they have
> different cos_max. Do you mean I should abstract a callback function for all
> features to handle this cos_max check? Thanks!

Yes. All features have a cos_max (even if the precise values may
differ), so common code can do the check if the values is stored
suitably in a field outside of any feature dependent data structures.

>> > -static int assemble_val_array(uint64_t *val,
>> > +static int combine_val_array(uint64_t *val,
>> 
>> Same comment as earlier on - please decide for a final name right
>> when introducing a function. In fact I'd prefer it to remain
>> "assemble".
>> 
> This is corrected in next version. I will change name back to assemble if you
> like it.

I'm fine with it. Personally I'd like "gather" even better, but the
naming choice in the end is up to you as long as it reflects the
purpose of the function (which imo "combine" doesn't).

Jan


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