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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel