diff mbox

[v8,07/24] x86: refactor psr: implement get value flow.

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

Commit Message

Yi Sun Feb. 15, 2017, 8:49 a.m. UTC
This patch implements get value flow including L3 CAT callback
function.

It also changes domctl interface to make it more general.

With this patch, 'psr-cat-show' can work for L3 CAT but not for
L3 code/data which is implemented in patch "x86: refactor psr:
implement get value flow for CDP.".

Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/domctl.c     | 18 +++++++++---------
 xen/arch/x86/psr.c        | 43 ++++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/psr.h |  4 ++--
 3 files changed, 49 insertions(+), 16 deletions(-)

Comments

Roger Pau Monné Feb. 28, 2017, 12:44 p.m. UTC | #1
On Wed, Feb 15, 2017 at 04:49:22PM +0800, Yi Sun wrote:
> This patch implements get value flow including L3 CAT callback
> function.
> 
> It also changes domctl interface to make it more general.
> 
> With this patch, 'psr-cat-show' can work for L3 CAT but not for
> L3 code/data which is implemented in patch "x86: refactor psr:
> implement get value flow for CDP.".
> 
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/arch/x86/domctl.c     | 18 +++++++++---------
>  xen/arch/x86/psr.c        | 43 ++++++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-x86/psr.h |  4 ++--
>  3 files changed, 49 insertions(+), 16 deletions(-)
[...]
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 8af59d9..c1afd36 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -116,6 +116,9 @@ struct feat_ops {
>      /* 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);
> +    /* get_val is used to get feature COS register value. */
> +    bool (*get_val)(const struct feat_node *feat, unsigned int cos,
> +                    enum cbm_type type, uint64_t *val);
>  };
>  
>  /*
> @@ -260,9 +263,22 @@ static bool l3_cat_get_feat_info(const struct feat_node *feat,
>      return true;
>  }
>  
> +static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos,
> +                           enum cbm_type type, uint64_t *val)
> +{
> +    if ( cos > feat->info.l3_cat_info.cos_max )
> +        /* Use default value. */
> +        cos = 0;

I don't know much, but shouldn't this return false instead of silently
defaulting to 0? This doesn't seem to be what the caller expects.

