Re: [Xen-devel] [PATCH v10 08/25] x86: refactor psr: L3 CAT: implement get value flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1455,25 +1455,37 @@ 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);
> +        {
> +            uint32_t val;
> +
> +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> +                              &val, PSR_CBM_TYPE_L3);
> +            domctl->u.psr_cat_op.data = val;
>              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);
> +        {
> +            uint32_t val;
> +
> +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> +                              &val, PSR_CBM_TYPE_L3_CODE);
> +            domctl->u.psr_cat_op.data = val;
>              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);
> +        {
> +            uint32_t val;
> +
> +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> +                              &val, PSR_CBM_TYPE_L3_DATA);
> +            domctl->u.psr_cat_op.data = val;
>              copyback = 1;
>              break;
> +        }

I think code would read better overall if you had a switch()-wide
variable (then probably encoding its width in its name, e.g. val32).

> --- 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,
> +                        uint32_t *val);
>      } *props;
>  
>      uint32_t cos_reg_val[MAX_COS_REG_CNT];
> @@ -265,10 +269,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,
> +                        uint32_t *val)
> +{
> +    *val = feat->cos_reg_val[cos];
> +}

This can be done by the caller - there's nothing feature specific in
here, so there's no need for a hook.

> @@ -494,24 +505,34 @@ 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 struct feat_node * psr_get_feat(unsigned int socket,

Stray blank after *.

> +                                       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 ERR_PTR(PTR_ERR(info));

Urgh. But yes, a cast would seem to be the worse alternative.

> @@ -521,9 +542,35 @@ 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;
> +
> +    ASSERT(d && val);

I don't think we ever ASSERT() domain pointers to be non-NULL.

Jan

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