Re: [PATCH v7 08/16] x86: implement set value flow for MBA

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

On 17-10-13 09:56:14, Jan Beulich wrote:
> >>> On 13.10.17 at 10:41, <yi.y.sun@linux.intel.com> wrote:
> > @@ -274,16 +280,18 @@ static enum psr_feat_type psr_type_to_feat_type(enum psr_type type)
> >      return feat_type;
> >  }
> >  
> > -static bool psr_check_cbm(unsigned int cbm_len, unsigned long cbm)
> > +/* Implementation of allocation features' functions. */
> > +static bool cat_check_cbm(const struct feat_node *feat, uint32_t *val)
> >  {
> >      unsigned int first_bit, zero_bit;
> > +    unsigned int cbm_len = feat->cat.cbm_len;
> > +    unsigned long cbm = *val;
> 
> These are necessary changes.
> 
> > -    /* Set bits should only in the range of [0, cbm_len]. */
> > -    if ( cbm & (~0ul << cbm_len) )
> > -        return false;
> > -
> > -    /* At least one bit need to be set. */
> > -    if ( cbm == 0 )
> > +    /*
> > +     * Set bits should be only in the range of [0, cbm_len).
> > +     * And, at least one bit need to be set.
> > +     */
> > +    if ( (cbm & (~0ul << cbm_len)) || !cbm )
> 
> But all of this doesn't really belong here. I don't outright object to
> you leaving it the way it is, but I'd prefer if you dropped these
> changes, or moved them to a separate patch if you think this is
> worthwhile.
> 
Then, I would prefer to drop these changes.

> > @@ -501,6 +511,35 @@ static bool mba_get_feat_info(const struct feat_node *feat,
> >  static void mba_write_msr(unsigned int cos, uint32_t val,
> >                            enum psr_type type)
> >  {
> > +    wrmsrl(MSR_IA32_PSR_MBA_MASK(cos), val);
> > +}
> > +
> > +static bool mba_sanitize_thrtl(const struct feat_node *feat, uint32_t *thrtl)
> > +{
> > +    if ( *thrtl > feat->mba.thrtl_max )
> > +        return false;
> > +
> > +    /*
> > +     * Per SDM (chapter "Memory Bandwidth Allocation Configuration"):
> > +     * 1. Linear mode: In the linear mode the input precision is defined
> > +     *    as 100-(MBA_MAX). For instance, if the MBA_MAX value is 90, the
> > +     *    input precision is 10%. Values not an even multiple of the
> > +     *    precision (e.g., 12%) will be rounded down (e.g., to 10% delay
> > +     *    applied).
> > +     * 2. Non-linear mode: Input delay values are powers-of-two from zero
> > +     *    to the MBA_MAX value from CPUID. In this case any values not a
> > +     *    power of two will be rounded down the next nearest power of two.
> > +     */
> > +    if ( feat->mba.linear )
> > +        *thrtl -= *thrtl % (100 - feat->mba.thrtl_max);
> > +    else
> > +    {
> > +        /* Not power of 2. */
> > +        if ( *thrtl & (*thrtl - 1) )
> > +            *thrtl &= 1 << (flsl(*thrtl) - 1);
> 
> fls() will do now that the parameter type is uint32_t.
> 
Yes, you are right. Sorry for missing it.

> Also why do you think &= is better than plain = here?

Not better. Will change it to '='.

> 
> Jan

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