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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

> +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().

> +        mod = *thrtl % (100 - feat->mba.thrtl_max);
> +        *thrtl -= mod;

Do you really need the intermediate variable?

> +    }
> +    else
> +    {
> +        /* Not power of 2. */
> +        if ( *thrtl & (*thrtl - 1) )
> +            *thrtl = *thrtl & (1 << (flsl(*thrtl) - 1));

&= or even simply =.

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