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 00:42:22, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/14/17 3:26 AM >>>
> > +static void l3_cdp_write_msr(unsigned int cos, uint32_t val, enum cbm_type 
> > type)
> > +{
> > +    wrmsrl(((type == PSR_CBM_TYPE_L3_DATA) ?
> > +            MSR_IA32_PSR_L3_MASK_DATA(cos) :
> > +            MSR_IA32_PSR_L3_MASK_CODE(cos)),
> > +           val);
> > +}
> > +
> >  static const struct feat_props l3_cdp_props = {
> >      .cos_num = 2,
> >      .type[0] = PSR_CBM_TYPE_L3_DATA,
> >      .type[1] = PSR_CBM_TYPE_L3_CODE,
> >      .get_feat_info = l3_cdp_get_feat_info,
> > +    .write_msr = l3_cdp_write_msr,
> 
> Adding this hook only now means the earlier CDP patches must not be applied on
> their own. You should state this prominently (in the patch introducing
> l3_cdp_props) for whoever is going to eventually commit (parts of) this 
> series.
> 
Ok, I will highlight it in CDP init patch. Also apply it to L2 CAT patches.

> > @@ -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 ...
> 
> >      }
> >  
> > -    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