Re: [Xen-devel] [PATCH v8 04/24] x86: refactor psr: implement CPU init and free flow.

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Feb 15, 2017 at 04:49:19PM +0800, Yi Sun wrote:
> This patch implements the CPU init and free flow including L3 CAT
> initialization and feature list free.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>

Either you need to use a separate patch to move cpuid_count_leaf or you
should state it is moved in the commit message.

> +
> +/* Common functions. */
> +static void free_feature(struct psr_socket_info *info)
> +{
> +    struct feat_node *feat, *next;
> +
> +    if ( !info )
> +        return;
> +
> +    /*
> +     * Free resources of features. But we do not free global feature list
> +     * entry, like feat_l3_cat. Although it may cause a few memory leak,
> +     * it is OK simplify things.

I don't think it is OK to leak memory in the hypervisor in general.
Please specify why it is OK in this particular case in the comment.

> +     */
> +    list_for_each_entry_safe(feat, next, &info->feat_list, list)
> +    {
> +        if ( !feat )
> +            return;
> +
> +        __clear_bit(feat->feature, &info->feat_mask);
> +        list_del(&feat->list);
> +        xfree(feat);
> +    }
> +}
> +
> -static int psr_cpu_prepare(unsigned int cpu)
> +static void cpu_init_work(void)
> +{
> +    struct psr_socket_info *info;
> +    unsigned int socket;
> +    unsigned int cpu = smp_processor_id();
> +    struct feat_node *feat;
> +    struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 };
> +
> +    if ( !cpu_has(&current_cpu_data, X86_FEATURE_PQE) )
> +        return;
> +    else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT )
> +    {
> +        __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability);
> +        return;
> +    }
> +
> +    socket = cpu_to_socket(cpu);
> +    info = socket_info + socket;
> +    if ( info->feat_mask )
> +        return;
> +
> +    INIT_LIST_HEAD(&info->feat_list);
> +    spin_lock_init(&info->ref_lock);
> +
> +    cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, &regs);
> +    if ( regs.b & PSR_RESOURCE_TYPE_L3 )
> +    {

You can move

   struct feat_node *feat

here.

> +        cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, &regs);
> +
> +        feat = feat_l3_cat;
> +        /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */
> +        feat_l3_cat = NULL;
> +        feat->ops = l3_cat_ops;
> +
> +        l3_cat_init_feature(regs, feat, info);
> +    }
> +}
> +
[...]
>  
> @@ -359,7 +528,7 @@ static int cpu_callback(
>      switch ( action )
>      {
>      case CPU_UP_PREPARE:
> -        rc = psr_cpu_prepare(cpu);
> +        rc = psr_cpu_prepare();
>          break;
>      case CPU_STARTING:
>          psr_cpu_init();
> @@ -388,10 +557,14 @@ static int __init psr_presmp_init(void)
>      if ( (opt_psr & PSR_CMT) && opt_rmid_max )
>          init_psr_cmt(opt_rmid_max);
>  
> -    psr_cpu_prepare(0);
> +    if ( opt_psr & PSR_CAT )
> +        init_psr();
> +
> +    if ( psr_cpu_prepare() )
> +        psr_free();
>  
>      psr_cpu_init();
> -    if ( psr_cmt_enabled() )
> +    if ( psr_cmt_enabled() || socket_info )

Why not have psr_cat_enabled() here?

Wei.

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