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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -18,6 +18,7 @@
>  #include <xen/init.h>
>  #include <xen/sched.h>
>  #include <asm/psr.h>
> +#include <asm/x86_emulate.h>

I'm pretty sure you don't need this. If anything you need
processor.h (as that's where the previous patch put
cpuid_count_leaf()), but I'm rather convinced that the header was
already included indirectly at this point.

> @@ -46,6 +50,9 @@
>   */
>  #define MAX_COS_REG_CNT  128
>  
> +/* CAT features use 1 COS register in one access. */
> +#define CAT_COS_NUM      1

With it being stored into the feature node now I don't see why you
need this constant anymore. And indeed it's being used exactly
once.

> @@ -126,11 +133,110 @@ struct psr_assoc {
>  
>  struct psr_cmt *__read_mostly psr_cmt;
>  
> +static struct psr_socket_info *__read_mostly socket_info;
> +
>  static unsigned int opt_psr;
>  static unsigned int __initdata opt_rmid_max = 255;
> +static unsigned int __read_mostly opt_cos_max = MAX_COS_REG_CNT;
>  static uint64_t rmid_mask;
>  static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
>  
> +/*
> + * Declare global feature node for every feature to facilitate the feature
> + * array creation. It is used to transiently store a spare node.
> + */
> +static struct feat_node *feat_l3_cat;
> +
> +/* Common functions */
> +#define cat_default_val(len)                 \
> +            ( (uint32_t)((1ul << len) - 1) )

Pretty odd construct, which I guess you use to avoid the
undefined-ness when len == 32. But this can be had without
extra cast, assuming len is in [1,32]:

#define cat_default_val(len) (0xffffffff >> (32 - (len)))

Also - stray blanks and missing parentheses around the use of macro
parameter.

> +/*
> + * Use this function to check if any allocation feature has been enabled
> + * in cmdline.
> + */
> +static bool psr_alloc_feat_enabled(void)
> +{
> +    return ((!socket_info) ? false : true );

Stray parentheses (all of them actually) and blank. Even more, why
not simply

    return socket_info;

?

> +static void free_feature(struct psr_socket_info *info)
> +{
> +    unsigned int i;
> +
> +    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;
> +        __clear_bit(i, &info->feat_mask);
> +    }
> +}

What the function does suggests its name ought to be
free_features().

> +static void cat_init_feature(struct cpuid_leaf regs,

I'm sure I've asked before to not pass structures by value. And
once you switch to a pointer, please don't forget to constify it.

> +                             struct feat_node *feat,
> +                             struct psr_socket_info *info,
> +                             enum psr_feat_type type)
> +{
> +    unsigned int socket, i;
> +    struct psr_cat_hw_info cat = { };
> +    uint64_t val;
> +
> +    /* No valid value so do not enable feature. */
> +    if ( !regs.a || !regs.d )
> +        return;
> +
> +    cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1;
> +    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] = cat_default_val(cat.cbm_len);
> +    /*
> +     * To handle cpu offline and then online case, we need read MSRs back to
> +     * save values into cos_reg_val array.
> +     */
> +    for ( i = 1; i <= cat.cos_max; i++ )
> +    {
> +        rdmsrl(MSR_IA32_PSR_L3_MASK(i), val);
> +        feat->cos_reg_val[i] = (uint32_t)val;
> +    }

You mention this in the changes done, but I don't understand why
you do this. What meaning to these values have to you? If you
want hardware and cached values to match up, the much more
conventional way of enforcing this would be to write the values
you actually want (normally all zero).

> +    feat->info.cat_info = cat;
> +    feat->cos_num = CAT_COS_NUM;
> +
> +    /* Add this feature into array. */
> +    info->features[type] = feat;
> +
> +    ASSERT(!test_bit(type, &info->feat_mask));
> +    __set_bit(type, &info->feat_mask);

    if ( __test_and_set_bit(type, &info->feat_mask) )
        ASSERT_UNREACHABLE();

> +    socket = cpu_to_socket(smp_processor_id());
> +    if ( !opt_cpu_info )
> +        return;
> +
> +    printk(XENLOG_INFO "%s CAT: enabled on socket %u, cos_max:%u, 
> cbm_len:%u\n",
> +           ((type == PSR_SOCKET_L3_CAT) ? "L3" : "L2"),
> +           socket, feat->info.cat_info.cos_max,
> +           feat->info.cat_info.cbm_len);
> +
> +    return;

Pointless statement at end of function.

> +/* L3 CAT ops */
> +static const struct feat_ops l3_cat_ops = {
> +};

Leaving an already declared function pointer as NULL? Please don't.

>  static void psr_cpu_init(void)
>  {
> +    struct psr_socket_info *info;
> +    unsigned int socket, i;
> +    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 ( info->feat_mask )
> +        goto assoc_init;
> +
> +    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> +        info->features[i] = NULL;

You've xzalloc()ed this memory - why do you need this loop?

> +    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;
> +        feat_l3_cat = NULL;
> +        feat->ops = l3_cat_ops;
> +
> +        cat_init_feature(regs, feat, info, PSR_SOCKET_L3_CAT);
> +    }
> +
> +assoc_init:

Labels indented by at least on space please.

Jan

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