[v10,03/25] x86: refactor psr: implement main data structures.

Commit Message

Yi Sun April 1, 2017, 1:53 p.m. UTC

To construct an extendible framework, we need analyze PSR features
and abstract the common things and feature specific things. Then,
encapsulate them into different data structures.

By analyzing PSR features, we can get below map.
                +------+------+------+
      --------->| Dom0 | Dom1 | ...  |
      |         +------+------+------+
      |            |
      |Dom ID      | cos_id of domain
      |            V
      |        +-----------------------------------------------------------------------------+
User --------->| PSR                                                                         |
     Socket ID |  +--------------+---------------+---------------+                           |
               |  | Socket0 Info | Socket 1 Info |    ...        |                           |
               |  +--------------+---------------+---------------+                           |
               |    |                   cos_id=0               cos_id=1          ...         |
               |    |          +-----------------------+-----------------------+-----------+ |
               |    |->Ref   : |         ref 0         |         ref 1         | ...       | |
               |    |          +-----------------------+-----------------------+-----------+ |
               |    |          +-----------------------+-----------------------+-----------+ |
               |    |->L3 CAT: |         cos 0         |         cos 1         | ...       | |
               |    |          +-----------------------+-----------------------+-----------+ |
               |    |          +-----------------------+-----------------------+-----------+ |
               |    |->L2 CAT: |         cos 0         |         cos 1         | ...       | |
               |    |          +-----------------------+-----------------------+-----------+ |
               |    |          +-----------+-----------+-----------+-----------+-----------+ |
               |    |->CDP   : | cos0 code | cos0 data | cos1 code | cos1 data | ...       | |
               |               +-----------+-----------+-----------+-----------+-----------+ |
               +-----------------------------------------------------------------------------+

So, we need define a socket info data structure, 'struct
psr_socket_info' to manage information per socket. It contains a
reference count array according to COS ID and a feature array to
manage all features enabled. Every entry of the reference count
array is used to record how many domains are using the COS registers
according to the COS ID. For example, L3 CAT and L2 CAT are enabled,
Dom1 uses COS_ID=1 registers of both features to save CBM values, like
below.
        +-------+-------+-------+-----+
        | COS 0 | COS 1 | COS 2 | ... |
        +-------+-------+-------+-----+
L3 CAT  | 0x7ff | 0x1ff | ...   | ... |
        +-------+-------+-------+-----+
L2 CAT  | 0xff  | 0xff  | ...   | ... |
        +-------+-------+-------+-----+

If Dom2 has same CBM values, it can reuse these registers which COS_ID=1.
That means, both Dom1 and Dom2 use same COS registers(ID=1) to keep same
L3/L2 values. So, the value of ref[1] is 2 which means 2 domains are using
COS_ID 1.

To manage a feature, we need define a feature node data structure,
'struct feat_node', to manage feature's specific HW info, its common
properties (callback functions - all feature's specific behaviors are
encapsulated into these callback functions, and generic values - e.g. the
cos_max), the feature independent values, and an array of all COS registers
values of this feature.

CDP is a special feature which uses two entries of the array
for one COS ID. So, the number of CDP COS registers is the half of L3
CAT. E.g. L3 CAT has 16 COS registers, then CDP has 8 COS registers if
it is enabled. CDP uses the COS registers array as below.

                         +-----------+-----------+-----------+-----------+-----------+
CDP cos_reg_val[] index: |     0     |     1     |     2     |     3     |    ...    |
                         +-----------+-----------+-----------+-----------+-----------+
                  value: | cos0 code | cos0 data | cos1 code | cos1 data |    ...    |
                         +-----------+-----------+-----------+-----------+-----------+

For more details, please refer SDM and patches to implement 'get value' and
'set value'.