> +
> +    *val = feat->cos_reg_val[cos];
> +
> +    return true;
> +}
> +
>  static const struct feat_ops l3_cat_ops = {
>      .get_cos_max = l3_cat_get_cos_max,
>      .get_feat_info = l3_cat_get_feat_info,
> +    .get_val = l3_cat_get_val,
>  };
>  
>  static void __init parse_psr_bool(char *s, char *value, char *feature,
> @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned int socket)
>      return socket_info + socket;
>  }
>  
> -int psr_get_info(unsigned int socket, enum cbm_type type,
> -                 uint32_t data[], unsigned int array_len)
> +static int psr_get(unsigned int socket, enum cbm_type type,
> +                   uint32_t data[], unsigned int array_len,
> +                   struct domain *d, uint64_t *val)
>  {
>      const struct psr_socket_info *info = get_socket_info(socket);
>      const struct feat_node *feat;
>      enum psr_feat_type feat_type;
> +    unsigned int cos;
>  
>      if ( IS_ERR(info) )
>          return PTR_ERR(info);
> @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
>          if ( feat->feature != feat_type )
>              continue;
>  
> +        if ( d )
> +        {
> +            cos = d->arch.psr_cos_ids[socket];
> +            if ( feat->ops.get_val(feat, cos, type, val) )
> +                return 0;
> +            else

No need for the "else" branch here.

Roger.
Yi Sun March 1, 2017, 5:21 a.m. UTC | #2
On 17-02-28 12:44:34, Roger Pau Monn� wrote:
> On Wed, Feb 15, 2017 at 04:49:22PM +0800, Yi Sun wrote:
> > +static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos,
> > +                           enum cbm_type type, uint64_t *val)
> > +{
> > +    if ( cos > feat->info.l3_cat_info.cos_max )
> > +        /* Use default value. */
> > +        cos = 0;
> 
> I don't know much, but shouldn't this return false instead of silently
> defaulting to 0? This doesn't seem to be what the caller expects.
> 
If cos exceeds the cos_max, we should return default value saved in
cos_reg_val[0]. Let me explain more, different features have different
cos_max, e.g. L3 CAT cos_max=16, L2 CAT cos_max=8, user can set L3 CAT
COS[9] for a domain. When COS ID 9 is set to ASSOC register, it works
for all features. HW automatically works as default value for L2 CAT
because the COS ID exceeds the max.

> > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
> >          if ( feat->feature != feat_type )
> >              continue;
> >  
> > +        if ( d )
> > +        {
> > +            cos = d->arch.psr_cos_ids[socket];
> > +            if ( feat->ops.get_val(feat, cos, type, val) )
> > +                return 0;
> > +            else
> 
> No need for the "else" branch here.
> 
Thanks!

> Roger.
Jan Beulich March 8, 2017, 3:35 p.m. UTC | #3
>>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
> @@ -260,9 +263,22 @@ static bool l3_cat_get_feat_info(const struct feat_node *feat,
>      return true;
>  }
>  
> +static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos,
> +                           enum cbm_type type, uint64_t *val)
> +{
> +    if ( cos > feat->info.l3_cat_info.cos_max )
> +        /* Use default value. */
> +        cos = 0;
> +
> +    *val = feat->cos_reg_val[cos];
> +
> +    return true;

This one never failing I wonder whether the same will apply to the
later ones. If so, there's little point in returning a boolean here, but
instead you could return the value instead of using indirection.

>  static void __init parse_psr_bool(char *s, char *value, char *feature,
> @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned int socket)
>      return socket_info + socket;
>  }
>  
> -int psr_get_info(unsigned int socket, enum cbm_type type,
> -                 uint32_t data[], unsigned int array_len)
> +static int psr_get(unsigned int socket, enum cbm_type type,

The immediately preceding patch introduced thus function, and
now you're changing its name. Please give it the intended final
name right away.

> +                   uint32_t data[], unsigned int array_len,
> +                   struct domain *d, uint64_t *val)

const struct domain *, but I'm not even sure that's an appropriate
parameter here:

> @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
>          if ( feat->feature != feat_type )
>              continue;
>  
> +        if ( d )
> +        {
> +            cos = d->arch.psr_cos_ids[socket];

You could equally well pass a more constrained pointer, like
psr_cos_ids[] on its own. But of course much depends on whether
you'll need d for other things in this function in later patches.

> +            if ( feat->ops.get_val(feat, cos, type, val) )
> +                return 0;
> +            else
> +                break;
> +        }
> +
>          if ( feat->ops.get_feat_info(feat, data, array_len) )
>              return 0;
>          else

Looking at the context here - is it really a good idea to overload the
function in this way, rather than creating a second one? Your
only complicating the live of the callers, as can be seen e.g. ...

> @@ -507,10 +534,16 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
>      return -ENOENT;
>  }
>  
> -int psr_get_l3_cbm(struct domain *d, unsigned int socket,
> -                   uint64_t *cbm, enum cbm_type type)
> +int psr_get_info(unsigned int socket, enum cbm_type type,
> +                 uint32_t data[], unsigned int array_len)
> +{
> +    return psr_get(socket, type, data, array_len, NULL, NULL);

... here and ...

> +}
> +
> +int psr_get_val(struct domain *d, unsigned int socket,
> +                uint64_t *val, enum cbm_type type)
>  {
> -    return 0;
> +    return psr_get(socket, type, NULL, 0, d, val);
>  }

... here (it is a bad sign that both pass NULL on either side).

Jan
Yi Sun March 10, 2017, 1:50 a.m. UTC | #4
On 17-03-08 08:35:53, Jan Beulich wrote:
> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
> > @@ -260,9 +263,22 @@ static bool l3_cat_get_feat_info(const struct feat_node *feat,
> >      return true;
> >  }
> >  
> > +static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos,
> > +                           enum cbm_type type, uint64_t *val)
> > +{
> > +    if ( cos > feat->info.l3_cat_info.cos_max )
> > +        /* Use default value. */
> > +        cos = 0;
> > +
> > +    *val = feat->cos_reg_val[cos];
> > +
> > +    return true;
> 
> This one never failing I wonder whether the same will apply to the
> later ones. If so, there's little point in returning a boolean here, but
> instead you could return the value instead of using indirection.
> 
I have modified this function to 'void' in next version.

> >  static void __init parse_psr_bool(char *s, char *value, char *feature,
> > @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned int socket)
> >      return socket_info + socket;
> >  }
> >  
> > -int psr_get_info(unsigned int socket, enum cbm_type type,
> > -                 uint32_t data[], unsigned int array_len)
> > +static int psr_get(unsigned int socket, enum cbm_type type,
> 
> The immediately preceding patch introduced thus function, and
> now you're changing its name. Please give it the intended final
> name right away.
> 
The name is not changed. You can see below lines. Here is just to implement
a helper function which is called by 'psr_get_info'.

> > +                   uint32_t data[], unsigned int array_len,
> > +                   struct domain *d, uint64_t *val)
> 
> const struct domain *, but I'm not even sure that's an appropriate
> parameter here:
> 
> > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
> >          if ( feat->feature != feat_type )
> >              continue;
> >  
> > +        if ( d )
> > +        {
> > +            cos = d->arch.psr_cos_ids[socket];
> 
> You could equally well pass a more constrained pointer, like
> psr_cos_ids[] on its own. But of course much depends on whether
> you'll need d for other things in this function in later patches.
> 
Ok, thanks! Will consider the parameter.

> > +            if ( feat->ops.get_val(feat, cos, type, val) )
> > +                return 0;
> > +            else
> > +                break;
> > +        }
> > +
> >          if ( feat->ops.get_feat_info(feat, data, array_len) )
> >              return 0;
> >          else
> 
> Looking at the context here - is it really a good idea to overload the
> function in this way, rather than creating a second one? Your
> only complicating the live of the callers, as can be seen e.g. ...
> 
These codes were separated into two functions before, 'psr_get_info' and
'psr_get_val'. But there are some common codes. So, Konrad suggested me
to create a helper function to reduce redundant codes.

