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

[Yi Sun]

On 16-09-12 01:30:22, Jan Beulich wrote:
> >>> On 12.09.16 at 05:26,  wrote:
> > On 16-09-09 02:32:25, Jan Beulich wrote:
> >> >>> On 09.09.16 at 10:09,  wrote:
> >> > 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
> > By analyzing the behaviors and requirements of different CAT features, I
> > divide COS find/allocate process to three steps.
> > 1) get_old_set_new(): get all current values into an array and the value of
> > the changed feature is replaced by the input value(m). Here, you get an 
> > array
> > which contains all features old value and the new value for changed feature.
> > 2) find_cos(): find if there is a matching COS combination which can match
> > the input array.
> > 3) alloc_new_cos(): if find_cos() cannot find a matching one, allocate a new
> > COS id or reuse the one which is only occupied by current domain.
> > 
> > So, our ideas are almost same. Only one exception to split get_old_set_new()
> > function into two parts. That increases one loop to traverse all features 
> > and
> > most features "set new value" callback functions will return directly 
> > because
> > the type cannot match. And, I think to put value array assembling work into 
> > one
> > function is reasonable. What do you think? Thanks!
> 
> How many iterations do you expect these loops to have? If it's only
> a few, I think the simpler and more clear abstraction would outweigh
> the performance benefits, the more that I hope none of this ends up
> sitting on a performance critical path in the first place.
> 
> Jan

So far, only two features and it would not be so many in the future. :)

Will add more callback function to set new value. Thanks!


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

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

[Jan Beulich]

>>> On 12.09.16 at 05:26,  wrote:
> On 16-09-09 02:32:25, Jan Beulich wrote:
>> >>> On 09.09.16 at 10:09,  wrote:
>> > 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
> By analyzing the behaviors and requirements of different CAT features, I
> divide COS find/allocate process to three steps.
> 1) get_old_set_new(): get all current values into an array and the value of
> the changed feature is replaced by the input value(m). Here, you get an 
> array
> which contains all features old value and the new value for changed feature.
> 2) find_cos(): find if there is a matching COS combination which can match
> the input array.
> 3) alloc_new_cos(): if find_cos() cannot find a matching one, allocate a new
> COS id or reuse the one which is only occupied by current domain.
> 
> So, our ideas are almost same. Only one exception to split get_old_set_new()
> function into two parts. That increases one loop to traverse all features and
> most features "set new value" callback functions will return directly because
> the type cannot match. And, I think to put value array assembling work into 
> one
> function is reasonable. What do you think? Thanks!

How many iterations do you expect these loops to have? If it's only
a few, I think the simpler and more clear abstraction would outweigh
the performance benefits, the more that I hope none of this ends up
sitting on a performance critical path in the first place.

Jan


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

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

[Yi Sun]

On 16-09-09 11:14:50, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 1/3] x86: refactor psr implementation in 
> hypervisor."):
> > On 09.09.16 at 10:09,  wrote:
> > > 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.
> 
> In the hope of helping to end this subthread: I'm a native speaker of
> English and Jan is right.
> 
> Ian.
Thank you!

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

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

[Yi Sun]

On 16-09-09 02:32:25, Jan Beulich wrote:
> >>> On 09.09.16 at 10:09,  wrote:
> > On 16-09-08 03:54:24, Jan Beulich wrote:
> >> >>> On 08.09.16 at 09:28,  wrote:
> >> > On 16-09-07 03:01:34, Jan Beulich wrote:
> >> >> >> >>> On 25.08.16 at 07:22,  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.
> 
Ok, got it. Thanks!

> >> >> >> > +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(_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().)
> 
Got it, thanks!

> >> >> >> > +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];
> >> >> >> 
> >> >> >> ?
> >> >> >> 
> >> 

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

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/3] x86: refactor psr implementation in 
hypervisor."):
> On 09.09.16 at 10:09,  wrote:
> > 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.

In the hope of helping to end this subthread: I'm a native speaker of
English and Jan is right.

Ian.

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

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

[Jan Beulich]

