Re: [Xen-devel] [PATCH v4 04/15] x86: implement data structure and CPU init flow for MBA

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sat, Sep 23, 2017 at 09:48:13AM +0000, Yi Sun wrote:
> This patch implements main data structures of MBA.
> 
> Like CAT features, MBA HW info has cos_max which means the max thrtl
> register number, and thrtl_max which means the max throttle value
> (delay value). It also has a flag to represent if the throttle
> value is linear or not.
> 
> One thrtl register of MBA stores a throttle value for one or more
> domains. The throttle value means the delay between L2 cache and next
> cache level.
> 
> This patch also implements init flow for MBA and register stub
> callback functions.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

With two nits below.

> @@ -332,20 +355,58 @@ static int cat_init_feature(const struct cpuid_leaf 
> *regs,
>      }
>  
>      default:
> -        return -ENOENT;
> +        return false;
>      }
>  
>      /* Add this feature into array. */
>      info->features[type] = feat;
>  
>      if ( !opt_cpu_info )
> -        return 0;
> +        return true;
>  
>      printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
>             cat_feat_name[type], cpu_to_socket(smp_processor_id()),
> -           feat->cos_max, feat->cbm_len);
> +           feat->cos_max, feat->cat.cbm_len);

I would rather do:

if ( opt_cpu_info )
    printk(...);

return true;

So that the function has a single return path for the success case.

>  
> -    return 0;
> +    return true;
> +}
> +
> +static bool mba_init_feature(const struct cpuid_leaf *regs,
> +                            struct feat_node *feat,
> +                            struct psr_socket_info *info,
> +                            enum psr_feat_type type)
> +{
> +    /* No valid value so do not enable feature. */
> +    if ( !regs->a || !regs->d || type != FEAT_TYPE_MBA )
> +        return false;
> +
> +    feat->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);
> +    if ( feat->cos_max < 1 )
> +        return false;
> +
> +    feat->mba.thrtl_max = (regs->a & MBA_THRTL_MAX_MASK) + 1;
> +
> +    if ( regs->c & MBA_LINEAR_MASK )
> +    {
> +        feat->mba.linear = true;
> +
> +        if ( feat->mba.thrtl_max >= 100 )
> +            return false;
> +    }
> +
> +    wrmsrl(MSR_IA32_PSR_MBA_MASK(0), 0);
> +
> +    /* Add this feature into array. */
> +    info->features[type] = feat;
> +
> +    if ( !opt_cpu_info )
> +        return true;
> +
> +    printk(XENLOG_INFO "MBA: enabled on socket %u, cos_max:%u, thrtl_max:%u, 
> linear:%u.\n",
                         ^ newline.

Thanks, Roger.

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