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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-03-24 10:52:34, Jan Beulich wrote:
> >>> 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.
> 
Yes, you are right. It is indirectly included through 'sched.h'.

> > @@ -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.
> 
I remember somebody suggested me not to use constant but should define a
macro. As it is only used once, I will remove this and 'CDP_COS_NUM' in
later patch.

> > +/*
> > + * 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.
> 
Thanks for the suggestion! Will change it.

> > +/*
> > + * 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;
> 
> ?
> 
How about '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().
> 
Ok, will modify it.

> > +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.
> 
Will correct this, thanks!

> > +                             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).
> 
When all cpus on a socket are offline, the free_feature() is called to free
features resources so that the values saved in cos_reg_val[] are lost. When the
socket is online again, features are allocated again so that cos_reg_val[]
members are all initialized to 0. Only is cos_reg_val[0] initialized to default
value in this function in old codes.

But domain is still alive so that its cos id on the socket is kept. The
corresponding MSR value is kept too per test. To make cos_reg_val[] values be
same as HW to not to mislead user, we should read back the valid values on HW
into cos_reg_val[].

> > +    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();
> 
Thanks!

> > +    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.
> 
Will remove it.

> > +/* L3 CAT ops */
> > +static const struct feat_ops l3_cat_ops = {
> > +};
> 
> Leaving an already declared function pointer as NULL? Please don't.
> 
Ok, will consider to move it and below code into later patch.
    feat->ops = l3_cat_ops;

> >  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?
> 
Hmm, no need indeed. Will remove this.

> > +    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.
> 
Got it, thanks!

> Jan

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