diff mbox

[v4,03/24] x86: refactor psr: implement main data structures.

Message ID 1481688484-5093-4-git-send-email-yi.y.sun@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yi Sun Dec. 14, 2016, 4: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 list 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.

For details, please refer spec and codes.

Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 xen/arch/x86/psr.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

Comments

Jan Beulich Dec. 22, 2016, 4:13 p.m. UTC | #1
>>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
> 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 list 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.

The special nature of CDP will make some special handling necessary,
which may need reflection in data structure arrangement. Would you
mind spelling out here how CDP handling is intended to work?

> +/*
> + * Per SDM 17.17.3.3 'Cache Allocation Technology: Cache Mask Configuration',

I think I've asked before to omit section numbers, which tend to
change. Just the title will be enough.

> +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 {
> +    /*
> +     * 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_node *feat,
> +                         struct psr_socket_info *info);
> +};

What is the reason to have a separate structure for this, when you
don't store a pointer in struct feat_node? If this was inlined there,
the odd forward declaration of struct feat_node wouldn't be needed
either. (The same question may apply to struct feat_hw_info.) 

> +
> +

Please avoid double blank lines.

Jan
Yi Sun Dec. 26, 2016, 6:56 a.m. UTC | #2
Hi,

I just realize it is Christmas holiday. Merry Christmas!

On 16-12-22 09:13:43, Jan Beulich wrote:
> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
> > 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 list 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.
> 
> The special nature of CDP will make some special handling necessary,
> which may need reflection in data structure arrangement. Would you
> mind spelling out here how CDP handling is intended to work?
> 
Yes, CDP has its special handling processes. The main difference has been
described above that CDP has half number of COS registers and uses two entries.
Because of these, I split CDP out from L3 CAT and implement CDP its own feature
callback functions from patch 13 to patch 16. You can check them for details.
Thanks!

> > +/*
> > + * Per SDM 17.17.3.3 'Cache Allocation Technology: Cache Mask Configuration',
> 
> I think I've asked before to omit section numbers, which tend to
> change. Just the title will be enough.
> 
Sorry for this, will mention title only.

> > +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 {
> > +    /*
> > +     * 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_node *feat,
> > +                         struct psr_socket_info *info);
> > +};
> 
> What is the reason to have a separate structure for this, when you
> don't store a pointer in struct feat_node? If this was inlined there,
> the odd forward declaration of struct feat_node wouldn't be needed
> either. (The same question may apply to struct feat_hw_info.) 
> 
I just want to make codes be clear. If you prefer inline declaration, I think I
should change it as below, right?

struct feat_node {
......
    struct feat_ops {
        ......
    } ops;
    struct feat_hw_info {
        ......
    } info;
......
};

> > +
> > +
> 
> Please avoid double blank lines.
> 
Sorry, will remove it. Thanks!

> Jan
Jan Beulich Jan. 3, 2017, 8 a.m. UTC | #3
>>> On 26.12.16 at 07:56, <yi.y.sun@linux.intel.com> wrote:
> On 16-12-22 09:13:43, Jan Beulich wrote:
>> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
>> > 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 list 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.
>> 
>> The special nature of CDP will make some special handling necessary,
>> which may need reflection in data structure arrangement. Would you
>> mind spelling out here how CDP handling is intended to work?
>> 
> Yes, CDP has its special handling processes. The main difference has been
> described above that CDP has half number of COS registers and uses two entries.
> Because of these, I split CDP out from L3 CAT and implement CDP its own feature
> callback functions from patch 13 to patch 16. You can check them for details.

Well, my point was to at least sketch out your (data structure
related) intentions in the comment here, to help reviewers (and
future readers) understand how the data structures fit that
special case.

>> > +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 {
>> > +    /*
>> > +     * 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_node *feat,
>> > +                         struct psr_socket_info *info);
>> > +};
>> 
>> What is the reason to have a separate structure for this, when you
>> don't store a pointer in struct feat_node? If this was inlined there,
>> the odd forward declaration of struct feat_node wouldn't be needed
>> either. (The same question may apply to struct feat_hw_info.) 
>> 
> I just want to make codes be clear. If you prefer inline declaration, I 
> think I
> should change it as below, right?
> 
> struct feat_node {
> ......
>     struct feat_ops {
>         ......
>     } ops;
>     struct feat_hw_info {
>         ......
>     } info;
> ......
> };

Well, not exactly: The struct <tag> { ... } <name>; wrappers
are unnecessary then too. With them kept there indeed would be
no big difference between both variants.

Jan
Yi Sun Jan. 3, 2017, 8:49 a.m. UTC | #4
On 17-01-03 01:00:37, Jan Beulich wrote:
> >>> On 26.12.16 at 07:56, <yi.y.sun@linux.intel.com> wrote:
> > On 16-12-22 09:13:43, Jan Beulich wrote:
> >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
> >> > 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.
> >> 
> >> The special nature of CDP will make some special handling necessary,
> >> which may need reflection in data structure arrangement. Would you
> >> mind spelling out here how CDP handling is intended to work?
> >> 
> > Yes, CDP has its special handling processes. The main difference has been
> > described above that CDP has half number of COS registers and uses two entries.
> > Because of these, I split CDP out from L3 CAT and implement CDP its own feature
> > callback functions from patch 13 to patch 16. You can check them for details.
> 
> Well, my point was to at least sketch out your (data structure
> related) intentions in the comment here, to help reviewers (and
> future readers) understand how the data structures fit that
> special case.
> 
Thanks! Will add more comments here to explain CDP special points.

> >> > +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 {
> >> > +    /*
> >> > +     * 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_node *feat,
> >> > +                         struct psr_socket_info *info);
> >> > +};
> >> 
> >> What is the reason to have a separate structure for this, when you
> >> don't store a pointer in struct feat_node? If this was inlined there,
> >> the odd forward declaration of struct feat_node wouldn't be needed
> >> either. (The same question may apply to struct feat_hw_info.) 
> >> 
> > I just want to make codes be clear. If you prefer inline declaration, I 
> > think I
> > should change it as below, right?
> > 
> > struct feat_node {
> > ......
> >     struct feat_ops {
> >         ......
> >     } ops;
> >     struct feat_hw_info {
> >         ......
> >     } info;
> > ......
> > };
> 
> Well, not exactly: The struct <tag> { ... } <name>; wrappers
> are unnecessary then too. With them kept there indeed would be
> no big difference between both variants.
> 
To facilitate the callback functions assignment for a feature, I defined
feature specific callback function ops like below.

struct feat_ops l3_cat_ops = {
    .init_feature = l3_cat_init_feature,
    .get_max_cos_max = l3_cat_get_max_cos_max,
    ......
};

And directly assign it to global feature node in initialization process like
below.

static void cpu_init_work(void)
{
......
            feat_tmp = feat_l3_cat;
            feat_l3_cat = NULL;
            feat_tmp->ops = l3_cat_ops;
......
}

I think this can make codes be clear. How do you think? Is this way acceptable?
Thanks!

> Jan
Jan Beulich Jan. 3, 2017, 9:12 a.m. UTC | #5
>>> On 03.01.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
> On 17-01-03 01:00:37, Jan Beulich wrote:
>> >>> On 26.12.16 at 07:56, <yi.y.sun@linux.intel.com> wrote:
>> > On 16-12-22 09:13:43, Jan Beulich wrote:
>> >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
>> >> > +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 {
>> >> > +    /*
>> >> > +     * 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_node *feat,
>> >> > +                         struct psr_socket_info *info);
>> >> > +};
>> >> 
>> >> What is the reason to have a separate structure for this, when you
>> >> don't store a pointer in struct feat_node? If this was inlined there,
>> >> the odd forward declaration of struct feat_node wouldn't be needed
>> >> either. (The same question may apply to struct feat_hw_info.) 
>> >> 
>> > I just want to make codes be clear. If you prefer inline declaration, I 
>> > think I
>> > should change it as below, right?
>> > 
>> > struct feat_node {
>> > ......
>> >     struct feat_ops {
>> >         ......
>> >     } ops;
>> >     struct feat_hw_info {
>> >         ......
>> >     } info;
>> > ......
>> > };
>> 
>> Well, not exactly: The struct <tag> { ... } <name>; wrappers
>> are unnecessary then too. With them kept there indeed would be
>> no big difference between both variants.
>> 
> To facilitate the callback functions assignment for a feature, I defined
> feature specific callback function ops like below.
> 
> struct feat_ops l3_cat_ops = {
>     .init_feature = l3_cat_init_feature,
>     .get_max_cos_max = l3_cat_get_max_cos_max,
>     ......
> };
> 
> And directly assign it to global feature node in initialization process like
> below.
> 
> static void cpu_init_work(void)
> {
> ......
>             feat_tmp = feat_l3_cat;
>             feat_l3_cat = NULL;
>             feat_tmp->ops = l3_cat_ops;
> ......
> }
> 
> I think this can make codes be clear. How do you think? Is this way 
> acceptable?

Yes.

Jan
Yi Sun Jan. 3, 2017, 10:28 a.m. UTC | #6
On 17-01-03 02:12:53, Jan Beulich wrote:
> >>> On 03.01.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
> > On 17-01-03 01:00:37, Jan Beulich wrote:
> >> >>> On 26.12.16 at 07:56, <yi.y.sun@linux.intel.com> wrote:
> >> > On 16-12-22 09:13:43, Jan Beulich wrote:
> >> >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
> >> >> > +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 {
> >> >> > +    /*
> >> >> > +     * 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_node *feat,
> >> >> > +                         struct psr_socket_info *info);
> >> >> > +};
> >> >> 
> >> >> What is the reason to have a separate structure for this, when you
> >> >> don't store a pointer in struct feat_node? If this was inlined there,
> >> >> the odd forward declaration of struct feat_node wouldn't be needed
> >> >> either. (The same question may apply to struct feat_hw_info.) 
> >> >> 
> >> > I just want to make codes be clear. If you prefer inline declaration, I 
> >> > think I
> >> > should change it as below, right?
> >> > 
> >> > struct feat_node {
> >> > ......
> >> >     struct feat_ops {
> >> >         ......
> >> >     } ops;
> >> >     struct feat_hw_info {
> >> >         ......
> >> >     } info;
> >> > ......
> >> > };
> >> 
> >> Well, not exactly: The struct <tag> { ... } <name>; wrappers
> >> are unnecessary then too. With them kept there indeed would be
> >> no big difference between both variants.
> >> 
> > To facilitate the callback functions assignment for a feature, I defined
> > feature specific callback function ops like below.
> > 
> > struct feat_ops l3_cat_ops = {
> >     .init_feature = l3_cat_init_feature,
> >     .get_max_cos_max = l3_cat_get_max_cos_max,
> >     ......
> > };
> > 
> > And directly assign it to global feature node in initialization process like
> > below.
> > 
> > static void cpu_init_work(void)
> > {
> > ......
> >             feat_tmp = feat_l3_cat;
> >             feat_l3_cat = NULL;
> >             feat_tmp->ops = l3_cat_ops;
> > ......
> > }
> > 
> > I think this can make codes be clear. How do you think? Is this way 
> > acceptable?
> 
> Yes.
> 
Thanks a lot! Do you have any comment to other patches?

> Jan
Jan Beulich Jan. 3, 2017, 11:23 a.m. UTC | #7
>>> On 03.01.17 at 11:28, <yi.y.sun@linux.intel.com> wrote:
> Do you have any comment to other patches?

Well, I'll get to them as time permits.

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 96a8589..49a4598 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -17,12 +17,123 @@ 
 #include <xen/cpu.h>
 #include <xen/err.h>
 #include <xen/sched.h>
+#include <xen/list.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 17.17.3.3 'Cache Allocation Technology: Cache Mask Configuration',
+ * the MSRs range from 0C90H through 0D0FH (inclusive), enables support for
+ * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127].
+ *
+ * The MSRs range 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
+
+/*
+ * 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.
+ * nr_feat   - Record how many features enabled.
+ * feat_list - A list 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 {
+    /*
+     * bit 0:   L3 CAT
+     * bit 1:   L3 CDP
+     * bit 2:   L2 CAT
+     */
+    unsigned int feat_mask;
+    unsigned int nr_feat;
+    struct list_head feat_list;
+    unsigned int cos_ref[MAX_COS_REG_CNT];
+    spinlock_t ref_lock;
+};
+
+enum psr_feat_type {
+    PSR_SOCKET_L3_CAT = 0,
+    PSR_SOCKET_L3_CDP,
+    PSR_SOCKET_L2_CAT,
+};
+
+/* CAT/CDP HW info data structure. */
+struct psr_cat_hw_info {
+    unsigned int cbm_len;
+    unsigned int cos_max;
+};
+
+/* Encapsulate feature specific HW info here. */
+struct feat_hw_info {
+    union {
+        struct psr_cat_hw_info l3_cat_info;
+    };
+};
+
+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 {
+    /*
+     * 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_node *feat,
+                         struct psr_socket_info *info);
+};
+
+
+/*
+ * This structure represents one feature.
+ * feature     - Which feature it is.
+ * 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).
+ * list        - Feature list.
+ */
+struct feat_node {
+    enum psr_feat_type feature;
+    struct feat_ops ops;
+    struct feat_hw_info info;
+    uint64_t cos_reg_val[MAX_COS_REG_CNT];
+    struct list_head list;
+};
+
 struct psr_assoc {
     uint64_t val;
     uint64_t cos_mask;