Re: [Xen-devel] [PATCH v11 07/23] x86: refactor psr: L3 CAT: implement get value flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.05.17 at 10:44, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:

> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -476,23 +476,34 @@ static struct psr_socket_info *get_socket_info(unsigned 
> int socket)
>      return socket_info + socket;
>  }
>  
> +static struct feat_node *psr_get_feat_and_type(unsigned int socket,
> +                                               enum cbm_type type,
> +                                               enum psr_feat_type *feat_type)
> +{
> +    const struct psr_socket_info *info = get_socket_info(socket);
> +
> +    if ( IS_ERR(info) )
> +        return ERR_PTR(PTR_ERR(info));
> +
> +    *feat_type = psr_cbm_type_to_feat_type(type);
> +    if ( *feat_type >= ARRAY_SIZE(info->features) )
> +        return NULL;

Note how this return is not being taken care of by ...

> +    return info->features[*feat_type];
> +}
> +
>  int psr_get_info(unsigned int socket, enum cbm_type type,
>                   uint32_t data[], unsigned int array_len)
>  {
> -    const struct psr_socket_info *info = get_socket_info(socket);
>      const struct feat_node *feat;
>      enum psr_feat_type feat_type;
>  
>      ASSERT(data);
>  
> -    if ( IS_ERR(info) )
> -        return PTR_ERR(info);
> -
> -    feat_type = psr_cbm_type_to_feat_type(type);
> -    if ( feat_type >= ARRAY_SIZE(info->features) )
> -        return -ENOENT;
> +    feat = psr_get_feat_and_type(socket, type, &feat_type);
> +    if ( IS_ERR(feat) )
> +        return PTR_ERR(feat);

... the check here. I think you want to alter the return above.

And of course I wonder why you replace code here that was
only introduced one or two patches earlier. Perhaps that earlier
patch should do things this way right away?

> -    feat = info->features[feat_type];
>      if ( !feat || !feat_props[feat_type] )
>          return -ENOENT;

Afaics you need feat_type here only to get at the right feat_props[]
entry. If that's the case also for future callers of
psr_get_feat_and_type(), perhaps it would be better for it to
provide those two instead of the intermediate type? Of course
that would imply renaming the function. (This change would
clearly benefit the readability of psr_get_val() below.)

> @@ -502,9 +513,38 @@ 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;
> +    enum psr_feat_type feat_type;
> +    unsigned int cos, i;
> +
> +    ASSERT(val);
> +
> +    feat = psr_get_feat_and_type(socket, type, &feat_type);
> +    if ( IS_ERR(feat) )
> +        return PTR_ERR(feat);
> +
> +    if ( !feat || !feat_props[feat_type] )
> +        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->cos_max )
> +        cos = 0;
> +
> +    for ( i = 0; i < feat_props[feat_type]->cos_num; i++ )
> +        if ( type == feat_props[feat_type]->type[i] )
> +            *val = feat->cos_reg_val[cos * feat_props[feat_type]->cos_num + 
> i];
> +
>      return 0;
>  }

Do you really want to return success here even if you didn't write
to *val? With the way the callers are coded, this is an (at least
latent) information leak at present.

Jan


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