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

[Yi Sun <yi.y.sun@linux.intel.com>]

On 16-09-08 03:54:24, Jan Beulich wrote:
> >>> On 08.09.16 at 09:28, <yi.y.sun@linux.intel.com> wrote:
> > On 16-09-07 03:01:34, Jan Beulich wrote:
> >> >> >>> On 25.08.16 at 07:22, <yi.y.sun@linux.intel.com> 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?

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

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

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

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