Re: [PATCH v11 15/23] x86: refactor psr: CDP: implement set value callback function.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-06-06 02:43:42, Jan Beulich wrote:
> >>> On 06.06.17 at 10:22, <yi.y.sun@linux.intel.com> wrote:
> > On 17-06-06 01:48:13, Jan Beulich wrote:
> >> >>> On 02.06.17 at 09:59, <yi.y.sun@linux.intel.com> wrote:
> >> > On 17-05-31 03:44:31, Jan Beulich wrote:
> >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote:
> >> >> > @@ -765,7 +777,8 @@ static int insert_val_into_array(uint32_t val[],
> >> >> >  
> >> >> >      /* Value setting position is same as feature array. */
> >> >> >      for ( i = 0; i < props->cos_num; i++ )
> >> >> > -        if ( type == props->type[i] )
> >> >> > +        if ( type == props->type[i] ||
> >> >> > +             (feat_type == PSR_SOCKET_L3_CDP && type == PSR_CBM_TYPE_L3) )
> >> >> 
> >> >> Didn't the earlier patch take care of doing this substitution? Non-
> >> >> feature-specific code clearly shouldn't have such special cases if
> >> >> at all avoidable.
> >> >> 
> >> > User can set both DATA and CODE to same value at same time with below 
> >> > command:
> >> > xl psr-cat-set dom_id 0x3ff
> >> > 
> >> > Because no '-c' or '-d' is input, the cbm type will be 'PSR_CBM_TYPE_L3'.
> >> > 
> >> > To handle this case, we have to add a special case here. If the cbm tyep is
> >> > 'PSR_CBM_TYPE_L3' and the feature type is CDP, we set both DATA and CODE. This
> >> > should be the simplest way to handle this case.
> >> 
> >> Simplest or not, it is not really appropriate to have such special cases
> >> here. Along the lines of the earlier abstractions I've recommended
> >> (and which, at least afaic, made the overall series quite a bit more
> >> comprehensible), please re-consider how this can be done without
> >> having special case logic here (I can't immediately suggest an option,
> >> I'm sorry).
> >> 
> > How about a callback function here to handle this insertion? For L3/L2 CAT,
> > use a function just to assign new_val to val[]. For CDP, in its callback
> > function, check 'type' to decide insert new_val to both DATA and CODE or just
> > one item according to type.
> 
> Well, I'm not sure what to say. The history of this series tells me
> that you suggesting a new callback is likely to be not better than
> having open coded special case logic here. IOW neither is a good
> (or should I say preferred) solution here, and I'm relatively
> certain (as I had been with all the other callbacks that are now
> gone) that there is a reasonably clean solution without either, by
> simply using suitable abstracted data structures. As expressed
> back then, even if I can't immediately suggest how to make this
> work, I'm still insisting that you at least try to come up with a
> clean solution here.
> 
Ok, I should think more. :)

Then, please check below solution.

This case only happens in CDP mode that the input cbm_type corresponds to L3
CAT but current feat_type is CDP. In all other modes, the input cbm_type
corresponds to its own mode. So, maybe we can implement codes as below.

//Add an input parameter 'bool strict'
static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type, boot strict)
{
...
    switch ( type )
    {
    case PSR_CBM_TYPE_L3:
        feat_type = PSR_SOCKET_L3_CAT;

        /*
         * If type is L3 CAT but we cannot find it in feat_props array,
         * try CDP.
         */
        if ( !feat_props[feat_type] && !strict )
            feat_type = PSR_SOCKET_L3_CDP;

        break;

    case PSR_CBM_TYPE_L3_DATA:
    case PSR_CBM_TYPE_L3_CODE:
        feat_type = PSR_SOCKET_L3_CDP;
        break;
...
}

//Input feat_type is PSR_SOCKET_L3_CDP, type is PSR_CBM_TYPE_L3
static int insert_val_into_array(feat_type, type)
{
...
    for ( i = 0; i < props->cos_num; i++ )
    {
        if ( type == props->type[i] ||
             feat_type != psr_cbm_type_to_feat_type(type, true) )
        {
            val[i] = new_val;
            ret = 0;
        }
    }

    return ret;
}

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