Re: [Xen-devel] [PATCH 1/3] x86: refactor psr implementation in hypervisor.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.09.16 at 09:28, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 16-09-07 03:01:34, Jan Beulich wrote:
>> >> >>> On 25.08.16 at 07:22, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> >> > +    unsigned int (*exceed_range)(uint64_t *mask, struct feat_list 
>> >> > *pFeat,
>> >> > +                                 unsigned int cos);
>> >> 
>> >> According to the comment this is kind of a predicate, which seems
>> >> unlikely to return an unsigned value. In fact without a word on the
>> >> return value I'd expect such to return bool. And I'd also expect the
>> >> name to reflect the purpose, i.e. "exceeds_name()". Plus just like
>> >> for compare above I wonder whether come or all of the parameters
>> >> should be pointers to const (please go over the entire patch and do
>> >> constification wherever possible/sensible).
>> >> 
>> > Yes, you are right. I will change the function type to bool and add const
>> > for not changed input pointers.
>> > 
>> > This function is used to check if the input cos id exceeds the cos_max. If 
>> > yes
>> > and the set value is not default value, we should return error. So, I think
>> > to change the function name to exceed_cos_max(). How do you think?
>> 
>> Okay, except that I continue to think you mean "exceeds".
>> "exceed_cos_max" to me is kind of a directive, not a predicate.
>> 
> How about "beyond"?

What's wrong with "exceeds"?

>> >> > +static int l3_cat_compare_mask(uint64_t *mask, struct feat_list *pFeat,
>> >> > +                               unsigned int cos, bool_t *found)
>> >> > +{
>> >> > +    struct psr_cat_lvl_info cat_info;
>> >> > +    uint64_t l3_def_cbm;
>> >> > +
>> >> > +    memcpy(&cat_info, pFeat->feat_info, sizeof(struct 
>> >> > psr_cat_lvl_info));
>> >> 
>> >> Already here I think this memcpy()ing gets unwieldy. Can't you
>> >> simply make the structure field a union of all types of interest?
>> >> 
>> > Sorry that I am not very clear about your meaning to make a union. Do you 
>> > mean
>> > make feat_info a union? If so, it will lose the universality to cover all
>> > features. Future feature may have different info.
>> 
>> Which is the purpose of a union - you'd simply add a new member
>> then.
>>
> I guess your idea likes below. Right?
> union {
>     struct l3_info {
>         union {
>             uint64_t cbm;
>             struct {
>                 uint64_t code;
>                 uint64_t data;
>             };
>         };
> 
>     };
> 
>     struct l2_info {
>         uint64_t cbm;
>     };
> };
>  
> My original design is to use this feat_info cover all features and eliminate
> the feature's specific properties. If using above union, we still need to
> know the current feature is which when handles feat_info. That loses the
> abstraction.
> 
> If my thought is not right, please correct me. Thanks!

I don't understand what abstraction you would lose with the above
layout. The memcpy()int you currently do is, I'm sorry to say that,
horrible.

>> > I think I can replace the memcpy() to directly assign value to cat_info.
>> 
>> No, this copying (done in _many_ places) really should go away.
>> 
> I want to replace memcpy() to below codes.
> cat_info.cbm_len = feat_info[0];
> cat_info.cos_max = feat_info[1];

And again do that in a dozen places? No, please don't.

>> >> > +    if ( type == PSR_MASK_TYPE_L3_CBM )
>> >> > +        mask[0] = m;
>> >> 
>> >> This overwriting behavior also looks quite strange to me. What's
>> >> the purpose? And if this really is meant to be that way, why is
>> >> this not (leaving aside the other suggested adjustment)
>> >> 
>> >>     if ( type == PSR_MASK_TYPE_L3_CBM )
>> >>         mask[0] = m;
>> >>     else if ( old_cos > cat_info.cos_max )
>> >>         mask[0] = pFeat->cos_reg_val[0];
>> >>     else
>> >>         mask[0] = pFeat->cos_reg_val[old_cos];
>> >> 
>> >> ?
>> >> 
>> > get_old_set_new() is used to do below two things:
>> > 1. get old_cos register value of all supported features and
>> > 2. set the new value for appointed feature.
>> > 
>> > So, if the appointed feature is L3 CAT, we should set input vallue for it 
>> > here.
>> 
>> Okay, that answers the "what" aspect, but leaves open _why_ it
>> needs to be that way.
>> 
> A scenario here to help to understand _why_. 
> 
> Like the example for explaining get_old_set_new(), old_cos of the domain is 
> 1.
> Then, User wants to set L3 CAT CBM to 0x1ff and L2 CAT 0x3f. The original
> COS registers like below.
> 
>         -------------------------------
>         | COS 0 | COS 1 | COS 2 | ... |
>         -------------------------------
> L3 CAT  | 0x7ff | 0x3ff | 0x1ff | ... |
>         -------------------------------
> L2 CAT  | 0xff  | 0x3f  | 0x3f  | ... |
>         -------------------------------
> 
> Then, mask array should be assembled in get_old_set_new() like below:
> mask[0]: 0x1ff
> mask[1]: 0x3f
> 
> Then, we can use this mask array to find if there is matching COS through
> compare_mask(). We can find COS 2 is the matching one. 
> 
> If there is already a COS registers combination (e.g. L3 COS 2 and L2 COS 2)
> same as the mask array, we can reuse this combination directly but not to
> allocate a new COS ID. By this way, we can save the COS registers. You know,
> there is limitation on COS registers number.
> 
> Of course, if we cannot find it in existing combinations, we will call
> alloc_new_cos() to allocate a new COS to use.

I think I begin to see the purpose, but then again I still can't get
this explanation in line with there just being a single new value to
be set (m). Perhaps it would help if you split the lookup from the
setting or a new value.

Jan

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