Re: [Xen-devel] [PATCH v1 04/13] x86: implement data structure and CPU init flow for MBA

2017-08-17 Thread Wei Liu 
On Wed, Aug 16, 2017 at 03:18:13PM +0800, Yi Sun wrote:
> On 17-08-15 11:50:15, Wei Liu wrote:
> > >  struct feat_node {
> > > -/* cos_max and cbm_len are common values for all features so far. */
> > > +/* cos_max is common values for all features so far. */
> > >  unsigned int cos_max;
> > > -unsigned int cbm_len;
> > > +
> > > +/* Feature specific HW info. */
> > > +union {
> > > +struct {
> > > +/* The length of CBM got through CPUID. */
> > > +unsigned int cbm_len;
> > > +} cat_info;
> > > +
> > > +struct {
> > > +/* The max throttling value got through CPUID. */
> > > +unsigned int thrtl_max;
> > > +unsigned int linear;
> > > +} mba_info;
> > > +};
> > > +
> > 
> > I suggest you add a tag to specify which struct is in effect in the
> > union and ASSERT accordingly in their respective type specific
> > functions.
> 
> Before using struct in this union at all places, there is check to make sure 
> the
> correct feature node is used. So, is it necessary to add an additional flag to
> check? Thanks!

OK, in that case I think it is fine to not add a tag.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1 04/13] x86: implement data structure and CPU init flow for MBA

2017-08-16 Thread Yi Sun 
On 17-08-15 11:50:15, Wei Liu wrote:
> On Wed, Aug 09, 2017 at 03:41:43PM +0800, Yi Sun wrote:
> > -#define PSR_CAT(1<<1)
> > -#define PSR_CDP(1<<2)
> > +#define PSR_CMT(1u << 0)
> > +#define PSR_CAT(1u << 1)
> > +#define PSR_CDP(1u << 2)
> > +#define PSR_MBA(1u << 3)
> 
> I would split this part out to a separate patch so that it can be
> applied immediately.
> 
A fix patch has been sent out.

> >  struct feat_node {
> > -/* cos_max and cbm_len are common values for all features so far. */
> > +/* cos_max is common values for all features so far. */
> >  unsigned int cos_max;
> > -unsigned int cbm_len;
> > +
> > +/* Feature specific HW info. */
> > +union {
> > +struct {
> > +/* The length of CBM got through CPUID. */
> > +unsigned int cbm_len;
> > +} cat_info;
> > +
> > +struct {
> > +/* The max throttling value got through CPUID. */
> > +unsigned int thrtl_max;
> > +unsigned int linear;
> > +} mba_info;
> > +};
> > +
> 
> I suggest you add a tag to specify which struct is in effect in the
> union and ASSERT accordingly in their respective type specific
> functions.

Before using struct in this union at all places, there is check to make sure the
correct feature node is used. So, is it necessary to add an additional flag to
check? Thanks!

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1 04/13] x86: implement data structure and CPU init flow for MBA

2017-08-15 Thread Chao Peng 
On Wed, 2017-08-09 at 15:41 +0800, Yi Sun wrote:
> This patch implements main data structures of MBA.
> 
> Like CAT features, MBA HW info has cos_max which means the max cos
> registers number, and thrtl_max which means the max throttle value

Similarly, there is no existence of 'cos register', 'cos number' is even
better or anything else.

> (delay value). It also has a flag to represent if the throttle
> value is linear or not.
> 
> One COS register of MBA stores a throttle value for one or more

Ditto.
                                                         ^s
...

