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

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

On 17-10-05 02:55:17, Jan Beulich wrote:
> >>> 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:
[...]

> >> >> >  {
> >> >> >      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?
> 
Roger has replied.

> >> >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.
> 
Ok, I will send out a stand alone patch to fix this.

> >> > 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.
> 
Per discussion in other mails, I think I will restore codes in 
'mba_check_thrtl'.

> Jan

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