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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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

it is OK as it simplifies things.
> +     */
> +    list_for_each_entry_safe(feat, next, &info->feat_list, list)
> +    {
> +        __clear_bit(feat->feature, &info->feat_mask);
> +        list_del(&feat->list);
> +        xfree(feat);
> +    }
> +}
> +
> +/* L3 CAT functions implementation. */
> +static void l3_cat_init_feature(struct cpuid_leaf regs,
> +                                struct feat_node *feat,
> +                                struct psr_socket_info *info)
> +{
> +    struct psr_cat_hw_info l3_cat;

Having experienced a few times myself forgetting to set
_all_ the entries a structure allocated on the stack and then
debugging for hours - perhaps you could do:

l3_cat = { };

Which forces the compiler to initialize _all_ the values to zero.
> +    unsigned int socket;
> +
> +    /* No valid value so do not enable feature. */
> +    if ( !regs.a || !regs.b )

You use regs.d below. Would it make sense to check for that value as
well?

Or is a value of 0 for cox_max OK? I would think so, but not exactly
sure.

> +        return;
> +
> +    l3_cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1;
> +    l3_cat.cos_max = min(opt_cos_max, regs.d & CAT_COS_MAX_MASK);
> +
> +    /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */
> +    feat->cos_reg_val[0] = (1ull << l3_cat.cbm_len) - 1;
> +
> +    feat->feature = PSR_SOCKET_L3_CAT;
> +    ASSERT(!test_bit(PSR_SOCKET_L3_CAT, &info->feat_mask));
> +    __set_bit(PSR_SOCKET_L3_CAT, &info->feat_mask);
> +
> +    feat->info.l3_cat_info = l3_cat;
> +
> +    info->nr_feat++;
> +
> +    /* Add this feature into list. */
> +    list_add_tail(&feat->list, &info->feat_list);
> +
> +    socket = cpu_to_socket(smp_processor_id());
> +    if ( !opt_cpu_info )
> +        return;
> +
> +    printk(XENLOG_INFO "L3 CAT: enabled on socket %u, cos_max:%u, 
> cbm_len:%u\n",
> +           socket, feat->info.l3_cat_info.cos_max,
> +           feat->info.l3_cat_info.cbm_len);
> +}
> +
> +static const struct feat_ops l3_cat_ops = {
> +};
> +
>  static void __init parse_psr_bool(char *s, char *value, char *feature,
>                                    unsigned int mask)
>  {
> @@ -180,6 +257,9 @@ static void __init parse_psr_param(char *s)
>          if ( val_str && !strcmp(s, "rmid_max") )
>              opt_rmid_max = simple_strtoul(val_str, NULL, 0);
>  
> +        if ( val_str && !strcmp(s, "cos_max") )
> +            opt_cos_max = simple_strtoul(val_str, NULL, 0);
> +
>          s = ss + 1;
>      } while ( ss );
>  }
> @@ -335,18 +415,107 @@ void psr_domain_free(struct domain *d)
>      psr_free_rmid(d);
>  }
>  
> +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 )
> +    {
> +        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);
> +    }
> +}
> +
> +static void cpu_fini_work(unsigned int cpu)
> +{
> +    unsigned int socket = cpu_to_socket(cpu);
> +
> +    if ( !socket_cpumask[socket] || cpumask_empty(socket_cpumask[socket]) )

I fear I don't understand this.

Looking at 65a399ac it says:

+    .notifier_call = cpu_callback,
+    /*
+     * Ensure socket_cpumask is still valid in CPU_DEAD notification
+     * (E.g. our CPU_DEAD notification should be called ahead of
+     * cpu_smpboot_free).

Which means that socket_cpumask[socket] should have an value.

In fact cpumask_any(socket_cpumask[socket]) will return true at this
point.

Which means that code above gets called if this psr_cpu_fini is called
_after_ cpu_smpboot_free (b/c socket_cpumask[socket] will be NULL), see
cpu_smpboot_free.

But with .priority being -1 that won't happen.

But lets ignore that, lets say it does get called _after_
cpu_smpboot_free. In which case the first part of the 'if' condition
is true and we end up doing: cpumask_empty(NULL) which will result
in an page fault.

I think this is a bug.

Perhaps it should just be:

  /* .priority is = -1 and we MUST run before cpu_smpboot_free. */
  ASSERT(socket_cpumask[socket] && cpumask_any(socket_cpumask[socket]));

But since that is only compiled on debug=y it may be better to just
change that ASSERT to an 'if' with the 'else' hitting an ASSERT(1)? 

Like so:

if ( socket_cpumask[socket] && cpumask_any(socket_cpumask[socket] )
        free_feature(socket_info + socket);
else
        ASSERT(0);

?

But maybe I am reading the code wrong?

> +    {
> +        free_feature(socket_info + socket);
> +    }
> +}
> +
> +static void __init init_psr(void)

Why don't we make this return an value? And then the init
code(psr_presmp_init) can just return an error?


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