On 17-10-03 23:59:46, Jan Beulich wrote:
> >>> Yi Sun <yi.y....@linux.intel.com> 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....@linux.intel.com> 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?
> >> > 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.
> >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.
> > 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.
> >If you still think we should handle the case that user inputs an invalid
> >value every time, I think we can restore the codes in 'mba_check_thrtl'
> >to change invalid value to valid one, then insert the valid value into
> >val_array. Then, this check is always valid.
>
> I don't think I've asked to deal with "invalid" values here (which should be
> rejected anyway, but that's a different topic). Values adjusted by hardware
> don't fall into the "invalid" category for me.
>
If the pre-adjust codes in 'mba_check_thrtl' are restored, all values written
to HW are valid.
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
