Re: [Xen-devel] [PATCH v6 08/16] x86: implement set value flow for MBA

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-10-11 07:38:52, Jan Beulich wrote:
> >>> On 08.10.17 at 09:23, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -138,6 +138,12 @@ static const struct feat_props {
> >  
> >      /* write_msr is used to write out feature MSR register. */
> >      void (*write_msr)(unsigned int cos, uint32_t val, enum psr_type type);
> > +
> > +    /*
> > +     * check_val is used to check if input val fulfills SDM requirement.
> > +     * Change it to valid value if SDM allows.
> > +     */
> > +    bool (*check_val)(const struct feat_node *feat, unsigned long *val);
> 
> I'm pretty sure I've said so before - "check" to me implies all r/o
> inputs. Perhaps sanitize_val() or even just sanitize()?
> 
> And why unsigned long when the only caller has a uint32_t in its
> hands?
> 
To be compatible with cat_check_cbm (old name is 'psr_check_cbm' in L2 series),
the last parameter type is 'unsigned long'. We have discussed it in L2 patch set
v9, patch 10.

> > @@ -274,30 +280,30 @@ 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, unsigned long *cbm)
> >  {
> >      unsigned int first_bit, zero_bit;
> > +    unsigned int cbm_len = feat->cat.cbm_len;
> >  
> > -    /* 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 only in the range of [0, cbm_len].
> 
> As you alter the comment anyway, please also add the missing "be".
> Also - isn't the upper bound of the range exclusive, i.e. shouldn't
> this be [0, cbm_len)?
> 
Thanks!

> > +     * And, at least one bit need to be set.
> > +     */
> > +    if ( *cbm & (~0ul << cbm_len) || *cbm == 0 )
> 
> Parentheses missing for the left side operand of || and if you omit
> != 0 on the left part (which I appreciate) please also use ! instead
> of == 0 on the right side.
> 
Got it.

> > +static bool mba_check_thrtl(const struct feat_node *feat, unsigned long 
> > *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 )
> > +    {
> > +        unsigned int mod;
> > +
> > +        if ( feat->mba.thrtl_max >= 100 )
> > +            return false;
> 
> Don't you check this right after collecting CPUID output? If so,
> this should be at most an ASSERT().
> 
Yes, I have checked this in mba_init_feature. Will remove this check.

> > +        mod = *thrtl % (100 - feat->mba.thrtl_max);
> > +        *thrtl -= mod;
> 
> Do you really need the intermediate variable?
> 
Will remove 'mod'.

> > +    }
> > +    else
> > +    {
> > +        /* Not power of 2. */
> > +        if ( *thrtl & (*thrtl - 1) )
> > +            *thrtl = *thrtl & (1 << (flsl(*thrtl) - 1));
> 
> &= or even simply =.
> 
Ok, thanks!

> > @@ -950,6 +997,7 @@ static int insert_val_into_array(uint32_t val[],
> >      const struct feat_node *feat;
> >      const struct feat_props *props;
> >      unsigned int i;
> > +    unsigned long check_val = new_val;
> >      int ret;
> >  
> >      ASSERT(feat_type < FEAT_TYPE_NUM);
> > @@ -974,9 +1022,11 @@ static int insert_val_into_array(uint32_t val[],
> >      if ( array_len < props->cos_num )
> >          return -ENOSPC;
> >  
> > -    if ( !psr_check_cbm(feat->cat.cbm_len, new_val) )
> > +    if ( !props->check_val(feat, &check_val) )
> >          return -EINVAL;
> >  
> > +    new_val = check_val;
> 
> When the function pointer's parameter changes to uint32_t *
> you won't need the intermediate variable anymore afaict.
> 
> Jan

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