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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-05-30 08:05:02, Jan Beulich wrote:
> >>> 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.
> 
This NULL is taken care by below code:
    if ( !feat || !feat_props[feat_type] ) 

The returned errors are handled separately. For PTR_ERR, it is handled
above. For NULL, it is handled below.

I checked IS_ERR, I think it can handle the NULL case. The NULL will not
be treated as an error.

> 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?
> 
Because the helper function 'psr_get_feat_and_type' is only used by
'psr_get_info' if we implement it in previous patch. This seems
unnecessary. So, I introduce this helper function in this patch.
Shall I move it to previous patch?

> > -    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.)
> 
Ok, got it.

> > @@ -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.
> 
Will fix it. Thanks!

> Jan

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