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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.10.17 at 10:41, <yi.y.sun@xxxxxxxxxxxxxxx> 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.

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

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

Jan


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