| Message ID | 1487148579-7243-4-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 15, 2017 at 04:49:18PM +0800, Yi Sun 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. 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'. I would recommend that you merge this with a patch that actually makes use of this structures, or else it's hard to review it's usage IMHO. > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/arch/x86/psr.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 107 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index 96a8589..5acd9ca 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -13,16 +13,122 @@ > * 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/list.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 > + > +/* > + * 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 { > + /* > + * 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; > + unsigned int nr_feat; > + struct list_head feat_list; Isn't it a little bit overkill to use a list when there can only be a maximum of 3 features? (and the feat_mask is currently 32bits, so I guess you don't really foresee having more than 32 features). I would suggest using: struct feat_node *features[PSR_SOCKET_MAX_FEAT]; (PSR_SOCKET_MAX_FEAT comes from the expansion of the enum below). Then you can simply use the enum value of each feature as the position of it's corresponding feat_node into the array. > + 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, PSR_SOCKET_MAX_FEAT, > +}; > + > +/* 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; > + }; > +}; Why don't you use an union here directly, instead of encapsulating an union inside of a struct? union feat_hw_info { 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 { > + /* get_cos_max is used to get feature's cos_max. */ > + unsigned int (*get_cos_max)(const struct feat_node *feat); > +}; > + > +/* > + * 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; If you index them in an array with the key being psr_feat_type I don't think you need that field. > + struct feat_ops ops; I would place the function hooks in this struct directly, instead of nesting them inside of another struct. The hooks AFAICT are shared between all the different PSR features. > + struct feat_hw_info info; Same with this, you can place the actual union for storage here directly, instead of nesting it inside of feat_hw_info, so: union { struct psr_cat_hw_info l3_cat_info; } hw_info; Roger.
On 17-02-28 11:58:59, Roger Pau Monn� wrote: > On Wed, Feb 15, 2017 at 04:49:18PM +0800, Yi Sun 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. 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'. > > I would recommend that you merge this with a patch that actually makes use of > this structures, or else it's hard to review it's usage IMHO. > Sorry for this. To make codes less and simpler in a patch, I split this patch out to only show the data structures. I think I can merge it with next patch: [PATCH v8 04/24] x86: refactor psr: implement CPU init and free flow. How do you think? > > +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; > > + unsigned int nr_feat; > > + struct list_head feat_list; > > Isn't it a little bit overkill to use a list when there can only be a maximum > of 3 features? (and the feat_mask is currently 32bits, so I guess you don't > really foresee having more than 32 features). > > I would suggest using: > > struct feat_node *features[PSR_SOCKET_MAX_FEAT]; > > (PSR_SOCKET_MAX_FEAT comes from the expansion of the enum below). Then you can > simply use the enum value of each feature as the position of it's corresponding > feat_node into the array. > I really thought this before. But there may be different features enabled on different sockets. For example, socket 0 enables L3 CAT and L2 CAT but socket 1 only supports L3 CAT. So, the feature array may be different for different sockets. I think it is more complex to use array to handle all things than 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, > PSR_SOCKET_MAX_FEAT, > > +}; > > + > > +/* 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; > > + }; > > +}; > > Why don't you use an union here directly, instead of encapsulating an union > inside of a struct? > > union feat_hw_info { > 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 { > > + /* get_cos_max is used to get feature's cos_max. */ > > + unsigned int (*get_cos_max)(const struct feat_node *feat); > > +}; > > + > > +/* > > + * 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; > > If you index them in an array with the key being psr_feat_type I don't think > you need that field. > I need this to check if input type can match this feature, you can refer get val or set val flow. Thanks! > > + struct feat_ops ops; > > I would place the function hooks in this struct directly, instead of nesting > them inside of another struct. The hooks AFAICT are shared between all the > different PSR features. > Jan suggested this before in v4 patch. We have discussed this and Jan accepts current implementation. The reason is below: "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." > > + struct feat_hw_info info; > > Same with this, you can place the actual union for storage here directly, > instead of nesting it inside of feat_hw_info, so: > > union { > struct psr_cat_hw_info l3_cat_info; > } hw_info; > Thanks for the suggestion! Will do this. > Roger.
>>> On 01.03.17 at 06:10, <yi.y.sun@linux.intel.com> wrote: > On 17-02-28 11:58:59, Roger Pau Monn wrote: >> On Wed, Feb 15, 2017 at 04:49:18PM +0800, Yi Sun wrote: >> > + struct feat_ops ops; >> >> I would place the function hooks in this struct directly, instead of nesting >> them inside of another struct. The hooks AFAICT are shared between all the >> different PSR features. >> > Jan suggested this before in v4 patch. We have discussed this and Jan > accepts > current implementation. The reason is below: I'm pretty sure I didn't (in this specific form). Instead you want this to be a pointer (to a const struct instance), i.e. ... > "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; feat_tmp->ops = &l3_cat_ops; Jan
On 17-03-01 01:17:22, Jan Beulich wrote: > >>> On 01.03.17 at 06:10, <yi.y.sun@linux.intel.com> wrote: > > On 17-02-28 11:58:59, Roger Pau Monn wrote: > >> On Wed, Feb 15, 2017 at 04:49:18PM +0800, Yi Sun wrote: > >> > + struct feat_ops ops; > >> > >> I would place the function hooks in this struct directly, instead of nesting > >> them inside of another struct. The hooks AFAICT are shared between all the > >> different PSR features. > >> > > Jan suggested this before in v4 patch. We have discussed this and Jan > > accepts > > current implementation. The reason is below: > > I'm pretty sure I didn't (in this specific form). Instead you want this > to be a pointer (to a const struct instance), i.e. ... > Sorry for confusion. I think you suggested same thing (maybe I misunderstood?). Quoted from v4: "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.)" So I explained as below and asked if you can accept such implementation. And got your positive ack. Of course, I should declare l3_cat_ops to const. > > "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; > > feat_tmp->ops = &l3_cat_ops; > > Jan
>>> On 01.03.17 at 09:28, <yi.y.sun@linux.intel.com> wrote: > On 17-03-01 01:17:22, Jan Beulich wrote: >> >>> On 01.03.17 at 06:10, <yi.y.sun@linux.intel.com> wrote: >> > On 17-02-28 11:58:59, Roger Pau Monn wrote: >> >> On Wed, Feb 15, 2017 at 04:49:18PM +0800, Yi Sun wrote: >> >> > + struct feat_ops ops; >> >> >> >> I would place the function hooks in this struct directly, instead of > nesting >> >> them inside of another struct. The hooks AFAICT are shared between all the >> >> different PSR features. >> >> >> > Jan suggested this before in v4 patch. We have discussed this and Jan >> > accepts >> > current implementation. The reason is below: >> >> I'm pretty sure I didn't (in this specific form). Instead you want this >> to be a pointer (to a const struct instance), i.e. ... >> > Sorry for confusion. I think you suggested same thing (maybe I > misunderstood?). > > Quoted from v4: > "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.)" > > So I explained as below and asked if you can accept such implementation. And > got your positive ack. Of course, I should declare l3_cat_ops to const. In which case an earlier question to answer is: Why can it not be a pointer that gets stored? Jan
On Wed, Mar 01, 2017 at 01:10:02PM +0800, Yi Sun wrote: > On 17-02-28 11:58:59, Roger Pau Monn� wrote: > > On Wed, Feb 15, 2017 at 04:49:18PM +0800, Yi Sun 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. 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'. > > > > I would recommend that you merge this with a patch that actually makes use of > > this structures, or else it's hard to review it's usage IMHO. > > > Sorry for this. To make codes less and simpler in a patch, I split this patch > out to only show the data structures. I think I can merge it with next patch: > [PATCH v8 04/24] x86: refactor psr: implement CPU init and free flow. > > How do you think? Now that I've looked at the other patches this doesn't seem so abstract to me, I will leave that to the x86 maintainers, and whether they want to join it with some actual code or not. > > > +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; > > > + unsigned int nr_feat; > > > + struct list_head feat_list; > > > > Isn't it a little bit overkill to use a list when there can only be a maximum > > of 3 features? (and the feat_mask is currently 32bits, so I guess you don't > > really foresee having more than 32 features). > > > > I would suggest using: > > > > struct feat_node *features[PSR_SOCKET_MAX_FEAT]; > > > > (PSR_SOCKET_MAX_FEAT comes from the expansion of the enum below). Then you can > > simply use the enum value of each feature as the position of it's corresponding > > feat_node into the array. > > > I really thought this before. But there may be different features enabled on > different sockets. For example, socket 0 enables L3 CAT and L2 CAT but socket 1 > only supports L3 CAT. So, the feature array may be different for different > sockets. I think it is more complex to use array to handle all things than list. Different sockets with different features enabled should still be fine. Each socket has a feat_mask, and you can use that to know whether a certain feature is enabled or not. What I was proposing is to make feat_node an array of pointers, so if a feature is not supported that specific pointer would be NULL (ie: if feat_mask & PSR_SOCKET_L3_CDP == 0, features[PSR_SOCKET_L3_CDP] == NULL). And then you can avoid all the list traversing code. > > > + 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, > > PSR_SOCKET_MAX_FEAT, > > > +}; > > > + > > > +/* 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; > > > + }; > > > +}; > > > > Why don't you use an union here directly, instead of encapsulating an union > > inside of a struct? > > > > union feat_hw_info { > > 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 { > > > + /* get_cos_max is used to get feature's cos_max. */ > > > + unsigned int (*get_cos_max)(const struct feat_node *feat); > > > +}; > > > + > > > +/* > > > + * 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; > > > > If you index them in an array with the key being psr_feat_type I don't think > > you need that field. > > > I need this to check if input type can match this feature, you can refer get > val or set val flow. Thanks! If you move to the array of pointers proposed above you no longer need this since you can just do features[PSR_SOCKET_L3_CAT] and get the pointer to the feat_node struct, the index into the array is going to be the feature type already. > > > + struct feat_ops ops; > > > > I would place the function hooks in this struct directly, instead of nesting > > them inside of another struct. The hooks AFAICT are shared between all the > > different PSR features. > > > Jan suggested this before in v4 patch. We have discussed this and Jan accepts > current implementation. The reason is below: > > "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, > ...... > }; This should be constified as Jan already pointed out. I see that what I said before doesn't make sense, since you have both constant functions pointers (that can be shared across nodes), plus local node storage in this structure. Roger.
>>> On 01.03.17 at 09:49, <roger.pau@citrix.com> wrote: > What I was proposing is to make feat_node an array of pointers, so if a feature > is not supported that specific pointer would be NULL (ie: if feat_mask & > PSR_SOCKET_L3_CDP == 0, features[PSR_SOCKET_L3_CDP] == NULL). And then you can > avoid all the list traversing code. In which case the feature flags would look to become redundant too. Jan
On Wed, Mar 01, 2017 at 01:54:00AM -0700, Jan Beulich wrote: > >>> On 01.03.17 at 09:49, <roger.pau@citrix.com> wrote: > > What I was proposing is to make feat_node an array of pointers, so if a feature > > is not supported that specific pointer would be NULL (ie: if feat_mask & > > PSR_SOCKET_L3_CDP == 0, features[PSR_SOCKET_L3_CDP] == NULL). And then you can > > avoid all the list traversing code. > > In which case the feature flags would look to become redundant too. I guess so, I'm just not sure if you can have a feature available (so feat_mask bit set) but not enabled (so features[...] == NULL). Not sure if that makes sense or is desirable at all. Roger.
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 96a8589..5acd9ca 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -13,16 +13,122 @@ * 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/list.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 + +/* + * 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 { + /* + * 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; + 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 { + /* get_cos_max is used to get feature's cos_max. */ + unsigned int (*get_cos_max)(const struct feat_node *feat); +}; + +/* + * 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;
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. 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 <yi.y.sun@linux.intel.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/arch/x86/psr.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 107 insertions(+), 1 deletion(-)