[v9,07/25] x86: refactor psr: L3 CAT: implement get hw info flow.

Commit Message

Yi Sun March 16, 2017, 11:07 a.m. UTC

This patch implements get HW info flow including L3 CAT callback
function.

It also changes sysctl interface to make it more general.

With this patch, 'psr-hwinfo' can work for L3 CAT.

Signed-off-by: Yi Sun <[e-mail]>
Reviewed-by: Konrad Rzeszutek Wilk <[e-mail]>
Reviewed-by: Wei Liu <[e-mail]>
---
v9:
    - replace feature list handling to feature array handling.
      (suggested by Roger Pau)
    - define 'PSR_INFO_SIZE'.
      (suggested by Roger Pau)
    - fix coding style issue.
      (suggested by Roger Pau and Jan Beulich)
    - use 'ARRAY_SIZE'.
      (suggested by Roger Pau)
    - rename 'l3_cat_get_feat_info' to 'cat_get_feat_info' to make it a common
      function for both L3/L2 CAT.
      (suggested by Roger Pau)
    - move constant to the right of comparison.
      (suggested by Wei Liu)
    - remove wrong comment.
      (suggested by Jan Beulich)
    - rename macros used by psr_get_info to make them meaningful.
      (suggested by Jan Beulich)
    - remove assignment for 'PSR_SOCKET_UNKNOWN'.
      (suggested by Jan Beulich)
    - retain blank line after 'case XEN_SYSCTL_PSR_CAT_get_l3_info'.
      (suggested by Jan Beulich)
    - modify patch title to indicate 'L3 CAT'.
      (suggested by Jan Beulich)
    - move common data check into common function.
      (suggested by Jan Beulich)
v6:
    - fix coding style issue.
      (suggested by Konrad Rzeszutek Wilk)
    - define 'PSR_SOCKET_UNKNOWN' in 'psr_feat_type'.
      (suggested by Konrad Rzeszutek Wilk)
    - change '-ENOTSOCK' to 'ERANGE'.
      (suggested by Konrad Rzeszutek Wilk)
    - modify position of macros to remove odd spacing in psr.h.
      (suggested by Konrad Rzeszutek Wilk)
v5:
    - change 'dat[]' to 'data[]'.
      (suggested by Jan Beulich)
    - modify parameter type to avoid fixed width type when there is no such
      intention.
      (suggested by Jan Beulich)
    - use 'const' when it is possible.
      (suggested by Jan Beulich)
    - check feature type outside callback function.
      (suggested by Jan Beulich)
    - modify macros names to add prefix 'PSR_' and change 'CDP_FLAG' to
      'PSR_FLAG'.
      (suggested by Jan Beulich)
v4:
    - create this patch to make codes easier to understand.
      (suggested by Jan Beulich)
---
 xen/arch/x86/psr.c        | 74 +++++++++++++++++++++++++++++++++++++++++++++--
 xen/arch/x86/sysctl.c     | 14 ++++++---
 xen/include/asm-x86/psr.h | 17 +++++++----
 3 files changed, 93 insertions(+), 12 deletions(-)

Comments

Jan Beulich March 27, 2017, 9:07 a.m. UTC | #1

