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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.09.16 at 10:09, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 16-09-08 03:54:24, Jan Beulich wrote:
>> >>> 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"?
>> 
> Sorry, I may misunderstand you. Maybe "check_exceed_cos_max" is a good name?

According to my knowledge of English this would still need to be
"check_exceeds_cos_max", and then the check ends up being quite
redundant.

>> >> >> > +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.
>> 
> Sorry to make you feel bad. I will avoid to use memcpy() in the codes.
> 
> I think I have got your mean. I will construct a union for feat_info in next
> version. Different features can use different members in union. Then, I can
> directly assign the struct value without memcpy. Thanks for the guidance!

You shouldn't need to do such assignments in the vast majority of
places you do the memcpy()ing in right now. (And don't forget,
structure assignment in the end is only a type safe variant of
memcpy().)

>> >> >> > +    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
> 
> Sure, let me introduce it from beginning.
> 
> First time, user wants to set L3 CAT of Dom1 to 0x1ff for example. The 
> old_cos
> of Dom1 is 0. L3 CAT is the first element of feature list. The COS registers
> values are below at this time.
> 
>         -------------------------------
>         | COS 0 | COS 1 | COS 2 | ... |
>         -------------------------------
> L3 CAT  | 0x7ff | ...   | ...   | ... |
>         -------------------------------
> L2 CAT  | 0xff  | ...   | ...   | ... |
>         -------------------------------
> 
> The mask array assembled in get_old_set_new() is:
> mask[0]: 0x1ff
> mask[1]: 0xff
> 
> It cannot find a matching COS so allocate COS 1 to store the value set.
> The COS registers values are changed to below now.
> 
>         -------------------------------
>         | COS 0 | COS 1 | COS 2 | ... |
>         -------------------------------
> L3 CAT  | 0x7ff | 0x1ff | ...   | ... |
>         -------------------------------
> L2 CAT  | 0xff  | 0xff  | ...   | ... |
>         -------------------------------
> 
> Then, user wants to set L3 CAT of Dom2 to 0x1ff too. The old_cos of Dom2
> is 0 too.
> 
> The mask array assembled in get_old_set_new() is:
> mask[0]: 0x1ff
> mask[1]: 0xff
> 
> So, it can find a matching COS, COS 1. Then, it can reuse COS 1 for Dom2.

So the lookup is then to find an entry with all current values except
for the one intended to be changed. If that's a correct understanding
of mine, then I think the primitives should be more primitive, making
the overall operation
1) get all current values into an array,
2) update the one array slot that's meant to get changed,
3) look for matching entries,
4) allocate new entry if none found.
And with that I'm then not even sure how many of these steps
actually need hooks, as most if not all of this looks pretty
independent of the aspects of the specific CAT variant. Hooks
should be introduced solely for cases where behavior necessarily
differs between variants.

Jan

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