Signed-off-by: Yi Sun <[e-mail]>
---
v10:
    - remove initialization for 'PSR_SOCKET_L3_CAT'.
      (suggested by Jan Beulich)
    - rename 'feat_ops' to 'feat_props'.
      (suggested by Jan Beulich)
    - move 'cbm_len' to 'feat_props' because it is feature independent so far.
      (suggested by Jan Beulich)
    - move 'cos_max' to 'feat_props' because it is feature independent.
      (suggested by Jan Beulich)
    - move 'cos_num' to 'feat_props' because it is feature independent.
      (suggested by Jan Beulich)
    - remove union 'info' and struct 'psr_cat_hw_info'.
    - remove 'get_cos_max' from 'feat_props'.
      (suggested by Jan Beulich)
    - remove 'feat_mask' from 'psr_socket_info' because we can use 'features[]'
      to check if any feature is initialized.
      (suggested by Jan Beulich)
    - move 'ref_lock' above 'cos_ref'.
      (suggested by Jan Beulich)
    - adjust comments and commit message according to above changes.
v9:
    - replace feature list to a feature pointer array.
      (suggested by Roger Pau)
    - add 'PSR_SOCKET_MAX_FEAT' in 'enum psr_feat_type' to know features
      account.
      (suggested by Roger Pau)
    - move 'feat_ops' declaration into 'feat_node' structure.
      (suggested by Roger Pau)
    - directly use uninon for feature HW info and move its declaration into
      'feat_node' structure.
      (suggested by Roger Pau)
    - remove 'enum psr_feat_type feature' declared in 'feat_ops' because it is
      not useful after using feature pointer array.
      (suggested by Roger Pau)
    - rename 'l3_cat_info' to 'cat_info' to be used by all CAT/CDP features.
    - remove 'nr_feat' which is only for a record.
      (suggested by Jan Beulich)
    - add 'cos_num' to record how many COS registers are used by a feature in
      one time access.
      (suggested by Jan Beulich)
    - replace 'uint64_t' to 'uint32_t' for cbm value because SDM specifies the
      max 32 bits for it.
      (suggested by Jan Beulich)
v7:
    - sort inclusion files position.
      (suggested by Wei Liu)
v6:
    - make commit message be clearer.
      (suggested by Konrad Rzeszutek Wilk)
    - fix wordings.
      (suggested by Konrad Rzeszutek Wilk)
    - add comments to explain relationship between 'feat_mask' and
      'enum psr_feat_type'.
      (suggested by Konrad Rzeszutek Wilk)
v5:
    - remove section number.
      (suggested by Jan Beulich)
    - remove double blank.
      (suggested by Jan Beulich)
v4:
    - create this patch because of removing all old CAT/CDP codes to make
      implementation be more easily understood.
      (suggested by Jan Beulich)
---
 xen/arch/x86/psr.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

Comments

Jan Beulich April 3, 2017, 3:50 p.m. UTC | #1

>>> On 01.04.17 at 15:53, <[e-mail]> wrote:

I was about to ack this, but there are a few more things which bother
me.

> ---
> v10:
>     - remove initialization for 'PSR_SOCKET_L3_CAT'.
>       (suggested by Jan Beulich)
>     - rename 'feat_ops' to 'feat_props'.
>       (suggested by Jan Beulich)
>     - move 'cbm_len' to 'feat_props' because it is feature independent so far.
>       (suggested by Jan Beulich)
>     - move 'cos_max' to 'feat_props' because it is feature independent.
>       (suggested by Jan Beulich)
>     - move 'cos_num' to 'feat_props' because it is feature independent.
>       (suggested by Jan Beulich)
>     - remove union 'info' and struct 'psr_cat_hw_info'.
>     - remove 'get_cos_max' from 'feat_props'.
>       (suggested by Jan Beulich)
>     - remove 'feat_mask' from 'psr_socket_info' because we can use 'features[]'
>       to check if any feature is initialized.
>       (suggested by Jan Beulich)
>     - move 'ref_lock' above 'cos_ref'.
>       (suggested by Jan Beulich)

The movement done is fine for the moment, but it would have been
even better if you had moved it ahead of the other array too.

> +enum psr_feat_type {
> +    PSR_SOCKET_L3_CAT,
> +    PSR_SOCKET_L3_CDP,
> +    PSR_SOCKET_L2_CAT,
> +    PSR_SOCKET_MAX_FEAT,
> +};

It's not really logical to have the first three here - they should have
been added by their respective patches. Which gets me back to
the question of the usefulness of a patch like this one: Without the
following patches, everything being added here is unused. Iirc the
original version of this patch was quite a bit larger, better justifying
to break out all of this. The size it has shrunk to by now is a pretty
good indication that it should have been folded altogether.

