Re: [Xen-devel] [PATCH v10 05/25] x86: refactor psr: L3 CAT: implement CPU init and free flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> @@ -76,7 +79,7 @@ struct feat_node {
>       *
>       * Feature independent HW info and common values are also defined in it.
>       */
> -    const struct feat_props {
> +    struct feat_props {

As said before, the const here should stay. The init-time writing
to the structure can be done without going through this pointer.

> +static void free_socket_resources(unsigned int socket)
> +{
> +    unsigned int i;
> +    struct psr_socket_info *info = socket_info + socket;
> +
> +    if ( !info )
> +        return;
> +
> +    /*
> +     * Free resources of features. The global feature object, e.g. 
> feat_l3_cat,
> +     * may not be freed here if it is not added into array. It is simply 
> being
> +     * kept until the next CPU online attempt.
> +     */
> +    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> +    {
> +        if ( !info->features[i] )
> +            continue;
> +
> +        xfree(info->features[i]);
> +        info->features[i] = NULL;

There's no need for the if() here. And I'm sure this was pointed out
already (perhaps in a different context).

> +static bool feat_init_done(const struct psr_socket_info *info)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> +    {
> +        if ( !info->features[i] )
> +            continue;
> +
> +        return true;
> +    }
> +
> +    return false;
> +}

At the first glance this is a very strange function.

> +/* CAT common functions implementation. */
> +static void cat_init_feature(const struct cpuid_leaf *regs,
> +                             struct feat_node *feat,
> +                             struct psr_socket_info *info,
> +                             enum psr_feat_type type)
> +{
> +    unsigned int socket, i;
> +
> +    /* No valid value so do not enable feature. */
> +    if ( !regs->a || !regs->d )
> +        return;
> +
> +    feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;
> +    feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);
> +
> +    switch ( type )
> +    {
> +    case PSR_SOCKET_L3_CAT:
> +        /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). 
> */
> +        feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len);
> +
> +        /*
> +         * To handle cpu offline and then online case, we need restore MSRs 
> to
> +         * default values.
> +         */
> +        for ( i = 1; i <= feat->props->cos_max; i++ )
> +        {
> +            wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]);
> +            feat->cos_reg_val[i] = feat->cos_reg_val[0];
> +        }

I continue to have difficulty with this: Why is offline-then-online
any different from first-time-online? Why wouldn't setting the
registers to their intended values not be taken care of by
context switch code, once vCPU-s get scheduled onto the newly
onlined CPU?

> +        break;
> +
> +    default:
> +        return;
> +    }
> +
> +    /* Add this feature into array. */
> +    info->features[type] = feat;
> +
> +    socket = cpu_to_socket(smp_processor_id());

No need for this variable, and definitely no need to do the
assignment ahead of ...

> +    if ( !opt_cpu_info )
> +        return;

... this.

>  static void psr_cpu_init(void)
>  {
> +    struct psr_socket_info *info;
> +    unsigned int socket;
> +    unsigned int cpu = smp_processor_id();
> +    struct feat_node *feat;
> +    struct cpuid_leaf regs;
> +
> +    if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) )
> +        goto assoc_init;
> +
> +    if ( boot_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT )
> +    {
> +        setup_clear_cpu_cap(X86_FEATURE_PQE);
> +        goto assoc_init;
> +    }
> +
> +    socket = cpu_to_socket(cpu);
> +    info = socket_info + socket;
> +    if ( feat_init_done(info) )
> +        goto assoc_init;

Hmm, so you bail here if any of the features was already set up.
But you don't bail if none of the features were available as the
reason for the setup not having been done before. I think this
can be solved in a better way once we have the static props
array: You need to do anything here only if the props slot is not
NULL, but the feature slot is NULL.

In any event you intentions are likely easier to understand for
a reader if this single-use function was inlined here.

Jan

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