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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> 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.

>> >  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?

>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.

> 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.

>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.

Jan


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