Re: [Xen-devel] [PATCH v4 07/15] x86: implement set value flow for MBA

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.10.17 at 06:48, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-10-03 23:59:46, Jan Beulich wrote:
>> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 09/29/17 4:58 AM >>>
>> >On 17-09-28 05:36:11, Jan Beulich wrote:
>> >> >>> On 23.09.17 at 11:48, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> >> > This patch implements set value flow for MBA including its callback
>> >> > function and domctl interface.
>> >> > 
>> >> > It also changes the memebers in 'cos_write_info' to transfer the
>> >> > feature array, feature properties array and value array. Then, we
>> >> > can write all features values on the cos id into MSRs.
>> >> > 
>> >> > Because multiple features may co-exist, we need handle all features to 
>> >> > write
>> >> > values of them into a COS register with new COS ID. E.g:
>> >> > 1. L3 CAT and MBA co-exist.
>> >> > 2. Dom1 and Dom2 share a same COS ID (2). The L3 CAT CBM of Dom1 is 
>> >> > 0x1ff,
>> >> >    the MBA Thrtle of Dom1 is 0xa.
>> >> > 3. User wants to change MBA Thrtl of Dom1 to be 0x14. Because COS ID 2 
>> >> > is
>> >> >    used by Dom2 too, we have to pick a new COS ID 3. The values of Dom1 
>> >> > on
>> >> >    COS ID 3 are all default values as below:
>> >> >            ---------
>> >> >            | COS 3 |
>> >> >            ---------
>> >> >    L3 CAT  | 0x7ff |
>> >> >            ---------
>> >> >    MBA     | 0x0   |
>> >> >            ---------
>> >> > 4. After setting, the L3 CAT CBM value of Dom1 should be kept and the 
>> >> > new MBA
>> >> >    Thrtl is set. So, the values on COS ID 3 should be below.
>> >> >            ---------
>> >> >            | COS 3 |
>> >> >            ---------
>> >> >    L3 CAT  | 0x1ff |
>> >> >            ---------
>> >> >    MBA     | 0x14  |
>> >> >            ---------
>> >> > 
>> >> > So, we should write all features values into their MSRs. That requires 
>> >> > the
>> >> > feature array, feature properties array and value array are input.
>> >> 
>> >> How is this last aspect (and the respective changes) related to MBA?
>> >> I.e. why isn't this needed with the (also independent but possibly
>> >> co-existing) L2/L3 CAT features?
>> >> 
>> >I tried to introduce this in L2 CAT patch set but did not succeed. As there 
>> >is
>> >no HW that L2 CAT and L3 CAT co-exist so far, I did not insist on this.
>> 
>> Hmm, I'm afraid this wasn't then made clear enough to understand. I would
>> certainly not have been against something that could in theory occur with
>> L2/L3 CAT alone. In any event this means you don't want to mix this into this
>> MBA specific change here.
>> 
> Anyway, I think you suggest to split this as a new patch, right?

Yes indeed.

>> >> >  static void do_write_psr_msrs(void *data)
>> >> >  {
>> >> >      const struct cos_write_info *info = data;
>> >> > -    struct feat_node *feat = info->feature;
>> >> > -    const struct feat_props *props = info->props;
>> >> > -    unsigned int i, cos = info->cos, cos_num = props->cos_num;
>> >> > +    unsigned int i, index = 0, cos = info->cos;
>> >> > +    const uint32_t *val_array = info->val;
>> >> >  
>> >> > -    for ( i = 0; i < cos_num; i++ )
>> >> > +    for ( i = 0; i < ARRAY_SIZE(feat_props); i++ )
>> >> >      {
>> >> > -        if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] )
>> >> > +        struct feat_node *feat = info->features[i];
>> >> > +        const struct feat_props *props = info->props[i];
>> >> > +        unsigned int cos_num, j;
>> >> > +
>> >> > +        if ( !feat || !props )
>> >> > +            continue;
>> >> > +
>> >> > +        cos_num = props->cos_num;
>> >> > +        if ( info->array_len < index + cos_num )
>> >> > +            return;
>> >> > +
>> >> > +        for ( j = 0; j < cos_num; j++ )
>> >> >          {
>> >> > -            feat->cos_reg_val[cos * cos_num + i] = info->val[i];
>> >> > -            props->write_msr(cos, info->val[i], props->type[i]);
>> >> > +            if ( feat->cos_reg_val[cos * cos_num + j] != 
>> >> > val_array[index + j] )
>> >> > +                feat->cos_reg_val[cos * cos_num + j] =
>> >> > +                    props->write_msr(cos, val_array[index + j], 
>> >> > props->type[j]);
>> >> 
>> >> This renders partly useless the check: If hardware can alter the
>> >> value, repeatedly requesting the same value to be written will
>> >> no longer guarantee the MSR write to be skipped. If hardware
>> >> behavior can't be predicted you may want to consider recording
>> >> both the value in found by reading back the register written and
>> >> the value that was written - a match with either would eliminate
>> >> the need to do the write.
>> >> 
>> >The hardware behavior is explicitly defined by SDM and mentioned in
>> >'xl-psr.markdown' and 'intel_psr_mba.pandoc'. User should know that HW
>> >can alter MBA value if the value is not valid.
>> 
>> So if hardware behavior is fully defined, why don't you pre-adjust what is
>> to be written to the value hardware would alter it to?
>> 
> In previous version of MBA patch set, I pre-adjust the value in 
> 'mba_check_thrtl'.
> But Roger did not like that. So, the pre-adjust codes are removed.

Could you point me at or repeat the reason(s) of his dislike?

>> >This check is not only for MBA but also for CAT features that the HW
>> >cannot alter CAT value.
>> 
>> I don't understand this part.
>> 
> I mean the check here are for all features so we cannot remove it.

I _still_ don't understand: If the check can't be removed (even
without MBA in mind), then the implication would be that the
code prior to this series is buggy. In which case I'd expect you to
submit a standalone bug fix, rather than mixing the fix into here.

>> > Although this check is not a critical check,
>> >it can prevent some non-necessary MSR write.
>> 
>> That's my point - while previously all unnecessary writes were avoided,
>> you now avoid only some.
>> 
> Without the pre-adjust codes in 'mba_check_thrtl', if user inputs value, e.g.
> 11/22/33/..., this check cannot prevent the write action. So, only some can
> be avoided in current codes.

Right. If it's worthwhile avoiding the writes, all of them should be
avoided when the resulting value isn't going to change. Otherwise
the write avoidance logic can/should be dropped altogether.

Jan

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