diff mbox

[v9,08/25] x86: refactor psr: L3 CAT: implement get value flow.

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

Commit Message

Yi Sun March 16, 2017, 11:07 a.m. UTC 
There is an interface in user space to show feature value of
domains.

This patch implements get value flow in hypervisor 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>
---
v9:
    - add commit message to explain there is an user space interface.
    - rename 'l3_cat_get_val' to 'cat_get_val' to cover all L3/L2 CAT features.
      (suggested by Roger Pau)
    - replace feature list handling to feature array handling.
      (suggested by Roger Pau)
    - change parameter of 'psr_get'. Use 'psr_cos_ids' directly to replace
      domain. Also declare it to 'const'.
      (suggested by Jan Beulich)
    - change code flow to remove 'psr_get' but add 'psr_get_feat' to make codes
      more reasonable.
      (suggested by Jan Beulich)
    - modify patch title to indicate 'L3 CAT'.
      (suggested by Jan Beulich)
    - move cos check into common function because this check is required by all
      features.
      (suggested by Jan Beulich)
    - fix coding style issue.
      (suggested by Jan Beulich)
    - changes about 'uint64_t' to 'uint32_t'..
      (suggested by Jan Beulich)
v7:
    - rename '__psr_get' to 'psr_get'.
      (suggested by Wei Liu)
v6:
    - modify commit message to make it clearer.
      (suggested by Konrad Rzeszutek Wilk)
    - remove one extra space in code.
      (suggested by Konrad Rzeszutek Wilk)
    - remove unnecessary comment.
      (suggested by Konrad Rzeszutek Wilk)
    - write a helper function to move get info and get val functions into
      it. Because most codes of 'get_info' and 'get_val' are same.
      (suggested by Konrad Rzeszutek Wilk)
v5:
    - rename 'dat[]' to 'data[]'
      (suggested by Jan Beulich)
    - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'.
      (suggested by Jan Beulich)
    - check if feature type match in caller of feature callback function.
      (suggested by Jan Beulich)
v4:
    - create this patch to make codes easier to understand.
      (suggested by Jan Beulich)
---
 xen/arch/x86/domctl.c     | 21 +++++++++--------
 xen/arch/x86/psr.c        | 58 +++++++++++++++++++++++++++++++++++++++++------
 xen/include/asm-x86/psr.h |  4 ++--
 3 files changed, 65 insertions(+), 18 deletions(-)

Comments

Jan Beulich March 27, 2017, 9:23 a.m. UTC | #1 
>>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1455,23 +1455,26 @@ 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);
> +            domctl->u.psr_cat_op.data = 0;
> +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> +                              (uint32_t *)&domctl->u.psr_cat_op.data,

This is exactly why I generally object to casts: The high half of
the field will remain untouched, likely confusing the caller. You
need to decide at what layer you want to do the extension from
the internally used 32-bit type to the public interface induced
64-bit one.

> @@ -504,21 +515,30 @@ 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 const struct feat_node * psr_get_feat(unsigned int socket,
> +                                             enum cbm_type type)
>  {
>      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);
> +        return NULL;

You're losing the error information here - is that intentional?

> +    feat_type = psr_cbm_type_to_feat_type(type);
> +    feat = info->features[feat_type];
> +    return feat;

No need for at least the intermediate variable "feat". Instead
please add a blank line before the final return statement.

> @@ -528,9 +548,33 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
>      return -EINVAL;
>  }
>  
> -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,
> +                uint32_t *val, enum cbm_type type)
>  {
> +    const struct feat_node *feat;
> +    unsigned int cos;
> +
> +    if ( !d || !val )
> +        return -EINVAL;

Wouldn't this better be an ASSERT()? -EINVAL should generally
indicate bad hypercall input, not things got wrong internally.

Jan
Yi Sun March 27, 2017, 12:59 p.m. UTC | #2 
On 17-03-27 03:23:08, Jan Beulich wrote:
> >>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> wrote:
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -1455,23 +1455,26 @@ 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);
> > +            domctl->u.psr_cat_op.data = 0;
> > +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +                              (uint32_t *)&domctl->u.psr_cat_op.data,
> 
> This is exactly why I generally object to casts: The high half of
> the field will remain untouched, likely confusing the caller. You
> need to decide at what layer you want to do the extension from
> the internally used 32-bit type to the public interface induced
> 64-bit one.
> 
'psr_cat_op.data' is used as interface between tools/ and hyperviosr. We
defined it as 'uint64_t' to fulfill future requests because MSRs registers
are 64bit although the upper 32bit are not used yet.

Per your suggetion to use 'uint32_t' internally for CBM, I changed
psr_get_val/psr_set_val parameters type from 'uint64_t' to 'uint32_t'. That
is the reason to do cast here. Is this an appropriate choice?