>>> On 09.09.16 at 10:09,  wrote:
> On 16-09-08 03:54:24, Jan Beulich wrote:
>> >>> On 08.09.16 at 09:28,  wrote:
>> > On 16-09-07 03:01:34, Jan Beulich wrote:
>> >> >> >>> On 25.08.16 at 07:22,  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(_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 

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

[Yi Sun]

On 16-09-08 03:54:24, Jan Beulich wrote:
> >>> On 08.09.16 at 09:28,  wrote:
> > On 16-09-07 03:01:34, Jan Beulich wrote:
> >> >> >>> On 25.08.16 at 07:22,  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(_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(), 

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

[Jan Beulich]

>>> On 08.09.16 at 09:28,  wrote:
> On 16-09-07 03:01:34, Jan Beulich wrote:
>> >> >>> On 25.08.16 at 07:22,  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(_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 

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

[Yi Sun]

On 16-09-07 03:01:34, Jan Beulich wrote:
> >> >>> On 25.08.16 at 07:22,  wrote:
> >> > + struct psr_socket_alloc_info *info);
> >> > +/*
> >> > + * get_old_set_new is used in set value process to get all features'
> >> > + * COS registers values according to original cos id of the domain.
> >> > + * Then, assemble them into an mask array as feature list order.
> >> 
> >> This sentence in particular doesn't make any sense to me. What
> >> follows below also looks like it is in need of improvement.
> >> 
> > Do you mean the comments are not accurate?
> 
> I simply wasn't able to tell, because of not being able to interpret
> the sentence.
> 
> > How about below description?
> >  
> > get_old_set_new will traverse all features in list. It 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.
> > 
> > All the values are set into mask array according the traversal order, 
> > meaning
> > the same order of feature list members.
> 
> Sounds reasonable. I suppose the example that you gave right
> next wasn't meant to go into the comment.
> 
Great, will replace the comments with the new sentences. The example will not
go in.

> >> > +/*
> >> > + * exceed_range is used to check if the input cos id exceeds the
> >> > + * feature's cos_max and if the input mask value is not the default 
> >> > one.
> >> > + * Even if the associated cos exceeds the cos_max, HW can work as 
> >> > default
> >> > + * value. That is the reason we need check is mask value is default 
> >> > one.
> >> > + * If both criteria are fulfilled, that means the input exceeds the
> >> > + * range.
> >> > + */
> >> > +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"?

> >> > +#define MAX_FEAT_INFO_SIZE 8
> >> > +#define MAX_COS_REG_NUM  128
> >> 
> >> Are these numbers arbitrary, or based on something?
> >> 
> > MAX_FEAT_INFO_SIZE is got from the sizeof(struct psr_cat_lvl_info) and
> > consider the extension for future feature.
> 
> In that case please use that sizeof() in the expression here.
> 
Sure. Thanks!

> > MAX_COS_REG_NUM is got from spec that the max COS registers number is 128
> > for all PSR features so far.
> 
> "So far" makes me still wonder: Is this an architectural limit or one
> resulting from current (hardware) implementations. In the former
> case I don't think a comment is strictly needed, but in the latter
> case the choice should be explained.
> 
It is the latter case. I will add comment to explain it. Thanks!

> >> > +struct psr_socket_alloc_info {
> >> 
> >> I've yet to see whether the "alloc" in the name really makes sense.
> 
> And btw., having seen the entire patch I don't think this alloc_ infix
> is warranted both here and in the variable name.
> 
Ok, will consider to remove it in codes. Thanks!

> >> > +/* Common functions for supporting feature callback functions. */
> >> > +static void add_feature(struct feat_list *pHead, struct feat_list *pTmp)
> >> > +{
> >> > +if ( NULL == pHead || NULL == pTmp )
> >> > +return;
> >> > +
> >> > +while ( pHead->pNext )
> >> > +pHead = pHead->pNext;
> >> > +
> >> > +pHead->pNext = pTmp;
> >> > +}
> >> 
> >> Do you really need custom list management here?
> >> 
> > It seems xen list interfaces require the input list be a double linked list 
> > but
> > my list is a single linked list. Furthermore, I only need simple add to tail
> > function and free function. So I create custom list management functions.
> 
> Unless there's a strong need, I'd like you to go with what is there,
> or introduce _generic_ singly linked list management.
> 
I will check below list management interfaces to see if I can reuse them.
Thanks!
xen/include/xen/list.h

> >> > +static void free_feature(struct 

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

[Jan Beulich]

>> >>> On 25.08.16 at 07:22,  wrote:
>> > + struct psr_socket_alloc_info *info);
>> > +/*
>> > + * get_old_set_new is used in set value process to get all features'
>> > + * COS registers values according to original cos id of the domain.
>> > + * Then, assemble them into an mask array as feature list order.
>> 
>> This sentence in particular doesn't make any sense to me. What
>> follows below also looks like it is in need of improvement.
>> 
> Do you mean the comments are not accurate?

I simply wasn't able to tell, because of not being able to interpret
the sentence.

> How about below description?
>  
> get_old_set_new will traverse all features in list. It 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.
> 
> All the values are set into mask array according the traversal order, meaning
> the same order of feature list members.

Sounds reasonable. I suppose the example that you gave right
next wasn't meant to go into the comment.

>> > +/*
>> > + * exceed_range is used to check if the input cos id exceeds the
>> > + * feature's cos_max and if the input mask value is not the default 
>> > one.
>> > + * Even if the associated cos exceeds the cos_max, HW can work as 
>> > default
>> > + * value. That is the reason we need check is mask value is default 
>> > one.
>> > + * If both criteria are fulfilled, that means the input exceeds the
>> > + * range.
>> > + */
>> > +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.

>> > +#define MAX_FEAT_INFO_SIZE 8
>> > +#define MAX_COS_REG_NUM  128
>> 
>> Are these numbers arbitrary, or based on something?
>> 
> MAX_FEAT_INFO_SIZE is got from the sizeof(struct psr_cat_lvl_info) and
> consider the extension for future feature.

In that case please use that sizeof() in the expression here.

> MAX_COS_REG_NUM is got from spec that the max COS registers number is 128
> for all PSR features so far.

"So far" makes me still wonder: Is this an architectural limit or one
resulting from current (hardware) implementations. In the former
case I don't think a comment is strictly needed, but in the latter
case the choice should be explained.

>> > +struct psr_socket_alloc_info {
>> 
>> I've yet to see whether the "alloc" in the name really makes sense.

And btw., having seen the entire patch I don't think this alloc_ infix
is warranted both here and in the variable name.

>> > +/* Common functions for supporting feature callback functions. */
>> > +static void add_feature(struct feat_list *pHead, struct feat_list *pTmp)
>> > +{
>> > +if ( NULL == pHead || NULL == pTmp )
>> > +return;
>> > +
>> > +while ( pHead->pNext )
>> > +pHead = pHead->pNext;
>> > +
>> > +pHead->pNext = pTmp;
>> > +}
>> 
>> Do you really need custom list management here?
>> 
> It seems xen list interfaces require the input list be a double linked list 
> but
> my list is a single linked list. Furthermore, I only need simple add to tail
> function and free function. So I create custom list management functions.

Unless there's a strong need, I'd like you to go with what is there,
or introduce _generic_ singly linked list management.

>> > +static void free_feature(struct psr_socket_alloc_info *info)
>> > +{
>> > +struct feat_list *pTmp;
>> > +struct feat_list *pPrev;
>> > +
>> > +if ( NULL == info )
>> > +return;
>> > +
>> > +if ( NULL == info->pFeat )
>> > +return;
>> > +
>> > +pTmp = info->pFeat->pNext;
>> > +while ( pTmp )
>> > +{
>> > +pPrev = pTmp;
>> > +pTmp = pTmp->pNext;
>> > +clear_bit(pPrev->feature, &(info->features));
>> > +xfree(pPrev);
>> > +pPrev = NULL;
>> > +}
>> > +}
>> 
>> Why does info->pFeat not get freed here?
>> 
> The info->pFeat is a redundant list head to facilitate list operations.

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

[Yi Sun]

Hi, Jan,

Thank you very much for reviewing my patches in details! Please
check my comments below.

On 16-09-06 01:40:00, Jan Beulich wrote:
> >>> On 25.08.16 at 07:22,  wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -23,22 +23,116 @@
> >  #define PSR_CAT(1<<1)
> >  #define PSR_CDP(1<<2)
> >  
> > -struct psr_cat_cbm {
> > -union {
> > -uint64_t cbm;
> > -struct {
> > -uint64_t code;
> > -uint64_t data;
> > -};
> > -};
> > +/*
> > + * To add a new PSR feature, please follow below steps:
> > + * 1. Implement feature specific callback functions listed in
> > + *"struct feat_ops";
> > + * 2. Implement a feature specific "struct feat_ops" object and register
> > + *feature specific callback functions into it;
> > + * 3. Delcare feat_list object for the feature and malloc memory for it
> > + *in internal_cpu_prepare(). Correspondingly, free them in
> > + *free_feature();
> > + * 4. Add feature initialization codes in internal_cpu_init().
> > + */
> > +
> > +struct feat_list;
> > +struct psr_socket_alloc_info;
> > +
> > +/* Every feature enabled should implement such ops and callback functions. 
> > */
> > +struct feat_ops {
> > +/*
> > + * init_feature is used in cpu initialization process to do feature
> > + * specific initialization works.
> > + */
> > +void (*init_feature)(unsigned int eax, unsigned int ebx,
> > + unsigned int ecx, unsigned int edx,
> > + struct feat_list *pFeat,
> 
> Here and below - we don't normally use identifiers with capital letters
> in the middle.
> 
Thanks! I will fix it.

> > + struct psr_socket_alloc_info *info);
> > +/*
> > + * get_old_set_new is used in set value process to get all features'
> > + * COS registers values according to original cos id of the domain.
> > + * Then, assemble them into an mask array as feature list order.
> 
> This sentence in particular doesn't make any sense to me. What
> follows below also looks like it is in need of improvement.
> 
Do you mean the comments are not accurate? How about below description?

get_old_set_new will traverse all features in list. It 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.

All the values are set into mask array according the traversal order, meaning
the same order of feature list members.

For example, old_cos of the domain is 1. User wants to set L3 CAT CBM to 0x1ff.
The original COS registers like below.

---
| COS 0 | COS 1 | ... |
---
L3 CAT  | 0x7ff | 0x3ff | ... |
---
L2 CAT  | 0xff  | 0x3f  | ... |
---

Then, mask array should be:
mask[0]: 0x1ff
mask[1]: 0x3f

Then, we can use this mask array to find if there is matching COS through
compare_mask().

> > + * Then, set the new feature value into feature corresponding position
> > + * in the array. This function is used to pair with compare_mask.
> > + */
> > +int (*get_old_set_new)(uint64_t *mask,
> > +   struct feat_list *pFeat,
> > +   unsigned int old_cos,
> > +   enum mask_type type,
> > +   uint64_t m);
> > +/*
> > + * compare_mask is used to in set value process to compare if the
> > + * input mask array can match all the features' COS registers values
> > + * according to input cos id.
> > + */
> > +int (*compare_mask)(uint64_t *mask, struct feat_list *pFeat,
> > +unsigned int cos, bool_t *found);
> 
> For a "compare" function I'd expect a word on the meaning of the
> return value, and I'd expect most if not all pointer parameters to
> be pointers to const.
>
Thanks! I will explain the return value.
 
> Also plain bool please.
> 
Thanks! I will fix it.

> > +/*
> > + * get_cos_max_as_type is used to get the cos_max value of the feature
> > + * according to input mask_type.
> > + */
> > +unsigned int (*get_cos_max_as_type)(struct feat_list *pFeat,
> > +enum mask_type type);
> 
> DYM mean "get_cos_max_from_type()"?
> 
Yes, correct. I will revise the function name.

> > +/*
> > + * exceed_range is used to check if the input cos id exceeds the
> > + * feature's cos_max and if the input mask value is not the default 
> > one.
> > + * Even if the associated cos exceeds the cos_max, HW can work as 
> > default
> > + * value. That is the reason we need check is mask value is default 
> > one.
> > + * If both criteria are fulfilled, that means the input exceeds the
> > + * range.
> > + */
> > +unsigned int (*exceed_range)(uint64_t *mask, struct feat_list *pFeat,
> 

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

[Jan Beulich]

>>> On 25.08.16 at 07:22,  wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -23,22 +23,116 @@
>  #define PSR_CAT(1<<1)
>  #define PSR_CDP(1<<2)
>  
> -struct psr_cat_cbm {
> -union {
> -uint64_t cbm;
> -struct {
> -uint64_t code;
> -uint64_t data;
> -};
> -};
> +/*
> + * To add a new PSR feature, please follow below steps:
> + * 1. Implement feature specific callback functions listed in
> + *"struct feat_ops";
> + * 2. Implement a feature specific "struct feat_ops" object and register
> + *feature specific callback functions into it;
> + * 3. Delcare feat_list object for the feature and malloc memory for it
> + *in internal_cpu_prepare(). Correspondingly, free them in
> + *free_feature();
> + * 4. Add feature initialization codes in internal_cpu_init().
> + */
> +
> +struct feat_list;
> +struct psr_socket_alloc_info;
> +
> +/* Every feature enabled should implement such ops and callback functions. */
> +struct feat_ops {
> +/*
> + * init_feature is used in cpu initialization process to do feature
> + * specific initialization works.
> + */
> +void (*init_feature)(unsigned int eax, unsigned int ebx,
> + unsigned int ecx, unsigned int edx,
> + struct feat_list *pFeat,

Here and below - we don't normally use identifiers with capital letters
in the middle.

> + struct psr_socket_alloc_info *info);
> +/*
> + * get_old_set_new is used in set value process to get all features'
> + * COS registers values according to original cos id of the domain.
> + * Then, assemble them into an mask array as feature list order.

This sentence in particular doesn't make any sense to me. What
follows below also looks like it is in need of improvement.

> + * Then, set the new feature value into feature corresponding position
> + * in the array. This function is used to pair with compare_mask.
> + */
> +int (*get_old_set_new)(uint64_t *mask,
> +   struct feat_list *pFeat,
> +   unsigned int old_cos,
> +   enum mask_type type,
> +   uint64_t m);
> +/*
> + * compare_mask is used to in set value process to compare if the
> + * input mask array can match all the features' COS registers values
> + * according to input cos id.
> + */
> +int (*compare_mask)(uint64_t *mask, struct feat_list *pFeat,
> +unsigned int cos, bool_t *found);

For a "compare" function I'd expect a word on the meaning of the
return value, and I'd expect most if not all pointer parameters to
be pointers to const.

Also plain bool please.

> +/*
> + * get_cos_max_as_type is used to get the cos_max value of the feature
> + * according to input mask_type.
> + */
> +unsigned int (*get_cos_max_as_type)(struct feat_list *pFeat,
> +enum mask_type type);

DYM mean "get_cos_max_from_type()"?

> +/*
> + * exceed_range is used to check if the input cos id exceeds the
> + * feature's cos_max and if the input mask value is not the default one.
> + * Even if the associated cos exceeds the cos_max, HW can work as default
> + * value. That is the reason we need check is mask value is default one.
> + * If both criteria are fulfilled, that means the input exceeds the
> + * range.
> + */
> +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).

> +/*
> + * write_msr is used to write feature specific MSR registers.
> + */

This is a single line comment.

> +int (*write_msr)(unsigned int cos, uint64_t *mask,
> + struct feat_list *pFeat);

Such write operations don't normally alter the value to be written.
If that's different here, that's perhaps worth a word in the comment.
If it isn't, then I don't see why the address of the value gets passed
instead of the value itself.

Oh, having gone further in the patch I see this is to update multiple
MSRs. I guess this would best be reflected by making the parameter
const uint64_t masks[] (albeit it's also not clear to me why this
necessarily have to be masks kind items, and not just arbitrary
values - proper choice of variable and parameter names helps the
understanding).

> +#define MAX_FEAT_INFO_SIZE 8
> 

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

[Yi Sun]

Current psr.c is designed for supporting L3 CAT/CDP. It has many
limitations to add new feature. Considering to support more PSR
features, we need refactor PSR implementation to make it more
flexible and fulfill the principle, open for extension but closed
for modification.

The core of the refactoring is to abstract the common actions and
encapsulate them into "struct feat_ops". The detailed steps to add
a new feature are described at the head of psr.c.

Signed-off-by: Yi Sun 
---
 xen/arch/x86/domctl.c |   36 +-
 xen/arch/x86/psr.c| 1121 +++--
 xen/arch/x86/sysctl.c |9 +-
 xen/include/asm-x86/psr.h |   21 +-
 4 files changed, 912 insertions(+), 275 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index bed70aa..a51ed2c 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1358,41 +1358,41 @@ long arch_do_domctl(
 switch ( domctl->u.psr_cat_op.cmd )
 {
 case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM:
-ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
- domctl->u.psr_cat_op.data,
- PSR_CBM_TYPE_L3);
+ret = psr_set_val(d, domctl->u.psr_cat_op.target,
+  domctl->u.psr_cat_op.data,
+  PSR_MASK_TYPE_L3_CBM);
 break;
 
 case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE:
-ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
- domctl->u.psr_cat_op.data,
- PSR_CBM_TYPE_L3_CODE);
+ret = psr_set_val(d, domctl->u.psr_cat_op.target,
+  domctl->u.psr_cat_op.data,
+  PSR_MASK_TYPE_L3_CODE);
 break;
 
 case XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA:
-ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
- domctl->u.psr_cat_op.data,
- PSR_CBM_TYPE_L3_DATA);
+ret = psr_set_val(d, domctl->u.psr_cat_op.target,
+  domctl->u.psr_cat_op.data,
+  PSR_MASK_TYPE_L3_DATA);
 break;
 
 case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
-ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
- >u.psr_cat_op.data,
- PSR_CBM_TYPE_L3);
+ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+  >u.psr_cat_op.data,
+  PSR_MASK_TYPE_L3_CBM);
 copyback = 1;
 break;
 
 case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE:
-ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
- >u.psr_cat_op.data,
- PSR_CBM_TYPE_L3_CODE);
+ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+  >u.psr_cat_op.data,
+  PSR_MASK_TYPE_L3_CODE);
 copyback = 1;
 break;
 
 case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA:
-ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
- >u.psr_cat_op.data,
- PSR_CBM_TYPE_L3_DATA);
+ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+  >u.psr_cat_op.data,
+  PSR_MASK_TYPE_L3_DATA);
 copyback = 1;
 break;
 
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 0b5073c..a77b55a 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -23,22 +23,116 @@
 #define PSR_CAT(1<<1)
 #define PSR_CDP(1<<2)
 
-struct psr_cat_cbm {
-union {
-uint64_t cbm;
-struct {
-uint64_t code;
-uint64_t data;
-};
-};
+/*
+ * To add a new PSR feature, please follow below steps:
+ * 1. Implement feature specific callback functions listed in
+ *"struct feat_ops";
+ * 2. Implement a feature specific "struct feat_ops" object and register
+ *feature specific callback functions into it;
+ * 3. Delcare feat_list object for the feature and malloc memory for it
+ *in internal_cpu_prepare(). Correspondingly, free them in
+ *free_feature();
+ * 4. Add feature initialization codes in internal_cpu_init().
+ */
+
+struct feat_list;
+struct psr_socket_alloc_info;
+
+/* Every feature enabled should implement such ops and callback functions. */
+struct feat_ops {
+/*
+ * init_feature is used in cpu initialization process to do feature
+ * specific initialization works.
+ */
+void (*init_feature)(unsigned int eax, unsigned int ebx,
+ unsigned int ecx, unsigned int edx,
+ struct feat_list *pFeat,
+ struct