Re: [Xen-devel] [PATCH v8 07/24] x86: refactor psr: implement get value flow.

2017-03-10 Thread Jan Beulich 
>>> On 10.03.17 at 02:50,  wrote:
> On 17-03-08 08:35:53, Jan Beulich wrote:
>> >>> On 15.02.17 at 09:49,  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


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

Re: [Xen-devel] [PATCH v8 07/24] x86: refactor psr: implement get value flow.

2017-03-09 Thread Yi Sun 
On 17-03-08 08:35:53, Jan Beulich wrote:
> >>> On 15.02.17 at 09:49,  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

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

Re: [Xen-devel] [PATCH v8 07/24] x86: refactor psr: implement get value flow.

2017-03-08 Thread Jan Beulich 
>>> On 15.02.17 at 09:49,  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


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

Re: [Xen-devel] [PATCH v8 07/24] x86: refactor psr: implement get value flow.

2017-02-28 Thread Yi Sun 
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.

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

Re: [Xen-devel] [PATCH v8 07/24] x86: refactor psr: implement get value flow.

2017-02-28 Thread Roger Pau Monné 
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 
> Reviewed-by: Konrad Rzeszutek Wilk 
> ---
>  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.

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

[Xen-devel] [PATCH v8 07/24] x86: refactor psr: implement get value flow.

2017-02-15 Thread Yi Sun 
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 
Reviewed-by: Konrad Rzeszutek Wilk 
---
 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/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,
- >u.psr_cat_op.data,
- PSR_CBM_TYPE_L3);
+ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+  >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,
- >u.psr_cat_op.data,
- PSR_CBM_TYPE_L3_CODE);
+ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+  >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,
- >u.psr_cat_op.data,
- PSR_CBM_TYPE_L3_DATA);
+ret = psr_get_val(d, domctl->u.psr_cat_op.target,
+  >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)
 {
-