> > @@ -504,21 +515,30 @@ 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 const struct feat_node * psr_get_feat(unsigned int socket,
> > +                                             enum cbm_type type)
> >  {
> >      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);
> > +        return NULL;
> 
> You're losing the error information here - is that intentional?
> 
This function returns the 'struct feat_node *' object. If error happens, it
returns NULL. The caller handles the error.

> > +    feat_type = psr_cbm_type_to_feat_type(type);
> > +    feat = info->features[feat_type];
> > +    return feat;
> 
> No need for at least the intermediate variable "feat". Instead
> please add a blank line before the final return statement.
> 
Sure. Thanks!

> > @@ -528,9 +548,33 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
> >      return -EINVAL;
> >  }
> >  
> > -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,
> > +                uint32_t *val, enum cbm_type type)
> >  {
> > +    const struct feat_node *feat;
> > +    unsigned int cos;
> > +
> > +    if ( !d || !val )
> > +        return -EINVAL;
> 
> Wouldn't this better be an ASSERT()? -EINVAL should generally
> indicate bad hypercall input, not things got wrong internally.
> 
Ok, thanks!

> Jan
Jan Beulich March 27, 2017, 1:34 p.m. UTC | #3 
>>> On 27.03.17 at 14:59, <yi.y.sun@linux.intel.com> wrote:
> On 17-03-27 03:23:08, Jan Beulich wrote:
>> >>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> wrote:
>> > --- a/xen/arch/x86/domctl.c
>> > +++ b/xen/arch/x86/domctl.c
>> > @@ -1455,23 +1455,26 @@ 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);
>> > +            domctl->u.psr_cat_op.data = 0;
>> > +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
>> > +                              (uint32_t *)&domctl->u.psr_cat_op.data,
>> 
>> This is exactly why I generally object to casts: The high half of
>> the field will remain untouched, likely confusing the caller. You
>> need to decide at what layer you want to do the extension from
>> the internally used 32-bit type to the public interface induced
>> 64-bit one.
>> 
> 'psr_cat_op.data' is used as interface between tools/ and hyperviosr. We
> defined it as 'uint64_t' to fulfill future requests because MSRs registers
> are 64bit although the upper 32bit are not used yet.
> 
> Per your suggetion to use 'uint32_t' internally for CBM, I changed
> psr_get_val/psr_set_val parameters type from 'uint64_t' to 'uint32_t'. That
> is the reason to do cast here. Is this an appropriate choice?

No, as said, it is not. The choice of types is fine, but you need to
make this work without casts (i.e. presumably with some
intermediate variable at one of the layers).

>> > @@ -504,21 +515,30 @@ 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 const struct feat_node * psr_get_feat(unsigned int socket,
>> > +                                             enum cbm_type type)
>> >  {
>> >      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);
>> > +        return NULL;
>> 
>> You're losing the error information here - is that intentional?
>> 
> This function returns the 'struct feat_node *' object. If error happens, it
> returns NULL. The caller handles the error.

You didn't understand: Your callee has handed you error
information (a -E... value encoded as a pointer), which you
discard.

Jan
Yi Sun March 28, 2017, 2:13 a.m. UTC | #4 
On 17-03-27 07:34:43, Jan Beulich wrote:
> >>> On 27.03.17 at 14:59, <yi.y.sun@linux.intel.com> wrote:
> > On 17-03-27 03:23:08, Jan Beulich wrote:
> >> >>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> wrote:
> >> > --- a/xen/arch/x86/domctl.c
> >> > +++ b/xen/arch/x86/domctl.c
> >> > @@ -1455,23 +1455,26 @@ 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);
> >> > +            domctl->u.psr_cat_op.data = 0;
> >> > +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> >> > +                              (uint32_t *)&domctl->u.psr_cat_op.data,
> >> 
> >> This is exactly why I generally object to casts: The high half of
> >> the field will remain untouched, likely confusing the caller. You
> >> need to decide at what layer you want to do the extension from
> >> the internally used 32-bit type to the public interface induced
> >> 64-bit one.
> >> 
> > 'psr_cat_op.data' is used as interface between tools/ and hyperviosr. We
> > defined it as 'uint64_t' to fulfill future requests because MSRs registers
> > are 64bit although the upper 32bit are not used yet.
> > 
> > Per your suggetion to use 'uint32_t' internally for CBM, I changed
> > psr_get_val/psr_set_val parameters type from 'uint64_t' to 'uint32_t'. That
> > is the reason to do cast here. Is this an appropriate choice?
> 
> No, as said, it is not. The choice of types is fine, but you need to
> make this work without casts (i.e. presumably with some
> intermediate variable at one of the layers).
> 
Then, I have to use a local variable to do this.

