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

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

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.

> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -137,7 +137,10 @@ static const struct feat_props {
> >                            uint32_t data[], unsigned int array_len);
> >  
> >      /* write_msr is used to write out feature MSR register. */
> > -    void (*write_msr)(unsigned int cos, uint32_t val, enum psr_type type);
> > +    uint32_t (*write_msr)(unsigned int cos, uint32_t val, enum psr_type type);
> 
> Again the type change would better be a prereq patch, to allow the
> focus here to be on MBA.
> 
Sure, will move it to a new  patch.

> > @@ -502,9 +514,23 @@ static bool mba_get_feat_info(const struct feat_node *feat,
> >      return true;
> >  }
> >  
> > -static void mba_write_msr(unsigned int cos, uint32_t val,
> > -                          enum psr_type type)
> > +static uint32_t mba_write_msr(unsigned int cos, uint32_t val,
> > +                              enum psr_type type)
> > +{
> > +    wrmsrl(MSR_IA32_PSR_MBA_MASK(cos), val);
> > +
> > +    /* Read actual value set by hardware. */
> > +    rdmsrl(MSR_IA32_PSR_MBA_MASK(cos), val);
> > +
> > +    return val;
> > +}
> > +
> > +static bool mba_check_thrtl(const struct feat_node *feat, unsigned long thrtl)
> >  {
> > +    if ( thrtl > feat->mba.thrtl_max )
> > +        return false;
> > +
> > +    return true;
> 
> This can be had with a single return statement.
> 
Sure.

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

This check is not only for MBA but also for CAT features that the HW
cannot alter CAT value. Although this check is not a critical check,
it can prevent some non-necessary MSR write.

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.

> Jan

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