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

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

On 17-10-03 23:59:46, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@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.sun@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