> >> > @@ -504,21 +515,30 @@ 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 const struct feat_node * psr_get_feat(unsigned int socket,
> >> > +                                             enum cbm_type type)
> >> >  {
> >> >      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);
> >> > +        return NULL;
> >> 
> >> You're losing the error information here - is that intentional?
> >> 
> > This function returns the 'struct feat_node *' object. If error happens, it
> > returns NULL. The caller handles the error.
> 
> You didn't understand: Your callee has handed you error
> information (a -E... value encoded as a pointer), which you
> discard.
> 
Thanks for explanation! I will modify the type of 'psr_get_feat' to be able to
return such error back.

> Jan
Jan Beulich March 28, 2017, 8:10 a.m. UTC | #5 
>>> On 28.03.17 at 04:13, <yi.y.sun@linux.intel.com> wrote:
> On 17-03-27 07:34:43, Jan Beulich wrote:
>> >>> On 27.03.17 at 14:59, <yi.y.sun@linux.intel.com> wrote:
>> > On 17-03-27 03:23:08, Jan Beulich wrote:
>> >> >>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> wrote:
>> >> > @@ -504,21 +515,30 @@ 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 const struct feat_node * psr_get_feat(unsigned int socket,
>> >> > +                                             enum cbm_type type)
>> >> >  {
>> >> >      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);
>> >> > +        return NULL;
>> >> 
>> >> You're losing the error information here - is that intentional?
>> >> 
>> > This function returns the 'struct feat_node *' object. If error happens, it
>> > returns NULL. The caller handles the error.
>> 
>> You didn't understand: Your callee has handed you error
>> information (a -E... value encoded as a pointer), which you
>> discard.
>> 
> Thanks for explanation! I will modify the type of 'psr_get_feat' to be able to
> return such error back.

Why would you need to modify its type? Just pass on the error
indicator, instead of returning NULL. It's the caller(s) who need(s)
to change.

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 02b48e8..e714d1d 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1455,23 +1455,26 @@  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);
+            domctl->u.psr_cat_op.data = 0;
+            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+                              (uint32_t *)&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);
+            domctl->u.psr_cat_op.data = 0;
+            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+                              (uint32_t *)&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);
+            domctl->u.psr_cat_op.data = 0;
+            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+                              (uint32_t *)&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 83358e3..53105f5 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -97,6 +97,10 @@  struct feat_node {
         /* 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. */
+        void (*get_val)(const struct feat_node *feat, unsigned int cos,
+                        enum cbm_type type, uint32_t *val);
     } ops;
 
     /* Encapsulate feature specific HW info here. */
@@ -275,10 +279,17 @@  static bool cat_get_feat_info(const struct feat_node *feat,
     return true;
 }
 
+static void cat_get_val(const struct feat_node *feat, unsigned int cos,
+                        enum cbm_type type, uint32_t *val)
+{
+    *val = feat->cos_reg_val[cos];
+}
+
 /* 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,
+    .get_val = cat_get_val,
 };
 
 static void __init parse_psr_bool(char *s, char *value, char *feature,
@@ -504,21 +515,30 @@  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 const struct feat_node * psr_get_feat(unsigned int socket,
+                                             enum cbm_type type)
 {
     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);
+        return NULL;
+
+    feat_type = psr_cbm_type_to_feat_type(type);
+    feat = info->features[feat_type];
+    return feat;
+}
+
+int psr_get_info(unsigned int socket, enum cbm_type type,
+                 uint32_t data[], unsigned int array_len)
+{
+    const struct feat_node *feat;
 
     if ( !data )
         return -EINVAL;
 
-    feat_type = psr_cbm_type_to_feat_type(type);
-    feat = info->features[feat_type];
+    feat = psr_get_feat(socket, type);
     if ( !feat )
         return -ENOENT;
 
@@ -528,9 +548,33 @@  int psr_get_info(unsigned int socket, enum cbm_type type,
     return -EINVAL;
 }
 
-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,
+                uint32_t *val, enum cbm_type type)
 {
+    const struct feat_node *feat;
+    unsigned int cos;
+
+    if ( !d || !val )
+        return -EINVAL;
+
+    feat = psr_get_feat(socket, type);
+    if ( !feat )
+        return -ENOENT;
+
+    cos = d->arch.psr_cos_ids[socket];
+    /*
+     * If input cos exceeds current feature's cos_max, we should return its
+     * default value which is stored in cos 0. This case only happens
+     * when more than two features enabled concurrently and at least one
+     * features's cos_max is bigger than others. When a domain's working cos
+     * id is bigger than some features' cos_max, HW automatically works as
+     * default value for those features which cos_max is smaller.
+     */
+    if ( cos > feat->ops.get_cos_max(feat) )
+        cos = 0;
+
+    feat->ops.get_val(feat, cos, type, val);
+
     return 0;
 }
 
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index e27e685..cd00733 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -72,8 +72,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,
+                uint32_t *val, enum cbm_type type);
 int psr_set_l3_cbm(struct domain *d, unsigned int socket,
                    uint64_t cbm, enum cbm_type type);