| Message ID | 1484805686-7249-4-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
.snip.. > 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 spec and codes. Thanks for the description. Only had one comment about it: 'spec and codes'? Do you mean to specification. But what codes? This one? No need to mention that, that is kind of implicit. > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > v5: > - explain CDP more in commit message. > - remove exact SDM chapter number but only keep title. > - remove init_feature from callback function ops structure. > --- > xen/arch/x86/psr.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 104 insertions(+) > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index 96a8589..f7ff3fc 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -17,12 +17,116 @@ > #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 chapter 'Cache Allocation Technology: Cache Mask Configuration', > + * the MSRs range from 0C90H through 0D0FH (inclusive), enables support for s/enables/enable/ > + * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127]. > + * > + * The MSRs range from 0D10H through 0D4FH (inclusive), enables support for s/enables/enable/ > + * 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 Why not an enum? I am going to assume it is due to you programming this in the MSRs. If so, I would recommend you have #define instead of a comment. #define L3_CAT (1U<<0) #define L3_CDP (1U<<1) . and so on.. > + */ > + 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, Is there particular mapping between these and the feat_mask bit mask values? It kind of begs to be combined (unless you really need the 'feat_mask' to be of specific order - in which case make sure you have BUILD_BUG_ON to make sure nobody moves the #define values around - or put a comment saying that you need the specific order of bits). > +}; > + > +/* 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; > -- > 1.9.1 >
>>> On 30.01.17 at 23:20, <konrad.wilk@oracle.com> wrote: >> --- a/xen/arch/x86/psr.c >> +++ b/xen/arch/x86/psr.c >> @@ -17,12 +17,116 @@ >> #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 chapter 'Cache Allocation Technology: Cache Mask Configuration', >> + * the MSRs range from 0C90H through 0D0FH (inclusive), enables support for > > s/enables/enable/ >> + * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127]. >> + * >> + * The MSRs range from 0D10H through 0D4FH (inclusive), enables support for > > s/enables/enable/ For both of them - why? Both talk about a (single) range. Jan
On Tue, Jan 31, 2017 at 03:10:14AM -0700, Jan Beulich wrote: > >>> On 30.01.17 at 23:20, <konrad.wilk@oracle.com> wrote: > >> --- a/xen/arch/x86/psr.c > >> +++ b/xen/arch/x86/psr.c > >> @@ -17,12 +17,116 @@ > >> #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 chapter 'Cache Allocation Technology: Cache Mask Configuration', > >> + * the MSRs range from 0C90H through 0D0FH (inclusive), enables support for > > > > s/enables/enable/ > >> + * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127]. > >> + * > >> + * The MSRs range from 0D10H through 0D4FH (inclusive), enables support for > > > > s/enables/enable/ > > For both of them - why? Both talk about a (single) range. The 's' on the verb makes it singular. The MSRs is plural. You don't do plural verb and plural subject - so the 's' has to be either on the MSRs (and 'enable'), or you move the s from MSRs ('MSR' and 'enables').
>>> On 31.01.17 at 15:12, <konrad.wilk@oracle.com> wrote: > On Tue, Jan 31, 2017 at 03:10:14AM -0700, Jan Beulich wrote: >> >>> On 30.01.17 at 23:20, <konrad.wilk@oracle.com> wrote: >> >> --- a/xen/arch/x86/psr.c >> >> +++ b/xen/arch/x86/psr.c >> >> @@ -17,12 +17,116 @@ >> >> #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 chapter 'Cache Allocation Technology: Cache Mask Configuration', >> >> + * the MSRs range from 0C90H through 0D0FH (inclusive), enables support for >> > >> > s/enables/enable/ >> >> + * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127]. >> >> + * >> >> + * The MSRs range from 0D10H through 0D4FH (inclusive), enables support for >> > >> > s/enables/enable/ >> >> For both of them - why? Both talk about a (single) range. > > The 's' on the verb makes it singular. The MSRs is plural. > > You don't do plural verb and plural subject - so the 's' has to be either > on the MSRs (and 'enable'), or you move the s from MSRs ('MSR' and > 'enables'). But that's my point, just that the subject is "range" here. Otoh it could also be "The MSRs ranging from ... enable support". Jan
On Tue, Jan 31, 2017 at 08:07:17AM -0700, Jan Beulich wrote: > >>> On 31.01.17 at 15:12, <konrad.wilk@oracle.com> wrote: > > On Tue, Jan 31, 2017 at 03:10:14AM -0700, Jan Beulich wrote: > >> >>> On 30.01.17 at 23:20, <konrad.wilk@oracle.com> wrote: > >> >> --- a/xen/arch/x86/psr.c > >> >> +++ b/xen/arch/x86/psr.c > >> >> @@ -17,12 +17,116 @@ > >> >> #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 chapter 'Cache Allocation Technology: Cache Mask Configuration', > >> >> + * the MSRs range from 0C90H through 0D0FH (inclusive), enables support for > >> > > >> > s/enables/enable/ > >> >> + * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127]. > >> >> + * > >> >> + * The MSRs range from 0D10H through 0D4FH (inclusive), enables support for > >> > > >> > s/enables/enable/ > >> > >> For both of them - why? Both talk about a (single) range. > > > > The 's' on the verb makes it singular. The MSRs is plural. > > > > You don't do plural verb and plural subject - so the 's' has to be either > > on the MSRs (and 'enable'), or you move the s from MSRs ('MSR' and > > 'enables'). > > But that's my point, just that the subject is "range" here. Otoh it > could also be "The MSRs ranging from ... enable support". Right. That would be good too. > > Jan >
On 17-01-30 17:20:39, Konrad Rzeszutek Wilk wrote: > > +struct psr_socket_info { > > + /* > > + * bit 0: L3 CAT > > + * bit 1: L3 CDP > > + * bit 2: L2 CAT > > Why not an enum? I am going to assume it is due to you programming > this in the MSRs. If so, I would recommend you have #define instead > of a comment. > > #define L3_CAT (1U<<0) > #define L3_CDP (1U<<1) > > . and so on.. > > > + */ > > + 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, > > Is there particular mapping between these and the feat_mask bit mask > values? > > It kind of begs to be combined (unless you really need the 'feat_mask' > to be of specific order - in which case make sure you have BUILD_BUG_ON > to make sure nobody moves the #define values around - or put a comment > saying that you need the specific order of bits). > Like what you mentioned in patch 5 review comments, the feat_mask bit mask reuses 'enum psr_feat_type'. I will add comments to describe this. Thanks! BRs, Sun Yi
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 96a8589..f7ff3fc 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -17,12 +17,116 @@ #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 chapter '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 { + /* 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 spec and codes. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v5: - explain CDP more in commit message. - remove exact SDM chapter number but only keep title. - remove init_feature from callback function ops structure. --- xen/arch/x86/psr.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+)