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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.17 at 10:22, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-06-06 01:48:13, Jan Beulich wrote:
>> >>> On 02.06.17 at 09:59, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-05-31 03:44:31, Jan Beulich wrote:
>> >> >>> On 03.05.17 at 10:44, <yi.y.sun@xxxxxxxxxxxxxxx> 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.

Jan


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