> > @@ -507,10 +534,16 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
> >      return -ENOENT;
> >  }
> >  
> > -int psr_get_l3_cbm(struct domain *d, unsigned int socket,
> > -                   uint64_t *cbm, enum cbm_type type)
> > +int psr_get_info(unsigned int socket, enum cbm_type type,
> > +                 uint32_t data[], unsigned int array_len)
> > +{
> > +    return psr_get(socket, type, data, array_len, NULL, NULL);
> 
> ... here and ...
> 
> > +}
> > +
> > +int psr_get_val(struct domain *d, unsigned int socket,
> > +                uint64_t *val, enum cbm_type type)
> >  {
> > -    return 0;
> > +    return psr_get(socket, type, NULL, 0, d, val);
> >  }
> 
> ... here (it is a bad sign that both pass NULL on either side).
> 
Yes, these things look not good. But to keep a common helper I have to pass
all necessary parameters into it. What is your suggestion?

> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
Jan Beulich March 10, 2017, 9:05 a.m. UTC | #5
>>> On 10.03.17 at 02:50, <yi.y.sun@linux.intel.com> wrote:
> On 17-03-08 08:35:53, Jan Beulich wrote:
>> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
>> > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
>> >          if ( feat->feature != feat_type )
>> >              continue;
>> >  
>> > +        if ( d )
>> > +        {
>> > +            cos = d->arch.psr_cos_ids[socket];
>> > +            if ( feat->ops.get_val(feat, cos, type, val) )
>> > +                return 0;
>> > +            else
>> > +                break;
>> > +        }
>> > +
>> >          if ( feat->ops.get_feat_info(feat, data, array_len) )
>> >              return 0;
>> >          else
>> 
>> Looking at the context here - is it really a good idea to overload the
>> function in this way, rather than creating a second one? Your
>> only complicating the live of the callers, as can be seen e.g. ...
>> 
> These codes were separated into two functions before, 'psr_get_info' and
> 'psr_get_val'. But there are some common codes. So, Konrad suggested me
> to create a helper function to reduce redundant codes.

I think you went too far - breaking out common code is nice, but if
the two callers now pass NULL/0 each as two of the last four
arguments, this is a clear indication that you've folded more code
than was actually common.

>> > @@ -507,10 +534,16 @@ int psr_get_info(unsigned int socket, enum cbm_type 
> type,
>> >      return -ENOENT;
>> >  }
>> >  
>> > -int psr_get_l3_cbm(struct domain *d, unsigned int socket,
>> > -                   uint64_t *cbm, enum cbm_type type)
>> > +int psr_get_info(unsigned int socket, enum cbm_type type,
>> > +                 uint32_t data[], unsigned int array_len)
>> > +{
>> > +    return psr_get(socket, type, data, array_len, NULL, NULL);
>> 
>> ... here and ...
>> 
>> > +}
>> > +
>> > +int psr_get_val(struct domain *d, unsigned int socket,
>> > +                uint64_t *val, enum cbm_type type)
>> >  {
>> > -    return 0;
>> > +    return psr_get(socket, type, NULL, 0, d, val);
>> >  }
>> 
>> ... here (it is a bad sign that both pass NULL on either side).
>> 
> Yes, these things look not good. But to keep a common helper I have to pass
> all necessary parameters into it. What is your suggestion?