> -/* CAT common functions implementation. */
> +/* Implementation of allocation features' functions. */
>  static int cat_init_feature(const struct cpuid_leaf *regs,
>  struct feat_node *feat,
>  struct psr_socket_info *info,
> @@ -289,7 +310,6 @@ static int cat_init_feature(const struct
> cpuid_leaf *regs,
>  if ( !regs->a || !regs->d )
>  return -ENOENT;
>  
> -feat->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;

Does this have to be moved?

...

> +
> +feat->cos_reg_val[0] = 0;
> +wrmsrl(MSR_IA32_PSR_MBA_MASK(0), 0);

It's better to have a comment here to explain what the defaul value '0'
stands for in both linear mode and non-linear mode.

Chao

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1 04/13] x86: implement data structure and CPU init flow for MBA

2017-08-15 Thread Wei Liu 
On Wed, Aug 09, 2017 at 03:41:43PM +0800, Yi Sun wrote:
> This patch implements main data structures of MBA.
> 
> Like CAT features, MBA HW info has cos_max which means the max cos
> registers number, and thrtl_max which means the max throttle value
> (delay value). It also has a flag to represent if the throttle
> value is linear or not.
> 
> One COS register of MBA stores a throttle value for one or more
> domains. The throttle value means the transaction time between L2
> cache and next level memory to be delayed.
> 
> This patch also implements init flow for MBA and register stub
> callback functions.
> 
> Signed-off-by: Yi Sun 
> ---
> v1:
> - rebase codes onto L2 CAT v15.
> - use '(1u << X)'.
>   (suggested by Wei Liu)
> - move comment to appropriate place.
>   (suggested by Chao Peng)
> - implement 'mba_init_feature' and keep 'cat_init_feature'.
>   (suggested by Chao Peng)
> - keep 'regs.b' into a local variable to avoid reading CPUID every time.
>   (suggested by Chao Peng)
> ---
>  xen/arch/x86/psr.c  | 144 
> ++--
>  xen/include/asm-x86/msr-index.h |   1 +
>  xen/include/asm-x86/psr.h   |   2 +
>  3 files changed, 126 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 5ec00a9..d94a5b1 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -27,13 +27,16 @@
>   * - CMT Cache Monitoring Technology
>   * - COS/CLOSClass of Service. Also mean COS registers.
>   * - COS_MAX Max number of COS for the feature (minus 1)
> + * - MBA Memory Bandwidth Allocation
>   * - MSRsMachine Specific Registers
>   * - PSR Intel Platform Shared Resource
> + * - THRTL_MAX   Max throttle value (delay value) of MBA
>   */
>  
> -#define PSR_CMT(1<<0)
> -#define PSR_CAT(1<<1)
> -#define PSR_CDP(1<<2)
> +#define PSR_CMT(1u << 0)
> +#define PSR_CAT(1u << 1)
> +#define PSR_CDP(1u << 2)
> +#define PSR_MBA(1u << 3)

I would split this part out to a separate patch so that it can be
applied immediately.

>  
>  #define CAT_CBM_LEN_MASK 0x1f
>  #define CAT_COS_MAX_MASK 0x
> @@ -60,10 +63,14 @@
>   */
>  #define MAX_COS_NUM 2
>  
> +#define MBA_LINEAR (1u << 2)
> +#define MBA_THRTL_MAX_MASK 0xfff
> +
>  enum psr_feat_type {
>  FEAT_TYPE_L3_CAT,
>  FEAT_TYPE_L3_CDP,
>  FEAT_TYPE_L2_CAT,
> +FEAT_TYPE_MBA,
>  FEAT_TYPE_NUM,
>  FEAT_TYPE_UNKNOWN,
>  };
> @@ -71,7 +78,6 @@ enum psr_feat_type {
>  /*
>   * This structure represents one feature.
>   * cos_max - The max COS registers number got through CPUID.
> - * cbm_len - The length of CBM got through CPUID.
>   * 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.
> @@ -80,9 +86,23 @@ enum psr_feat_type {
>   *   cos_reg_val[1] (Code).
>   */
>  struct feat_node {
> -/* cos_max and cbm_len are common values for all features so far. */
> +/* cos_max is common values for all features so far. */
>  unsigned int cos_max;
> -unsigned int cbm_len;
> +
> +/* Feature specific HW info. */
> +union {
> +struct {
> +/* The length of CBM got through CPUID. */
> +unsigned int cbm_len;
> +} cat_info;
> +
> +struct {
> +/* The max throttling value got through CPUID. */
> +unsigned int thrtl_max;
> +unsigned int linear;
> +} mba_info;
> +};
> +

I suggest you add a tag to specify which struct is in effect in the
union and ASSERT accordingly in their respective type specific
functions.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v1 04/13] x86: implement data structure and CPU init flow for MBA

2017-08-09 Thread Yi Sun 
This patch implements main data structures of MBA.

Like CAT features, MBA HW info has cos_max which means the max cos
registers number, and thrtl_max which means the max throttle value
(delay value). It also has a flag to represent if the throttle
value is linear or not.

One COS register of MBA stores a throttle value for one or more
domains. The throttle value means the transaction time between L2
cache and next level memory to be delayed.

This patch also implements init flow for MBA and register stub
callback functions.

Signed-off-by: Yi Sun 
---
v1:
- rebase codes onto L2 CAT v15.
- use '(1u << X)'.
  (suggested by Wei Liu)
- move comment to appropriate place.
  (suggested by Chao Peng)
- implement 'mba_init_feature' and keep 'cat_init_feature'.
  (suggested by Chao Peng)
- keep 'regs.b' into a local variable to avoid reading CPUID every time.
  (suggested by Chao Peng)
---
 xen/arch/x86/psr.c  | 144 ++--
 xen/include/asm-x86/msr-index.h |   1 +
 xen/include/asm-x86/psr.h   |   2 +
 3 files changed, 126 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 5ec00a9..d94a5b1 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -27,13 +27,16 @@
  * - CMT Cache Monitoring Technology
  * - COS/CLOSClass of Service. Also mean COS registers.
  * - COS_MAX Max number of COS for the feature (minus 1)
+ * - MBA Memory Bandwidth Allocation
  * - MSRsMachine Specific Registers
  * - PSR Intel Platform Shared Resource
+ * - THRTL_MAX   Max throttle value (delay value) of MBA
  */
 
-#define PSR_CMT(1<<0)
-#define PSR_CAT(1<<1)
-#define PSR_CDP(1<<2)
+#define PSR_CMT(1u << 0)
+#define PSR_CAT(1u << 1)
+#define PSR_CDP(1u << 2)
+#define PSR_MBA(1u << 3)
 
 #define CAT_CBM_LEN_MASK 0x1f
 #define CAT_COS_MAX_MASK 0x
@@ -60,10 +63,14 @@
  */
 #define MAX_COS_NUM 2
 
+#define MBA_LINEAR (1u << 2)
+#define MBA_THRTL_MAX_MASK 0xfff
+
 enum psr_feat_type {
 FEAT_TYPE_L3_CAT,
 FEAT_TYPE_L3_CDP,
 FEAT_TYPE_L2_CAT,
+FEAT_TYPE_MBA,
 FEAT_TYPE_NUM,
 FEAT_TYPE_UNKNOWN,
 };
@@ -71,7 +78,6 @@ enum psr_feat_type {
 /*
  * This structure represents one feature.
  * cos_max - The max COS registers number got through CPUID.
- * cbm_len - The length of CBM got through CPUID.
  * 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.
@@ -80,9 +86,23 @@ enum psr_feat_type {
  *   cos_reg_val[1] (Code).
  */
 struct feat_node {
-/* cos_max and cbm_len are common values for all features so far. */
+/* cos_max is common values for all features so far. */
 unsigned int cos_max;
-unsigned int cbm_len;
+
+/* Feature specific HW info. */
+union {
+struct {
+/* The length of CBM got through CPUID. */
+unsigned int cbm_len;
+} cat_info;
+
+struct {
+/* The max throttling value got through CPUID. */
+unsigned int thrtl_max;
+unsigned int linear;
+} mba_info;
+};
+
 uint32_t cos_reg_val[MAX_COS_REG_CNT];
 };
 
@@ -161,6 +181,7 @@ static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
  */
 static struct feat_node *feat_l3;
 static struct feat_node *feat_l2_cat;
+static struct feat_node *feat_mba;
 
 /* Common functions */
 #define cat_default_val(len) (0x >> (32 - (len)))
@@ -273,7 +294,7 @@ static bool psr_check_cbm(unsigned int cbm_len, unsigned 
long cbm)
 return true;
 }
 
-/* CAT common functions implementation. */
+/* Implementation of allocation features' functions. */
 static int cat_init_feature(const struct cpuid_leaf *regs,
 struct feat_node *feat,
 struct psr_socket_info *info,
@@ -289,7 +310,6 @@ static int cat_init_feature(const struct cpuid_leaf *regs,
 if ( !regs->a || !regs->d )
 return -ENOENT;
 
-feat->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;
 feat->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);
 
 switch ( type )
@@ -299,13 +319,15 @@ static int cat_init_feature(const struct cpuid_leaf *regs,
 if ( feat->cos_max < 1 )
 return -ENOENT;
 
+feat->cat_info.cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;
+
 /* We reserve cos=0 as default cbm (all bits within cbm_len are 1). */
-feat->cos_reg_val[0] = cat_default_val(feat->cbm_len);
+feat->cos_reg_val[0] = cat_default_val(feat->cat_info.cbm_len);
 
 wrmsrl((type == FEAT_TYPE_L3_CAT ?
 MSR_IA32_PSR_L3_MASK(0) :
 MSR_IA32_PSR_L2_MASK(0)),
-   cat_default_val(feat->cbm_len));
+