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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> 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


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