If there's really that much code to be shared (which I continue not
to be convinced of), use of a function pointer may make this look a
little better. But I think when having tried to carry out the earlier
suggestion, you should have responded back right away indicating
this would result in other ugliness.

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 8e5259f..098e399 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1440,23 +1440,23 @@  long arch_do_domctl(
             break;
 
         case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
-            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
-                                 &domctl->u.psr_cat_op.data,
-                                 PSR_CBM_TYPE_L3);
+            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+                              &domctl->u.psr_cat_op.data,
+                              PSR_CBM_TYPE_L3);
             copyback = 1;
             break;
 
         case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE:
-            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
-                                 &domctl->u.psr_cat_op.data,
-                                 PSR_CBM_TYPE_L3_CODE);
+            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+                              &domctl->u.psr_cat_op.data,
+                              PSR_CBM_TYPE_L3_CODE);
             copyback = 1;
             break;
 
         case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA:
-            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
-                                 &domctl->u.psr_cat_op.data,
-                                 PSR_CBM_TYPE_L3_DATA);
+            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+                              &domctl->u.psr_cat_op.data,
+                              PSR_CBM_TYPE_L3_DATA);
             copyback = 1;
             break;
 
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 8af59d9..c1afd36 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -116,6 +116,9 @@  struct feat_ops {
     /* 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);
+    /* get_val is used to get feature COS register value. */
+    bool (*get_val)(const struct feat_node *feat, unsigned int cos,
+                    enum cbm_type type, uint64_t *val);
 };
 
 /*
@@ -260,9 +263,22 @@  static bool l3_cat_get_feat_info(const struct feat_node *feat,
     return true;
 }
 
+static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos,
+                           enum cbm_type type, uint64_t *val)
+{
+    if ( cos > feat->info.l3_cat_info.cos_max )
+        /* Use default value. */
+        cos = 0;
+
+    *val = feat->cos_reg_val[cos];
+
+    return true;
+}
+
 static const struct feat_ops l3_cat_ops = {
     .get_cos_max = l3_cat_get_cos_max,
     .get_feat_info = l3_cat_get_feat_info,
+    .get_val = l3_cat_get_val,
 };
 
 static void __init parse_psr_bool(char *s, char *value, char *feature,
@@ -482,12 +498,14 @@  static struct psr_socket_info *get_socket_info(unsigned int socket)
     return socket_info + socket;
 }
 
-int psr_get_info(unsigned int socket, enum cbm_type type,
-                 uint32_t data[], unsigned int array_len)
+static int psr_get(unsigned int socket, enum cbm_type type,
+                   uint32_t data[], unsigned int array_len,
+                   struct domain *d, uint64_t *val)
 {
     const struct psr_socket_info *info = get_socket_info(socket);
     const struct feat_node *feat;
     enum psr_feat_type feat_type;
+    unsigned int cos;
 
     if ( IS_ERR(info) )
         return PTR_ERR(info);
@@ -498,6 +516,15 @@  int psr_get_info(unsigned int socket, enum cbm_type type,
         if ( feat->feature != feat_type )
             continue;
 
+        if ( d )
+        {
+            cos = d->arch.psr_cos_ids[socket];
+            if ( feat->ops.get_val(feat, cos, type, val) )
+                return 0;
+            else
+                break;
+        }
+
         if ( feat->ops.get_feat_info(feat, data, array_len) )
             return 0;
         else
@@ -507,10 +534,16 @@  int psr_get_info(unsigned int socket, enum cbm_type type,
     return -ENOENT;
 }
 
-int psr_get_l3_cbm(struct domain *d, unsigned int socket,
-                   uint64_t *cbm, enum cbm_type type)
+int psr_get_info(unsigned int socket, enum cbm_type type,
+                 uint32_t data[], unsigned int array_len)
+{
+    return psr_get(socket, type, data, array_len, NULL, NULL);
+}
+
+int psr_get_val(struct domain *d, unsigned int socket,
+                uint64_t *val, enum cbm_type type)
 {
-    return 0;
+    return psr_get(socket, type, NULL, 0, d, val);
 }
 
 int psr_set_l3_cbm(struct domain *d, unsigned int socket,
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index 0342a80..569e7df 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -70,8 +70,8 @@  void psr_ctxt_switch_to(struct domain *d);
 
 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_get_val(struct domain *d, unsigned int socket,
+                uint64_t *val, enum cbm_type type);
 int psr_set_l3_cbm(struct domain *d, unsigned int socket,
                    uint64_t cbm, enum cbm_type type);