>>> On 16.03.17 at 12:07, <[e-mail]> wrote:
> +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type)
> +{
> +    enum psr_feat_type feat_type;
> +
> +    switch ( type )
> +    {
> +    case PSR_CBM_TYPE_L3:
> +        feat_type = PSR_SOCKET_L3_CAT;
> +        break;
> +    default:
> +        feat_type = PSR_SOCKET_UNKNOWN;
> +        break;

Is this actually reachable, if there are no bugs in the code? If not,
you will want to add ASSERT_UNREACHABLE().

> +int psr_get_info(unsigned int socket, enum cbm_type type,
> +                 uint32_t data[], unsigned int array_len)
> +{
> +    const struct psr_socket_info *info = get_socket_info(socket);
> +    const struct feat_node *feat;
> +    enum psr_feat_type feat_type;
> +
> +    if ( IS_ERR(info) )
> +        return PTR_ERR(info);
> +
> +    if ( !data )
> +        return -EINVAL;
> +
> +    feat_type = psr_cbm_type_to_feat_type(type);
> +    feat = info->features[feat_type];

You can't blindly use the return value here as array index, as (at
least in theory, see above) the function may return
PSR_SOCKET_UNKNOWN. IOW you need to check against
ARRAY_SIZE(info->features) first.

> +/* Used by psr_get_info() */
> +#define PSR_INFO_IDX_CBM_LEN            0
> +#define PSR_INFO_IDX_COS_MAX            1
> +#define PSR_INFO_IDX_FLAG               2
> +#define PSR_INFO_CAT_SIZE               3

So I need some explanation on the naming here: Are the first three
CAT-independent, but the last one is CAT-dependent? It doesn't
look so (or else it would be odd coincidence for the last one to be
one higher than the biggest of the _IDX ones). And if they're all
in either of the two categories, their names should reflect that
(i.e. either all have _CAT in their names, or none does).

> +
> +
>  struct psr_cmt_l3 {

No double blank lines please (and just in case: comments of this kind
apply to the entire series, i.e. you shouldn't expect them to be
repeated in other patches).

Jan

Yi Sun March 27, 2017, 12:24 p.m. UTC | #2

On 17-03-27 03:07:37, Jan Beulich wrote:
> >>> On 16.03.17 at 12:07, <[e-mail]> wrote:
> > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type)
> > +{
> > +    enum psr_feat_type feat_type;
> > +
> > +    switch ( type )
> > +    {
> > +    case PSR_CBM_TYPE_L3:
> > +        feat_type = PSR_SOCKET_L3_CAT;
> > +        break;
> > +    default:
> > +        feat_type = PSR_SOCKET_UNKNOWN;
> > +        break;
> 
> Is this actually reachable, if there are no bugs in the code? If not,
> you will want to add ASSERT_UNREACHABLE().
> 
If there is no bug, we should not reach here. Will use ASSERT_UNREACHABLE().

> > +int psr_get_info(unsigned int socket, enum cbm_type type,
> > +                 uint32_t data[], unsigned int array_len)
> > +{
> > +    const struct psr_socket_info *info = get_socket_info(socket);
> > +    const struct feat_node *feat;
> > +    enum psr_feat_type feat_type;
> > +
> > +    if ( IS_ERR(info) )
> > +        return PTR_ERR(info);
> > +
> > +    if ( !data )
> > +        return -EINVAL;
> > +
> > +    feat_type = psr_cbm_type_to_feat_type(type);
> > +    feat = info->features[feat_type];
> 
> You can't blindly use the return value here as array index, as (at
> least in theory, see above) the function may return
> PSR_SOCKET_UNKNOWN. IOW you need to check against
> ARRAY_SIZE(info->features) first.
> 
If I use 'ASSERT_UNREACHABLE()' above, I don't need check against
ARRAY_SIZE(info->features) here, right?

> > +/* Used by psr_get_info() */
> > +#define PSR_INFO_IDX_CBM_LEN            0
CAT/CDP specific.

> > +#define PSR_INFO_IDX_COS_MAX            1
Common so far.

> > +#define PSR_INFO_IDX_FLAG               2
CAT/CDP specific so far.

> > +#define PSR_INFO_CAT_SIZE               3
Array size which can be used for all features but may not be appropriate for
future features. So I defined it as CAT specific.

> 
> So I need some explanation on the naming here: Are the first three
> CAT-independent, but the last one is CAT-dependent? It doesn't
> look so (or else it would be odd coincidence for the last one to be
> one higher than the biggest of the _IDX ones). And if they're all
> in either of the two categories, their names should reflect that
> (i.e. either all have _CAT in their names, or none does).
> 
Please check above comments. Maybe below definitions are better?
PSR_INFO_IDX_CAT_CBM_LEN
PSR_INFO_IDX_COS_MAX
PSR_INFO_IDX_CAT_FLAG
PSR_INFO_ARRAY_SIZE

> > +
> > +
> >  struct psr_cmt_l3 {
> 
> No double blank lines please (and just in case: comments of this kind
> apply to the entire series, i.e. you shouldn't expect them to be
> repeated in other patches).
> 
Sorry for this. Do we have code style check tool in xen?

> Jan

Jan Beulich March 27, 2017, 12:51 p.m. UTC | #3

>>> On 27.03.17 at 14:24, <[e-mail]> wrote:
> On 17-03-27 03:07:37, Jan Beulich wrote:
>> >>> On 16.03.17 at 12:07, <[e-mail]> wrote:
>> > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type)
>> > +{
>> > +    enum psr_feat_type feat_type;
>> > +
>> > +    switch ( type )
>> > +    {
>> > +    case PSR_CBM_TYPE_L3:
>> > +        feat_type = PSR_SOCKET_L3_CAT;
>> > +        break;
>> > +    default:
>> > +        feat_type = PSR_SOCKET_UNKNOWN;
>> > +        break;
>> 
>> Is this actually reachable, if there are no bugs in the code? If not,
>> you will want to add ASSERT_UNREACHABLE().
>> 
> If there is no bug, we should not reach here. Will use ASSERT_UNREACHABLE().
> 
>> > +int psr_get_info(unsigned int socket, enum cbm_type type,
>> > +                 uint32_t data[], unsigned int array_len)
>> > +{
>> > +    const struct psr_socket_info *info = get_socket_info(socket);
>> > +    const struct feat_node *feat;
>> > +    enum psr_feat_type feat_type;
>> > +
>> > +    if ( IS_ERR(info) )
>> > +        return PTR_ERR(info);
>> > +
>> > +    if ( !data )
>> > +        return -EINVAL;
>> > +
>> > +    feat_type = psr_cbm_type_to_feat_type(type);
>> > +    feat = info->features[feat_type];
>> 
>> You can't blindly use the return value here as array index, as (at
>> least in theory, see above) the function may return
>> PSR_SOCKET_UNKNOWN. IOW you need to check against
>> ARRAY_SIZE(info->features) first.
>> 
> If I use 'ASSERT_UNREACHABLE()' above, I don't need check against
> ARRAY_SIZE(info->features) here, right?

That's a slightly difficult call: The assertion will expand to nothing
in production builds, so to be on the safe side I think you better
check function return values _everywhere_.

>> > +/* Used by psr_get_info() */
>> > +#define PSR_INFO_IDX_CBM_LEN            0
> CAT/CDP specific.
> 
>> > +#define PSR_INFO_IDX_COS_MAX            1
> Common so far.
> 
>> > +#define PSR_INFO_IDX_FLAG               2
> CAT/CDP specific so far.
> 
>> > +#define PSR_INFO_CAT_SIZE               3
> Array size which can be used for all features but may not be appropriate for
> future features. So I defined it as CAT specific.
> 
>> 
>> So I need some explanation on the naming here: Are the first three
>> CAT-independent, but the last one is CAT-dependent? It doesn't
>> look so (or else it would be odd coincidence for the last one to be
>> one higher than the biggest of the _IDX ones). And if they're all
>> in either of the two categories, their names should reflect that
>> (i.e. either all have _CAT in their names, or none does).
>> 
> Please check above comments. Maybe below definitions are better?
> PSR_INFO_IDX_CAT_CBM_LEN
> PSR_INFO_IDX_COS_MAX
> PSR_INFO_IDX_CAT_FLAG
> PSR_INFO_ARRAY_SIZE

But why would the array size be 3 for some feature only having
COS_MAX (for example)? I think you should
- put common indexes first
- have PSR_INFO_CAT_ARRAY_SIZE (or PSR_INFO_NUM_IDX_CAT
or whatever, but with CAT in it).

>> > +
>> > +
>> >  struct psr_cmt_l3 {
>> 
>> No double blank lines please (and just in case: comments of this kind
>> apply to the entire series, i.e. you shouldn't expect them to be
>> repeated in other patches).
>> 
> Sorry for this. Do we have code style check tool in xen?

No, no-one so far had the time to put one together.

Jan

Yi Sun March 27, 2017, 1:19 p.m. UTC | #4

On 17-03-27 06:51:07, Jan Beulich wrote:
> >>> On 27.03.17 at 14:24, <[e-mail]> wrote:
> > On 17-03-27 03:07:37, Jan Beulich wrote:
> >> >>> On 16.03.17 at 12:07, <[e-mail]> wrote:
> >> > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type)
> >> > +{
> >> > +    enum psr_feat_type feat_type;
> >> > +
> >> > +    switch ( type )
> >> > +    {
> >> > +    case PSR_CBM_TYPE_L3:
> >> > +        feat_type = PSR_SOCKET_L3_CAT;
> >> > +        break;
> >> > +    default:
> >> > +        feat_type = PSR_SOCKET_UNKNOWN;
> >> > +        break;
> >> 
> >> Is this actually reachable, if there are no bugs in the code? If not,
> >> you will want to add ASSERT_UNREACHABLE().
> >> 
> > If there is no bug, we should not reach here. Will use ASSERT_UNREACHABLE().
> > 
> >> > +int psr_get_info(unsigned int socket, enum cbm_type type,
> >> > +                 uint32_t data[], unsigned int array_len)
> >> > +{
> >> > +    const struct psr_socket_info *info = get_socket_info(socket);
> >> > +    const struct feat_node *feat;
> >> > +    enum psr_feat_type feat_type;
> >> > +
> >> > +    if ( IS_ERR(info) )
> >> > +        return PTR_ERR(info);
> >> > +
> >> > +    if ( !data )
> >> > +        return -EINVAL;
> >> > +
> >> > +    feat_type = psr_cbm_type_to_feat_type(type);
> >> > +    feat = info->features[feat_type];
> >> 
> >> You can't blindly use the return value here as array index, as (at
> >> least in theory, see above) the function may return
> >> PSR_SOCKET_UNKNOWN. IOW you need to check against
> >> ARRAY_SIZE(info->features) first.
> >> 
> > If I use 'ASSERT_UNREACHABLE()' above, I don't need check against
> > ARRAY_SIZE(info->features) here, right?
> 
> That's a slightly difficult call: The assertion will expand to nothing
> in production builds, so to be on the safe side I think you better
> check function return values _everywhere_.
> 
Got it, will check feat_type.

> >> > +/* Used by psr_get_info() */
> >> > +#define PSR_INFO_IDX_CBM_LEN            0
> > CAT/CDP specific.
> > 
> >> > +#define PSR_INFO_IDX_COS_MAX            1
> > Common so far.
> > 
> >> > +#define PSR_INFO_IDX_FLAG               2
> > CAT/CDP specific so far.
> > 
> >> > +#define PSR_INFO_CAT_SIZE               3
> > Array size which can be used for all features but may not be appropriate for
> > future features. So I defined it as CAT specific.
> > 
> >> 
> >> So I need some explanation on the naming here: Are the first three
> >> CAT-independent, but the last one is CAT-dependent? It doesn't
> >> look so (or else it would be odd coincidence for the last one to be
> >> one higher than the biggest of the _IDX ones). And if they're all
> >> in either of the two categories, their names should reflect that
> >> (i.e. either all have _CAT in their names, or none does).
> >> 
> > Please check above comments. Maybe below definitions are better?
> > PSR_INFO_IDX_CAT_CBM_LEN
> > PSR_INFO_IDX_COS_MAX
> > PSR_INFO_IDX_CAT_FLAG
> > PSR_INFO_ARRAY_SIZE
> 
> But why would the array size be 3 for some feature only having
> COS_MAX (for example)? I think you should
> - put common indexes first
> - have PSR_INFO_CAT_ARRAY_SIZE (or PSR_INFO_NUM_IDX_CAT
> or whatever, but with CAT in it).
> 
Per current known features, all of them (CAT/MBA) have three members in
HW info. So, I think we can define ARRAY_SIZE to 3 and define its name to
a common name so far.

BRs,
Sun Yi

Jan Beulich March 27, 2017, 1:32 p.m. UTC | #5

>>> On 27.03.17 at 15:19, <[e-mail]> wrote:
> On 17-03-27 06:51:07, Jan Beulich wrote:
>> >>> On 27.03.17 at 14:24, <[e-mail]> wrote:
>> > Please check above comments. Maybe below definitions are better?
>> > PSR_INFO_IDX_CAT_CBM_LEN
>> > PSR_INFO_IDX_COS_MAX
>> > PSR_INFO_IDX_CAT_FLAG
>> > PSR_INFO_ARRAY_SIZE
>> 
>> But why would the array size be 3 for some feature only having
>> COS_MAX (for example)? I think you should
>> - put common indexes first
>> - have PSR_INFO_CAT_ARRAY_SIZE (or PSR_INFO_NUM_IDX_CAT
>> or whatever, but with CAT in it).
>> 
> Per current known features, all of them (CAT/MBA) have three members in
> HW info. So, I think we can define ARRAY_SIZE to 3 and define its name to
> a common name so far.

Okay then.

Jan

Patch

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index a6a8081..83358e3 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -60,6 +60,7 @@  enum psr_feat_type {
     PSR_SOCKET_L3_CDP,
     PSR_SOCKET_L2_CAT,
     PSR_SOCKET_MAX_FEAT,
+    PSR_SOCKET_UNKNOWN,
 };
 
 /* CAT/CDP HW info data structure. */
@@ -92,6 +93,10 @@  struct feat_node {
     struct feat_ops {
         /* get_cos_max is used to get feature's cos_max. */
         unsigned int (*get_cos_max)(const struct feat_node *feat);
+
+        /* get_feat_info is used to get feature HW info. */
+        bool (*get_feat_info)(const struct feat_node *feat,
+                              uint32_t data[], unsigned int array_len);
     } ops;
 
     /* Encapsulate feature specific HW info here. */
@@ -185,6 +190,23 @@  static void free_feature(struct psr_socket_info *info)
     }
 }
 
+static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type)
+{
+    enum psr_feat_type feat_type;
+
+    switch ( type )
+    {
+    case PSR_CBM_TYPE_L3:
+        feat_type = PSR_SOCKET_L3_CAT;
+        break;
+    default:
+        feat_type = PSR_SOCKET_UNKNOWN;
+        break;
+    }
+
+    return feat_type;
+}
+
 /* CAT common functions implementation. */
 static void cat_init_feature(struct cpuid_leaf regs,
                              struct feat_node *feat,
@@ -240,9 +262,23 @@  static unsigned int cat_get_cos_max(const struct feat_node *feat)
     return feat->info.cat_info.cos_max;
 }
 
+static bool cat_get_feat_info(const struct feat_node *feat,
+                              uint32_t data[], unsigned int array_len)
+{
+    if ( array_len != PSR_INFO_CAT_SIZE )
+        return false;
+
+    data[PSR_INFO_IDX_CBM_LEN] = feat->info.cat_info.cbm_len;
+    data[PSR_INFO_IDX_COS_MAX] = feat->info.cat_info.cos_max;
+    data[PSR_INFO_IDX_FLAG] = 0;
+
+    return true;
+}
+
 /* L3 CAT ops */
 static const struct feat_ops l3_cat_ops = {
     .get_cos_max = cat_get_cos_max,
+    .get_feat_info = cat_get_feat_info,
 };
 
 static void __init parse_psr_bool(char *s, char *value, char *feature,
@@ -454,10 +490,42 @@  void psr_ctxt_switch_to(struct domain *d)
     }
 }
 
-int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
-                        uint32_t *cos_max, uint32_t *flags)
+static struct psr_socket_info *get_socket_info(unsigned int socket)
 {
-    return 0;
+    if ( !socket_info )
+        return ERR_PTR(-ENODEV);
+
+    if ( socket >= nr_sockets )
+        return ERR_PTR(-ERANGE);
+
+    if ( !socket_info[socket].feat_mask )
+        return ERR_PTR(-ENOENT);
+
+    return socket_info + socket;
+}
+
+int psr_get_info(unsigned int socket, enum cbm_type type,
+                 uint32_t data[], unsigned int array_len)
+{
+    const struct psr_socket_info *info = get_socket_info(socket);
+    const struct feat_node *feat;
+    enum psr_feat_type feat_type;
+
+    if ( IS_ERR(info) )
+        return PTR_ERR(info);
+
+    if ( !data )
+        return -EINVAL;
+
+    feat_type = psr_cbm_type_to_feat_type(type);
+    feat = info->features[feat_type];
+    if ( !feat )
+        return -ENOENT;
+
+    if ( feat->ops.get_feat_info(feat, data, array_len) )
+        return 0;
+
+    return -EINVAL;
 }
 
 int psr_get_l3_cbm(struct domain *d, unsigned int socket,
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 2f7056e..aab3d2d 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -175,14 +175,20 @@  long arch_do_sysctl(
         switch ( sysctl->u.psr_cat_op.cmd )
         {
         case XEN_SYSCTL_PSR_CAT_get_l3_info:
-            ret = psr_get_cat_l3_info(sysctl->u.psr_cat_op.target,
-                                      &sysctl->u.psr_cat_op.u.l3_info.cbm_len,
-                                      &sysctl->u.psr_cat_op.u.l3_info.cos_max,
-                                      &sysctl->u.psr_cat_op.u.l3_info.flags);
+        {
+            uint32_t data[PSR_INFO_CAT_SIZE];
+
+            ret = psr_get_info(sysctl->u.psr_cat_op.target,
+                               PSR_CBM_TYPE_L3, data, ARRAY_SIZE(data));
+
+            sysctl->u.psr_cat_op.u.l3_info.cbm_len = data[PSR_INFO_IDX_CBM_LEN];
+            sysctl->u.psr_cat_op.u.l3_info.cos_max = data[PSR_INFO_IDX_COS_MAX];
+            sysctl->u.psr_cat_op.u.l3_info.flags   = data[PSR_INFO_IDX_FLAG];
 
             if ( !ret && __copy_field_to_guest(u_sysctl, sysctl, u.psr_cat_op) )
                 ret = -EFAULT;
             break;
+        }
 
         default:
             ret = -EOPNOTSUPP;
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index 57f47e9..e27e685 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -19,20 +19,27 @@ 
 #include <xen/types.h>
 
 /* CAT cpuid level */
-#define PSR_CPUID_LEVEL_CAT   0x10
+#define PSR_CPUID_LEVEL_CAT             0x10
 
 /* Resource Type Enumeration */
 #define PSR_RESOURCE_TYPE_L3            0x2
 
 /* L3 Monitoring Features */
-#define PSR_CMT_L3_OCCUPANCY           0x1
+#define PSR_CMT_L3_OCCUPANCY            0x1
 
 /* CDP Capability */
-#define PSR_CAT_CDP_CAPABILITY       (1u << 2)
+#define PSR_CAT_CDP_CAPABILITY          (1u << 2)
 
 /* L3 CDP Enable bit*/
 #define PSR_L3_QOS_CDP_ENABLE_BIT       0x0
 
+/* Used by psr_get_info() */
+#define PSR_INFO_IDX_CBM_LEN            0
+#define PSR_INFO_IDX_COS_MAX            1
+#define PSR_INFO_IDX_FLAG               2
+#define PSR_INFO_CAT_SIZE               3
+
+
 struct psr_cmt_l3 {
     unsigned int features;
     unsigned int upscaling_factor;
@@ -63,8 +70,8 @@  int psr_alloc_rmid(struct domain *d);
 void psr_free_rmid(struct domain *d);
 void psr_ctxt_switch_to(struct domain *d);
 
-int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
-                        uint32_t *cos_max, uint32_t *flags);
+int psr_get_info(unsigned int socket, enum cbm_type type,
+                 uint32_t data[], unsigned int array_len);
 int psr_get_l3_cbm(struct domain *d, unsigned int socket,
                    uint64_t *cbm, enum cbm_type type);
 int psr_set_l3_cbm(struct domain *d, unsigned int socket,