Also I think we've had the discussion about the difference between
"max" and "num" already: The former normally indicates an inclusive
upper bound, while the latter would usually be an exclusive one.
Clearly the above naming doesn't match up with this.

> +/*
> + * This structure represents one feature.
> + * feat_props  - Feature properties, including operation callback functions
> +                 and feature common values.
> + * cos_reg_val - Array to store the values of COS registers. One entry stores
> + *               the value of one COS register.
> + *               For L3 CAT and L2 CAT, one entry corresponds to one COS_ID.
> + *               For CDP, two entries correspond to one COS_ID. E.g.
> + *               COS_ID=0 corresponds to cos_reg_val[0] (Data) and
> + *               cos_reg_val[1] (Code).
> + */
> +struct feat_node {
> +    /*
> +     * This structure defines feature operation callback functions. Every
> +     * feature enabled MUST implement such callback functions and register
> +     * them to props.
> +     *
> +     * Feature specific behaviors will be encapsulated into these callback
> +     * functions. Then, the main flows will not be changed when introducing
> +     * a new feature.
> +     *
> +     * Feature independent HW info and common values are also defined in it.
> +     */
> +    const struct feat_props {
> +        /*
> +         * cos_num, cos_max and cbm_len are common values for all features
> +         * so far.
> +         * cos_num - COS registers number that feature uses for one COS ID.
> +         *           It is defined in SDM.
> +         * cos_max - The max COS registers number got through CPUID.
> +         * cbm_len - The length of CBM got through CPUID.
> +         */
> +        unsigned int cos_num;
> +        unsigned int cos_max;
> +        unsigned int cbm_len;
> +    } *props;

Let's think the data arrangement changes done so far a little further:
Why do we need this pointer per-node (i.e. once per socket)? Now
that we have a well established order of features used to index
struct psr_socket_info's features[], why can't the same indexing be
used to obtain the props pointer from a static (single instance) array
of props pointers?

Otoh I'm not sure you really meant to move all three data members
into there, if you still have reason to believe that different sockets
may have different values for cos_max and/or cbm_len. I.e. these
might better be members of struct feat_node (just not placed in a
union, as you had them in v9).

Jan

Yi Sun April 5, 2017, 3:12 a.m. UTC | #2

On 17-04-03 09:50:34, Jan Beulich wrote:
> >>> On 01.04.17 at 15:53, <[e-mail]> wrote:
> 
> I was about to ack this, but there are a few more things which bother
> me.
> 
Do you mean ack to this patch 3 or whole patch set? Thanks!

> > ---
> > v10:
> >     - remove initialization for 'PSR_SOCKET_L3_CAT'.
> >       (suggested by Jan Beulich)
> >     - rename 'feat_ops' to 'feat_props'.
> >       (suggested by Jan Beulich)
> >     - move 'cbm_len' to 'feat_props' because it is feature independent so far.
> >       (suggested by Jan Beulich)
> >     - move 'cos_max' to 'feat_props' because it is feature independent.
> >       (suggested by Jan Beulich)
> >     - move 'cos_num' to 'feat_props' because it is feature independent.
> >       (suggested by Jan Beulich)
> >     - remove union 'info' and struct 'psr_cat_hw_info'.
> >     - remove 'get_cos_max' from 'feat_props'.
> >       (suggested by Jan Beulich)
> >     - remove 'feat_mask' from 'psr_socket_info' because we can use 'features[]'
> >       to check if any feature is initialized.
> >       (suggested by Jan Beulich)
> >     - move 'ref_lock' above 'cos_ref'.
> >       (suggested by Jan Beulich)
> 
> The movement done is fine for the moment, but it would have been
> even better if you had moved it ahead of the other array too.
> 
Got it.

> > +enum psr_feat_type {
> > +    PSR_SOCKET_L3_CAT,
> > +    PSR_SOCKET_L3_CDP,
> > +    PSR_SOCKET_L2_CAT,
> > +    PSR_SOCKET_MAX_FEAT,
> > +};
> 
> It's not really logical to have the first three here - they should have
> been added by their respective patches. Which gets me back to
> the question of the usefulness of a patch like this one: Without the
> following patches, everything being added here is unused. Iirc the
> original version of this patch was quite a bit larger, better justifying
> to break out all of this. The size it has shrunk to by now is a pretty
> good indication that it should have been folded altogether.
> 
Ok, I will add item one by one in related feature's patch. But can I keep
this patch 3? This patch outlines the infrastructure and I demonstrated how
I analyze the data structures in commit message. If I split these data
structures into pieces and implement them into different patches, I am
afraid to lose the completeness.

> Also I think we've had the discussion about the difference between
> "max" and "num" already: The former normally indicates an inclusive
> upper bound, while the latter would usually be an exclusive one.
> Clearly the above naming doesn't match up with this.
> 
Ok, may change it to 'PSR_SOCKET_FEAT_NUM'.

> > +/*
> > + * This structure represents one feature.
> > + * feat_props  - Feature properties, including operation callback functions
> > +                 and feature common values.
> > + * cos_reg_val - Array to store the values of COS registers. One entry stores
> > + *               the value of one COS register.
> > + *               For L3 CAT and L2 CAT, one entry corresponds to one COS_ID.
> > + *               For CDP, two entries correspond to one COS_ID. E.g.
> > + *               COS_ID=0 corresponds to cos_reg_val[0] (Data) and
> > + *               cos_reg_val[1] (Code).
> > + */
> > +struct feat_node {
> > +    /*
> > +     * This structure defines feature operation callback functions. Every
> > +     * feature enabled MUST implement such callback functions and register
> > +     * them to props.
> > +     *
> > +     * Feature specific behaviors will be encapsulated into these callback
> > +     * functions. Then, the main flows will not be changed when introducing
> > +     * a new feature.
> > +     *
> > +     * Feature independent HW info and common values are also defined in it.
> > +     */
> > +    const struct feat_props {
> > +        /*
> > +         * cos_num, cos_max and cbm_len are common values for all features
> > +         * so far.
> > +         * cos_num - COS registers number that feature uses for one COS ID.
> > +         *           It is defined in SDM.
> > +         * cos_max - The max COS registers number got through CPUID.
> > +         * cbm_len - The length of CBM got through CPUID.
> > +         */
> > +        unsigned int cos_num;
> > +        unsigned int cos_max;
> > +        unsigned int cbm_len;
> > +    } *props;
> 
> Let's think the data arrangement changes done so far a little further:
> Why do we need this pointer per-node (i.e. once per socket)? Now
> that we have a well established order of features used to index
> struct psr_socket_info's features[], why can't the same indexing be
> used to obtain the props pointer from a static (single instance) array
> of props pointers?
> 
Hmm, yes, we can declare a static standalone array of props pointers for all
features and all sockets. It does not belong to 'feat_node' or 'socket_info'.

> Otoh I'm not sure you really meant to move all three data members
> into there, if you still have reason to believe that different sockets
> may have different values for cos_max and/or cbm_len. I.e. these
> might better be members of struct feat_node (just not placed in a
> union, as you had them in v9).
> 
We still believe there may be chances that different sockets may have different
configurations. So, I would prefer to keep cos_max/cbm_len in 'feat_node'.

Because this Friday is the code freeze date, can I quickly make the changes
according to above comments and submit a new version if you do not have
further oppinion? Thanks!

BRs,
Sun Yi

Jan Beulich April 5, 2017, 8:20 a.m. UTC | #3

>>> On 05.04.17 at 05:12, <[e-mail]> wrote:
> On 17-04-03 09:50:34, Jan Beulich wrote:
>> >>> On 01.04.17 at 15:53, <[e-mail]> wrote:
>> 
>> I was about to ack this, but there are a few more things which bother
>> me.
>> 
> Do you mean ack to this patch 3 or whole patch set? Thanks!

Well, this one patch only - I didn't even get to look at the others yet.

>> > +enum psr_feat_type {
>> > +    PSR_SOCKET_L3_CAT,
>> > +    PSR_SOCKET_L3_CDP,
>> > +    PSR_SOCKET_L2_CAT,
>> > +    PSR_SOCKET_MAX_FEAT,
>> > +};
>> 
>> It's not really logical to have the first three here - they should have
>> been added by their respective patches. Which gets me back to
>> the question of the usefulness of a patch like this one: Without the
>> following patches, everything being added here is unused. Iirc the
>> original version of this patch was quite a bit larger, better justifying
>> to break out all of this. The size it has shrunk to by now is a pretty
>> good indication that it should have been folded altogether.
>> 
> Ok, I will add item one by one in related feature's patch. But can I keep
> this patch 3? This patch outlines the infrastructure and I demonstrated how
> I analyze the data structures in commit message. If I split these data
> structures into pieces and implement them into different patches, I am
> afraid to lose the completeness.

Well, in the interest of not needlessly delaying forward progress
I'm fine with you keeping this patch for now. Should the series
miss 4.9, though, I'd prefer if you eliminated it.

>> > +/*
>> > + * This structure represents one feature.
>> > + * feat_props  - Feature properties, including operation callback functions
>> > +                 and feature common values.
>> > + * cos_reg_val - Array to store the values of COS registers. One entry stores
>> > + *               the value of one COS register.
>> > + *               For L3 CAT and L2 CAT, one entry corresponds to one COS_ID.
>> > + *               For CDP, two entries correspond to one COS_ID. E.g.
>> > + *               COS_ID=0 corresponds to cos_reg_val[0] (Data) and
>> > + *               cos_reg_val[1] (Code).
>> > + */
>> > +struct feat_node {
>> > +    /*
>> > +     * This structure defines feature operation callback functions. Every
>> > +     * feature enabled MUST implement such callback functions and register
>> > +     * them to props.
>> > +     *
>> > +     * Feature specific behaviors will be encapsulated into these callback
>> > +     * functions. Then, the main flows will not be changed when introducing
>> > +     * a new feature.
>> > +     *
>> > +     * Feature independent HW info and common values are also defined in it.
>> > +     */
>> > +    const struct feat_props {
>> > +        /*
>> > +         * cos_num, cos_max and cbm_len are common values for all features
>> > +         * so far.
>> > +         * cos_num - COS registers number that feature uses for one COS ID.
>> > +         *           It is defined in SDM.
>> > +         * cos_max - The max COS registers number got through CPUID.
>> > +         * cbm_len - The length of CBM got through CPUID.
>> > +         */
>> > +        unsigned int cos_num;
>> > +        unsigned int cos_max;
>> > +        unsigned int cbm_len;
>> > +    } *props;
>> 
>> Let's think the data arrangement changes done so far a little further:
>> Why do we need this pointer per-node (i.e. once per socket)? Now
>> that we have a well established order of features used to index
>> struct psr_socket_info's features[], why can't the same indexing be
>> used to obtain the props pointer from a static (single instance) array
>> of props pointers?
>> 
> Hmm, yes, we can declare a static standalone array of props pointers for all
> features and all sockets. It does not belong to 'feat_node' or 
> 'socket_info'.
> 
>> Otoh I'm not sure you really meant to move all three data members
>> into there, if you still have reason to believe that different sockets
>> may have different values for cos_max and/or cbm_len. I.e. these
>> might better be members of struct feat_node (just not placed in a
>> union, as you had them in v9).
>> 
> We still believe there may be chances that different sockets may have different
> configurations. So, I would prefer to keep cos_max/cbm_len in 'feat_node'.

This is contradictory: The two fields aren't in struct feat_node in
the current version of the patch, so do you mean "keep in struct
feat_props" or "move back to struct feat_node"?

> Because this Friday is the code freeze date, can I quickly make the changes
> according to above comments and submit a new version if you do not have
> further oppinion? Thanks!

As said above, I didn't get to look at the other patches yet. It's
up to you whether to resubmit with the adjustments done here
(and carried through the rest of the series), or whether you wait.
As it's Wednesday already, don't have too much hope for the
series to make 4.9 - I'm sorry for this, but I also can't do much
about it.

Jan

Yi Sun April 5, 2017, 8:45 a.m. UTC | #4

On 17-04-05 02:20:53, Jan Beulich wrote:
> >>> On 05.04.17 at 05:12, <[e-mail]> wrote:
> > On 17-04-03 09:50:34, Jan Beulich wrote:
> >> >>> On 01.04.17 at 15:53, <[e-mail]> wrote:
> >> > +enum psr_feat_type {
> >> > +    PSR_SOCKET_L3_CAT,
> >> > +    PSR_SOCKET_L3_CDP,
> >> > +    PSR_SOCKET_L2_CAT,
> >> > +    PSR_SOCKET_MAX_FEAT,
> >> > +};
> >> 
> >> It's not really logical to have the first three here - they should have
> >> been added by their respective patches. Which gets me back to
> >> the question of the usefulness of a patch like this one: Without the
> >> following patches, everything being added here is unused. Iirc the
> >> original version of this patch was quite a bit larger, better justifying
> >> to break out all of this. The size it has shrunk to by now is a pretty
> >> good indication that it should have been folded altogether.
> >> 
> > Ok, I will add item one by one in related feature's patch. But can I keep
> > this patch 3? This patch outlines the infrastructure and I demonstrated how
> > I analyze the data structures in commit message. If I split these data
> > structures into pieces and implement them into different patches, I am
> > afraid to lose the completeness.
> 
> Well, in the interest of not needlessly delaying forward progress
> I'm fine with you keeping this patch for now. Should the series
> miss 4.9, though, I'd prefer if you eliminated it.
> 
As other patches are still not reviewed yet, I am afraid the 4.9 will be missed.
Then, I will consider to eliminate this patch 3.

> >> > +/*
> >> > + * This structure represents one feature.
> >> > + * feat_props  - Feature properties, including operation callback functions
> >> > +                 and feature common values.
> >> > + * cos_reg_val - Array to store the values of COS registers. One entry stores
> >> > + *               the value of one COS register.
> >> > + *               For L3 CAT and L2 CAT, one entry corresponds to one COS_ID.
> >> > + *               For CDP, two entries correspond to one COS_ID. E.g.
> >> > + *               COS_ID=0 corresponds to cos_reg_val[0] (Data) and
> >> > + *               cos_reg_val[1] (Code).
> >> > + */
> >> > +struct feat_node {
> >> > +    /*
> >> > +     * This structure defines feature operation callback functions. Every
> >> > +     * feature enabled MUST implement such callback functions and register
> >> > +     * them to props.
> >> > +     *
> >> > +     * Feature specific behaviors will be encapsulated into these callback
> >> > +     * functions. Then, the main flows will not be changed when introducing
> >> > +     * a new feature.
> >> > +     *
> >> > +     * Feature independent HW info and common values are also defined in it.
> >> > +     */
> >> > +    const struct feat_props {
> >> > +        /*
> >> > +         * cos_num, cos_max and cbm_len are common values for all features
> >> > +         * so far.
> >> > +         * cos_num - COS registers number that feature uses for one COS ID.
> >> > +         *           It is defined in SDM.
> >> > +         * cos_max - The max COS registers number got through CPUID.
> >> > +         * cbm_len - The length of CBM got through CPUID.
> >> > +         */
> >> > +        unsigned int cos_num;
> >> > +        unsigned int cos_max;
> >> > +        unsigned int cbm_len;
> >> > +    } *props;
> >> 
> >> Let's think the data arrangement changes done so far a little further:
> >> Why do we need this pointer per-node (i.e. once per socket)? Now
> >> that we have a well established order of features used to index
> >> struct psr_socket_info's features[], why can't the same indexing be
> >> used to obtain the props pointer from a static (single instance) array
> >> of props pointers?
> >> 
> > Hmm, yes, we can declare a static standalone array of props pointers for all
> > features and all sockets. It does not belong to 'feat_node' or 
> > 'socket_info'.
> > 
> >> Otoh I'm not sure you really meant to move all three data members
> >> into there, if you still have reason to believe that different sockets
> >> may have different values for cos_max and/or cbm_len. I.e. these
> >> might better be members of struct feat_node (just not placed in a
> >> union, as you had them in v9).
> >> 
> > We still believe there may be chances that different sockets may have different
> > configurations. So, I would prefer to keep cos_max/cbm_len in 'feat_node'.
> 
> This is contradictory: The two fields aren't in struct feat_node in
> the current version of the patch, so do you mean "keep in struct
> feat_props" or "move back to struct feat_node"?
> 
"move back to struct feat_node".

> > Because this Friday is the code freeze date, can I quickly make the changes
> > according to above comments and submit a new version if you do not have
> > further oppinion? Thanks!
> 
> As said above, I didn't get to look at the other patches yet. It's
> up to you whether to resubmit with the adjustments done here
> (and carried through the rest of the series), or whether you wait.
> As it's Wednesday already, don't have too much hope for the
> series to make 4.9 - I'm sorry for this, but I also can't do much
> about it.
> 
Per current status, I will wait for all your review comments. Thanks!

BRs,
Sun Yi

Patch

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 96a8589..cf352d2 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -13,16 +13,100 @@ 
  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
  * more details.
  */
-#include <xen/init.h>
 #include <xen/cpu.h>
 #include <xen/err.h>
+#include <xen/init.h>
 #include <xen/sched.h>
 #include <asm/psr.h>
 
+/*
+ * Terminology:
+ * - CAT         Cache Allocation Technology
+ * - CBM         Capacity BitMasks
+ * - CDP         Code and Data Prioritization
+ * - COS/CLOS    Class of Service. Also mean COS registers.
+ * - COS_MAX     Max number of COS for the feature (minus 1)
+ * - MSRs        Machine Specific Registers
+ * - PSR         Intel Platform Shared Resource
+ */
+
 #define PSR_CMT        (1<<0)
 #define PSR_CAT        (1<<1)
 #define PSR_CDP        (1<<2)
 
+/*
+ * Per SDM chapter 'Cache Allocation Technology: Cache Mask Configuration',
+ * the MSRs ranging from 0C90H through 0D0FH (inclusive), enables support for
+ * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127].
+ *
+ * The MSRs ranging from 0D10H through 0D4FH (inclusive), enables support for
+ * up to 64 L2 CAT COS. The COS_ID=[0,63].
+ *
+ * So, the maximum COS register count of one feature is 128.
+ */
+#define MAX_COS_REG_CNT  128
+
+enum psr_feat_type {
+    PSR_SOCKET_L3_CAT,
+    PSR_SOCKET_L3_CDP,
+    PSR_SOCKET_L2_CAT,
+    PSR_SOCKET_MAX_FEAT,
+};
+
+/*
+ * This structure represents one feature.
+ * feat_props  - Feature properties, including operation callback functions
+                 and feature common values.
+ * cos_reg_val - Array to store the values of COS registers. One entry stores
+ *               the value of one COS register.
+ *               For L3 CAT and L2 CAT, one entry corresponds to one COS_ID.
+ *               For CDP, two entries correspond to one COS_ID. E.g.
+ *               COS_ID=0 corresponds to cos_reg_val[0] (Data) and
+ *               cos_reg_val[1] (Code).
+ */
+struct feat_node {
+    /*
+     * This structure defines feature operation callback functions. Every
+     * feature enabled MUST implement such callback functions and register
+     * them to props.
+     *
+     * Feature specific behaviors will be encapsulated into these callback
+     * functions. Then, the main flows will not be changed when introducing
+     * a new feature.
+     *
+     * Feature independent HW info and common values are also defined in it.
+     */
+    const struct feat_props {
+        /*
+         * cos_num, cos_max and cbm_len are common values for all features
+         * so far.
+         * cos_num - COS registers number that feature uses for one COS ID.
+         *           It is defined in SDM.
+         * cos_max - The max COS registers number got through CPUID.
+         * cbm_len - The length of CBM got through CPUID.
+         */
+        unsigned int cos_num;
+        unsigned int cos_max;
+        unsigned int cbm_len;
+    } *props;
+
+    uint32_t cos_reg_val[MAX_COS_REG_CNT];
+};
+
+/*
+ * PSR features are managed per socket. Below structure defines the members
+ * used to manage these features.
+ * features  - A feature node array used to manage all features enabled.
+ * ref_lock  - A lock to protect cos_ref.
+ * cos_ref   - A reference count array to record how many domains are using the
+ *             COS ID. Every entry of cos_ref corresponds to one COS ID.
+ */
+struct psr_socket_info {
+    struct feat_node *features[PSR_SOCKET_MAX_FEAT];
+    spinlock_t ref_lock;
+    unsigned int cos_ref[MAX_COS_REG_CNT];
+};
+
 struct psr_assoc {
     uint64_t val;
     uint64_t cos_mask;