Re: [Xen-devel] [PATCH v12 15/23] x86: refactor psr: CDP: implement set value callback function.

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

On 17-06-30 15:22:56, Yi Sun wrote:
> On 17-06-30 00:42:22, Jan Beulich wrote:
> 
> > > @@ -805,17 +816,24 @@ static int insert_val_into_array(uint32_t val[],
> > >      if ( !psr_check_cbm(feat->cbm_len, new_val) )
> > >          return -EINVAL;
> > >  
> > > -    /* Value setting position is same as feature array. */
> > > +    /*
> > > +     * Value setting position is same as feature array.
> > > +     * For CDP, user may set both DATA and CODE to same value. For such 
> > > case,
> > > +     * user input 'PSR_CBM_TYPE_L3' as type. The strict feature type of
> > > +     * 'PSR_CBM_TYPE_L3' is L3 CAT. So, we should set new_val to both of 
> > > DATA
> > > +     * and CODE under such case.
> > > +     */
> > >      for ( i = 0; i < props->cos_num; i++ )
> > >      {
> > > -        if ( type == props->type[i] )
> > > +        if ( type == props->type[i] ||
> > > +             feat_type != psr_cbm_type_to_feat_type(type, true) )
> > 
> > While I think it is correct (at least up to the L2 CAT additions), it still
> > seems fragile to me to use != here (effectively allowing any other type to
> > come back). Couldn't props gain a field indicating the permitted alternative
> > type?
> > 
> Thanks for the good idea. Will add 'enum psr_feat_type alt_type;' in props
> to handle such case.
> 
> > >          {
> > >              val[i] = new_val;
> > > -            return 0;
> > > +            ret = 0;
> > >          }
> > 
> > Wouldn't it be better to return -EINVAL in a to be added else branch here
> > and ...
> > 
After reading codes again, I think we cannot return -EINVAL in else branch here.
E.g. for CDP, user wants to set CODE. Then, the 'type' is CODE. At the first
iteration, the props->type[0] is DATA which does not match 'type'. But we cannot
return error here. We should iterate next 'type[]'.

After iterating all type[], if we still do not find matching one, return the
error back. So, I use 'ret' here.

> > >      }
> > >  
> > > -    return -EINVAL;
> > > +    return ret;
> > >  }
> > 
> > ... to return zero here?
> > 
> Sure.
> 
> > Jan
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > https://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel

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