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

Commit Message

Yi Sun March 16, 2017, 11:07 a.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 save same
L3/L2 values. So, the value 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 callback
functions (all feature's specific behaviors are encapsulated into these
callback functions), 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]>
Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
---
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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)

Comments

Jan Beulich March 24, 2017, 4:19 p.m. UTC | #1

>>> On 16.03.17 at 12:07, <[e-mail]> wrote:
> +enum psr_feat_type {
> +    PSR_SOCKET_L3_CAT = 0,

Pointless " = 0".

> +    PSR_SOCKET_L3_CDP,
> +    PSR_SOCKET_L2_CAT,
> +    PSR_SOCKET_MAX_FEAT,
> +};
> +
> +/* CAT/CDP HW info data structure. */
> +struct psr_cat_hw_info {
> +    unsigned int cbm_len;
> +    unsigned int cos_max;

So you have this field, and ...

> +};
> +
> +/*
> + * This structure represents one feature.
> + * feat_ops    - Feature operation callback functions.
> + * info        - Feature HW info.
> + * 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).
> + * cos_num     - COS registers number that feature uses in one time access.
> + */
> +struct feat_node {
> +    /*
> +     * This structure defines feature operation callback functions. Every feature
> +     * enabled MUST implement such callback functions and register them to ops.
> +     *
> +     * Feature specific behaviors will be encapsulated into these callback
> +     * functions. Then, the main flows will not be changed when introducing a new
> +     * feature.
> +     */
> +    struct feat_ops {
> +        /* get_cos_max is used to get feature's cos_max. */
> +        unsigned int (*get_cos_max)(const struct feat_node *feat);

... you have this op, suggesting that you expect all features
to have a cos_max. Why don't you then store the value in a
field which is not per-feature, just like ...

> +    } ops;
> +
> +    /* Encapsulate feature specific HW info here. */
> +    union {
> +        struct psr_cat_hw_info cat_info;
> +    } info;
> +
> +    uint32_t cos_reg_val[MAX_COS_REG_CNT];
> +    unsigned int cos_num;

... this. I'm pretty sure that during v8 review I did say that
this approach should be extended to all pieces of information
where it can be applied.

Also please place the array last, so that accesses to most/all
other fields have a better chance of working with 8-bit
displacements.

Furthermore, didn't we settle on ops being a pointer to a const
struct, initialized by taking the address of a static const object?
There is no reason to duplicate all the pointers in every node.

> +struct psr_socket_info {
> +    /*
> +     * It maps to values defined in 'enum psr_feat_type' below. Value in 'enum
> +     * psr_feat_type' means the bit position.
> +     * bit 0:   L3 CAT
> +     * bit 1:   L3 CDP
> +     * bit 2:   L2 CAT
> +     */
> +    unsigned int feat_mask;

Comment or not I don't understand what use this mask is, and
this is again something which I'm pretty sure I've mentioned in
v8 review, when the switch to ...

> +    struct feat_node *features[PSR_SOCKET_MAX_FEAT];
> +    unsigned int cos_ref[MAX_COS_REG_CNT];

... this array was suggested by Roger. The pointers in the
array being non-NULL can - afaict - easily fulfill the role of
the mask bits, so the latter are redundant.

Jan

Yi Sun March 27, 2017, 2:38 a.m. UTC | #2

On 17-03-24 10:19:30, Jan Beulich wrote:
> >>> On 16.03.17 at 12:07, <[e-mail]> wrote:
> > +enum psr_feat_type {
> > +    PSR_SOCKET_L3_CAT = 0,
> 
> Pointless " = 0".
> 
Ok, will remove it.

> > +    PSR_SOCKET_L3_CDP,
> > +    PSR_SOCKET_L2_CAT,
> > +    PSR_SOCKET_MAX_FEAT,
> > +};
> > +
> > +/* CAT/CDP HW info data structure. */
> > +struct psr_cat_hw_info {
> > +    unsigned int cbm_len;
> > +    unsigned int cos_max;
> 
> So you have this field, and ...
> 
> > +};
> > +
> > +/*
> > + * This structure represents one feature.
> > + * feat_ops    - Feature operation callback functions.
> > + * info        - Feature HW info.
> > + * 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).
> > + * cos_num     - COS registers number that feature uses in one time access.
> > + */
> > +struct feat_node {
> > +    /*
> > +     * This structure defines feature operation callback functions. Every feature
> > +     * enabled MUST implement such callback functions and register them to ops.
> > +     *
> > +     * Feature specific behaviors will be encapsulated into these callback
> > +     * functions. Then, the main flows will not be changed when introducing a new
> > +     * feature.
> > +     */
> > +    struct feat_ops {
> > +        /* get_cos_max is used to get feature's cos_max. */
> > +        unsigned int (*get_cos_max)(const struct feat_node *feat);
> 
> ... you have this op, suggesting that you expect all features
> to have a cos_max. Why don't you then store the value in a
> field which is not per-feature, just like ...
> 
> > +    } ops;
> > +
> > +    /* Encapsulate feature specific HW info here. */
> > +    union {
> > +        struct psr_cat_hw_info cat_info;
> > +    } info;
> > +
> > +    uint32_t cos_reg_val[MAX_COS_REG_CNT];
> > +    unsigned int cos_num;
> 
> ... this. I'm pretty sure that during v8 review I did say that
> this approach should be extended to all pieces of information
> where it can be applied.
> 
I thought this when implementing v9. As cos_max is part of feature HW info, I
thought it would be better to keep it in hw_info structure. Different features
may have different hw_info, so the callback function is needed to get cos_max.
Of course, we can keep a copy in feat_node but it is redundant. How do you
think?

> Also please place the array last, so that accesses to most/all
> other fields have a better chance of working with 8-bit
> displacements.
> 
Sure. Thanks!

> Furthermore, didn't we settle on ops being a pointer to a const
> struct, initialized by taking the address of a static const object?
> There is no reason to duplicate all the pointers in every node.
> 
Will correct this.

> > +struct psr_socket_info {
> > +    /*
> > +     * It maps to values defined in 'enum psr_feat_type' below. Value in 'enum
> > +     * psr_feat_type' means the bit position.
> > +     * bit 0:   L3 CAT
> > +     * bit 1:   L3 CDP
> > +     * bit 2:   L2 CAT
> > +     */
> > +    unsigned int feat_mask;
> 
> Comment or not I don't understand what use this mask is, and
> this is again something which I'm pretty sure I've mentioned in
> v8 review, when the switch to ...
> 
> > +    struct feat_node *features[PSR_SOCKET_MAX_FEAT];
> > +    unsigned int cos_ref[MAX_COS_REG_CNT];
> 
> ... this array was suggested by Roger. The pointers in the
> array being non-NULL can - afaict - easily fulfill the role of
> the mask bits, so the latter are redundant.
> 
I thought 'feat_mask' can facilitate things handling. If we check feature array
to know if any feature has been initialized, we have to iterate the array. But
it is simple to check 'feat_mask'.
1. In 'psr_cpu_init', we can use it to check if initialization on the socket
   has been done.
    if ( info->feat_mask )
        goto assoc_init;
2. Same purpose in 'psr_assoc_init'.
    if ( info->feat_mask )
        psra->cos_mask = ((1ull << get_count_order(cos_max)) - 1) <<
                         PSR_ASSOC_REG_SHIFT;
3. Same purpose in 'get_socket_info'.
    if ( !socket_info[socket].feat_mask )
 
What is your advice? Thanks!

> Jan

Jan Beulich March 27, 2017, 6:20 a.m. UTC | #3

>>> On 27.03.17 at 04:38, <[e-mail]> wrote:
> On 17-03-24 10:19:30, Jan Beulich wrote:
>> >>> On 16.03.17 at 12:07, <[e-mail]> wrote:
>> > +struct psr_cat_hw_info {
>> > +    unsigned int cbm_len;
>> > +    unsigned int cos_max;
>> 
>> So you have this field, and ...
>> 
>> > +};
>> > +
>> > +/*
>> > + * This structure represents one feature.
>> > + * feat_ops    - Feature operation callback functions.
>> > + * info        - Feature HW info.
>> > + * 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).
>> > + * cos_num     - COS registers number that feature uses in one time access.
>> > + */
>> > +struct feat_node {
>> > +    /*
>> > +     * This structure defines feature operation callback functions. Every feature
>> > +     * enabled MUST implement such callback functions and register them to ops.
>> > +     *
>> > +     * Feature specific behaviors will be encapsulated into these callback
>> > +     * functions. Then, the main flows will not be changed when introducing a new
>> > +     * feature.
>> > +     */
>> > +    struct feat_ops {
>> > +        /* get_cos_max is used to get feature's cos_max. */
>> > +        unsigned int (*get_cos_max)(const struct feat_node *feat);
>> 
>> ... you have this op, suggesting that you expect all features
>> to have a cos_max. Why don't you then store the value in a
>> field which is not per-feature, just like ...
>> 
>> > +    } ops;
>> > +
>> > +    /* Encapsulate feature specific HW info here. */
>> > +    union {
>> > +        struct psr_cat_hw_info cat_info;
>> > +    } info;
>> > +
>> > +    uint32_t cos_reg_val[MAX_COS_REG_CNT];
>> > +    unsigned int cos_num;
>> 
>> ... this. I'm pretty sure that during v8 review I did say that
>> this approach should be extended to all pieces of information
>> where it can be applied.
>> 
> I thought this when implementing v9. As cos_max is part of feature HW info, I
> thought it would be better to keep it in hw_info structure. Different features
> may have different hw_info, so the callback function is needed to get cos_max.
> Of course, we can keep a copy in feat_node but it is redundant. How do you
> think?

I don't follow - as long as you have a universal get_cos_max()
accesses, and as long as what that function returns depends
only on invariable things like CPUID output, I don't see why
this needs to be a function instead of a data field. If some
(perhaps future, theoretical) feature didn't want/need a
get_cos_max() function, the presence of that hook would
become questionable, yet it could surely become an optional
hook. However, the hook being optional could as well be
represented by the data field getting assigned a value of 0.

Bottom line: Data which can be calculated at initialization
time should be stored in a date object, rather than re-
calculating it over and over.

>> > +struct psr_socket_info {
>> > +    /*
>> > +     * It maps to values defined in 'enum psr_feat_type' below. Value in 'enum
>> > +     * psr_feat_type' means the bit position.
>> > +     * bit 0:   L3 CAT
>> > +     * bit 1:   L3 CDP
>> > +     * bit 2:   L2 CAT
>> > +     */
>> > +    unsigned int feat_mask;
>> 
>> Comment or not I don't understand what use this mask is, and
>> this is again something which I'm pretty sure I've mentioned in
>> v8 review, when the switch to ...
>> 
>> > +    struct feat_node *features[PSR_SOCKET_MAX_FEAT];
>> > +    unsigned int cos_ref[MAX_COS_REG_CNT];
>> 
>> ... this array was suggested by Roger. The pointers in the
>> array being non-NULL can - afaict - easily fulfill the role of
>> the mask bits, so the latter are redundant.
>> 
> I thought 'feat_mask' can facilitate things handling. If we check feature 
> array
> to know if any feature has been initialized, we have to iterate the array. 
> But
> it is simple to check 'feat_mask'.
> 1. In 'psr_cpu_init', we can use it to check if initialization on the socket
>    has been done.
>     if ( info->feat_mask )
>         goto assoc_init;
> 2. Same purpose in 'psr_assoc_init'.
>     if ( info->feat_mask )
>         psra->cos_mask = ((1ull << get_count_order(cos_max)) - 1) <<
>                          PSR_ASSOC_REG_SHIFT;
> 3. Same purpose in 'get_socket_info'.
>     if ( !socket_info[socket].feat_mask )
>  
> What is your advice? Thanks!

My advice is: Avoid redundant data as much as possible. Any such
instance poses the risk of the two pieces of information going out
of sync. (That isn't to say that there aren't cases where redundancy
is almost unavoidable, e.g. in order to not overly complicate code,
but that's pretty clearly not the case here).

Jan

Yi Sun March 27, 2017, 7:12 a.m. UTC | #4

On 17-03-27 00:20:58, Jan Beulich wrote:
> >>> On 27.03.17 at 04:38, <[e-mail]> wrote:
> > On 17-03-24 10:19:30, Jan Beulich wrote:
> >> >>> On 16.03.17 at 12:07, <[e-mail]> wrote:
> >> > +struct psr_cat_hw_info {
> >> > +    unsigned int cbm_len;
> >> > +    unsigned int cos_max;
> >> 
> >> So you have this field, and ...
> >> 
> >> > +};
> >> > +
> >> > +/*
> >> > + * This structure represents one feature.
> >> > + * feat_ops    - Feature operation callback functions.
> >> > + * info        - Feature HW info.
> >> > + * 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).
> >> > + * cos_num     - COS registers number that feature uses in one time access.
> >> > + */
> >> > +struct feat_node {
> >> > +    /*
> >> > +     * This structure defines feature operation callback functions. Every feature
> >> > +     * enabled MUST implement such callback functions and register them to ops.
> >> > +     *
> >> > +     * Feature specific behaviors will be encapsulated into these callback
> >> > +     * functions. Then, the main flows will not be changed when introducing a new
> >> > +     * feature.
> >> > +     */
> >> > +    struct feat_ops {
> >> > +        /* get_cos_max is used to get feature's cos_max. */
> >> > +        unsigned int (*get_cos_max)(const struct feat_node *feat);
> >> 
> >> ... you have this op, suggesting that you expect all features
> >> to have a cos_max. Why don't you then store the value in a
> >> field which is not per-feature, just like ...
> >> 
> >> > +    } ops;
> >> > +
> >> > +    /* Encapsulate feature specific HW info here. */
> >> > +    union {
> >> > +        struct psr_cat_hw_info cat_info;
> >> > +    } info;
> >> > +
> >> > +    uint32_t cos_reg_val[MAX_COS_REG_CNT];
> >> > +    unsigned int cos_num;
> >> 
> >> ... this. I'm pretty sure that during v8 review I did say that
> >> this approach should be extended to all pieces of information
> >> where it can be applied.
> >> 
> > I thought this when implementing v9. As cos_max is part of feature HW info, I
> > thought it would be better to keep it in hw_info structure. Different features
> > may have different hw_info, so the callback function is needed to get cos_max.
> > Of course, we can keep a copy in feat_node but it is redundant. How do you
> > think?
> 
> I don't follow - as long as you have a universal get_cos_max()
> accesses, and as long as what that function returns depends
> only on invariable things like CPUID output, I don't see why
> this needs to be a function instead of a data field. If some
> (perhaps future, theoretical) feature didn't want/need a
> get_cos_max() function, the presence of that hook would
> become questionable, yet it could surely become an optional
> hook. However, the hook being optional could as well be
> represented by the data field getting assigned a value of 0.
> 
> Bottom line: Data which can be calculated at initialization
> time should be stored in a date object, rather than re-
> calculating it over and over.
> 
The purpose to use the function is just not to define a redundant member
in 'struct feat_node'.

The cos_max is got in cat_init_feature in patch 5 and kept in the feature's
hw_info. The 'get_cos_max' only returns DIFFERENT features' cos_max without
recalculation. E.g:

CAT/CDP:
static unsigned int cat_get_cos_max(const struct feat_node *feat)
{
    return feat->info.cat_info.cos_max;
}

MBA:
static unsigned int mba_get_cos_max(const struct feat_node *feat)
{
    return feat->info.mba_info.cos_max;
}

But I think it is ok to add a new member in 'struct feat_node' to keep
cos_max for the feature.

What do you prefer? Thanks!

> >> > +struct psr_socket_info {
> >> > +    /*
> >> > +     * It maps to values defined in 'enum psr_feat_type' below. Value in 'enum
> >> > +     * psr_feat_type' means the bit position.
> >> > +     * bit 0:   L3 CAT
> >> > +     * bit 1:   L3 CDP
> >> > +     * bit 2:   L2 CAT
> >> > +     */
> >> > +    unsigned int feat_mask;
> >> 
> >> Comment or not I don't understand what use this mask is, and
> >> this is again something which I'm pretty sure I've mentioned in
> >> v8 review, when the switch to ...
> >> 
> >> > +    struct feat_node *features[PSR_SOCKET_MAX_FEAT];
> >> > +    unsigned int cos_ref[MAX_COS_REG_CNT];
> >> 
> >> ... this array was suggested by Roger. The pointers in the
> >> array being non-NULL can - afaict - easily fulfill the role of
> >> the mask bits, so the latter are redundant.
> >> 
> > I thought 'feat_mask' can facilitate things handling. If we check feature 
> > array
> > to know if any feature has been initialized, we have to iterate the array. 
> > But
> > it is simple to check 'feat_mask'.
> > 1. In 'psr_cpu_init', we can use it to check if initialization on the socket
> >    has been done.
> >     if ( info->feat_mask )
> >         goto assoc_init;
> > 2. Same purpose in 'psr_assoc_init'.
> >     if ( info->feat_mask )
> >         psra->cos_mask = ((1ull << get_count_order(cos_max)) - 1) <<
> >                          PSR_ASSOC_REG_SHIFT;
> > 3. Same purpose in 'get_socket_info'.
> >     if ( !socket_info[socket].feat_mask )
> >  
> > What is your advice? Thanks!
> 
> My advice is: Avoid redundant data as much as possible. Any such
> instance poses the risk of the two pieces of information going out
> of sync. (That isn't to say that there aren't cases where redundancy
> is almost unavoidable, e.g. in order to not overly complicate code,
> but that's pretty clearly not the case here).
> 
If so, I think I should define a function to iterate the function array
to return TRUE if any feature has been set into the array. Then, use
this function to replace above checking points.

> Jan

Jan Beulich March 27, 2017, 7:37 a.m. UTC | #5

>>> On 27.03.17 at 09:12, <[e-mail]> wrote:
> On 17-03-27 00:20:58, Jan Beulich wrote:
>> >>> On 27.03.17 at 04:38, <[e-mail]> wrote:
>> > On 17-03-24 10:19:30, Jan Beulich wrote:
>> >> >>> On 16.03.17 at 12:07, <[e-mail]> wrote:
>> >> > +struct psr_cat_hw_info {
>> >> > +    unsigned int cbm_len;
>> >> > +    unsigned int cos_max;
>> >> 
>> >> So you have this field, and ...
>> >> 
>> >> > +};
>> >> > +
>> >> > +/*
>> >> > + * This structure represents one feature.
>> >> > + * feat_ops    - Feature operation callback functions.
>> >> > + * info        - Feature HW info.
>> >> > + * 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).
>> >> > + * cos_num     - COS registers number that feature uses in one time access.
>> >> > + */
>> >> > +struct feat_node {
>> >> > +    /*
>> >> > +     * This structure defines feature operation callback functions. Every feature
>> >> > +     * enabled MUST implement such callback functions and register them to ops.
>> >> > +     *
>> >> > +     * Feature specific behaviors will be encapsulated into these callback
>> >> > +     * functions. Then, the main flows will not be changed when introducing a new
>> >> > +     * feature.
>> >> > +     */
>> >> > +    struct feat_ops {
>> >> > +        /* get_cos_max is used to get feature's cos_max. */
>> >> > +        unsigned int (*get_cos_max)(const struct feat_node *feat);
>> >> 
>> >> ... you have this op, suggesting that you expect all features
>> >> to have a cos_max. Why don't you then store the value in a
>> >> field which is not per-feature, just like ...
>> >> 
>> >> > +    } ops;
>> >> > +
>> >> > +    /* Encapsulate feature specific HW info here. */
>> >> > +    union {
>> >> > +        struct psr_cat_hw_info cat_info;
>> >> > +    } info;
>> >> > +
>> >> > +    uint32_t cos_reg_val[MAX_COS_REG_CNT];
>> >> > +    unsigned int cos_num;
>> >> 
>> >> ... this. I'm pretty sure that during v8 review I did say that
>> >> this approach should be extended to all pieces of information
>> >> where it can be applied.
>> >> 
>> > I thought this when implementing v9. As cos_max is part of feature HW info, I
>> > thought it would be better to keep it in hw_info structure. Different features
>> > may have different hw_info, so the callback function is needed to get cos_max.
>> > Of course, we can keep a copy in feat_node but it is redundant. How do you
>> > think?
>> 
>> I don't follow - as long as you have a universal get_cos_max()
>> accesses, and as long as what that function returns depends
>> only on invariable things like CPUID output, I don't see why
>> this needs to be a function instead of a data field. If some
>> (perhaps future, theoretical) feature didn't want/need a
>> get_cos_max() function, the presence of that hook would
>> become questionable, yet it could surely become an optional
>> hook. However, the hook being optional could as well be
>> represented by the data field getting assigned a value of 0.
>> 
>> Bottom line: Data which can be calculated at initialization
>> time should be stored in a date object, rather than re-
>> calculating it over and over.
>> 
> The purpose to use the function is just not to define a redundant member
> in 'struct feat_node'.
> 
> The cos_max is got in cat_init_feature in patch 5 and kept in the feature's
> hw_info. The 'get_cos_max' only returns DIFFERENT features' cos_max without
> recalculation. E.g:
> 
> CAT/CDP:
> static unsigned int cat_get_cos_max(const struct feat_node *feat)
> {
>     return feat->info.cat_info.cos_max;
> }
> 
> MBA:
> static unsigned int mba_get_cos_max(const struct feat_node *feat)
> {
>     return feat->info.mba_info.cos_max;
> }
> 
> But I think it is ok to add a new member in 'struct feat_node' to keep
> cos_max for the feature.
> 
> What do you prefer? Thanks!

Sigh. If you see the above two functions, and if you expect future
new features to have similar functions, then why in the world would
you want to make the field feature specific? If every feature is
expected to have some form of maximum COS, then this is a
property applicable to all features and hence should be a field in
the common part of the structure.

Jan

Patch

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 96a8589..822f1c0 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -13,16 +13,112 @@ 
  * 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 = 0,
+    PSR_SOCKET_L3_CDP,
+    PSR_SOCKET_L2_CAT,
+    PSR_SOCKET_MAX_FEAT,
+};
+
+/* CAT/CDP HW info data structure. */
+struct psr_cat_hw_info {
+    unsigned int cbm_len;
+    unsigned int cos_max;
+};
+
+/*
+ * This structure represents one feature.
+ * feat_ops    - Feature operation callback functions.
+ * info        - Feature HW info.
+ * 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).
+ * cos_num     - COS registers number that feature uses in one time access.
+ */
+struct feat_node {
+    /*
+     * This structure defines feature operation callback functions. Every feature
+     * enabled MUST implement such callback functions and register them to ops.
+     *
+     * Feature specific behaviors will be encapsulated into these callback
+     * functions. Then, the main flows will not be changed when introducing a new
+     * feature.
+     */
+    struct feat_ops {
+        /* get_cos_max is used to get feature's cos_max. */
+        unsigned int (*get_cos_max)(const struct feat_node *feat);
+    } ops;
+
+    /* Encapsulate feature specific HW info here. */
+    union {
+        struct psr_cat_hw_info cat_info;
+    } info;
+
+    uint32_t cos_reg_val[MAX_COS_REG_CNT];
+    unsigned int cos_num;
+};
+
+/*
+ * PSR features are managed per socket. Below structure defines the members
+ * used to manage these features.
+ * feat_mask - Mask used to record features enabled on socket. There may be
+ *             some features enabled at same time.
+ * features  - An feature node array used to manage all features enabled.
+ * 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.
+ * ref_lock  - A lock to protect cos_ref.
+ */
+struct psr_socket_info {
+    /*
+     * It maps to values defined in 'enum psr_feat_type' below. Value in 'enum
+     * psr_feat_type' means the bit position.
+     * bit 0:   L3 CAT
+     * bit 1:   L3 CDP
+     * bit 2:   L2 CAT
+     */
+    unsigned int feat_mask;
+    struct feat_node *features[PSR_SOCKET_MAX_FEAT];
+    unsigned int cos_ref[MAX_COS_REG_CNT];
+    spinlock_t ref_lock;
+};
+
 struct psr_assoc {
     uint64_t val;
     uint64_t cos_mask;