Re: [PATCH v12 04/23] x86: refactor psr: L3 CAT: implement main data structures, CPU init and free flows.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-06-28 01:12:23, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:25 AM >>>
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -13,16 +13,112 @@
> >   * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> >   * more details.
> >   */
> > -#include <xen/init.h>
> >  #include <xen/cpu.h>
> >  #include <xen/err.h>
> > +#include <xen/init.h>
> >  #include <xen/sched.h>
> >  #include <asm/psr.h>
> >  
> > +/*
> > + * Terminology:
> > + * - CAT         Cache Allocation Technology
> > + * - CBM         Capacity BitMasks
> > + * - CDP         Code and Data Prioritization
> > + * - CMT         Cache Monitoring Technology
> > + * - COS/CLOS    Class of Service. Also mean COS registers.
> > + * - COS_MAX     Max number of COS for the feature (minus 1)
> > + * - MSRs        Machine Specific Registers
> > + * - PSR         Intel Platform Shared Resource
> > + */
> > +
> >  #define PSR_CMT        (1<<0)
> >  #define PSR_CAT        (1<<1)
> >  #define PSR_CDP        (1<<2)
> >  
> > +#define CAT_CBM_LEN_MASK 0x1f
> > +#define CAT_COS_MAX_MASK 0xffff
> > +
> > +/*
> > + * Per SDM chapter 'Cache Allocation Technology: Cache Mask Configuration',
> > + * the MSRs ranging from 0C90H through 0D0FH (inclusive), enables support for
> > + * up to 128 L3 CAT Classes of Service. The COS_ID=[0,127].
> > + *
> > + * The MSRs ranging from 0D10H through 0D4FH (inclusive), enables support for
> > + * up to 64 L2 CAT COS. The COS_ID=[0,63].
> > + *
> > + * So, the maximum COS register count of one feature is 128.
> > + */
> > +#define MAX_COS_REG_CNT  128
> > +
> > +/*
> > + * Every PSR feature uses some COS registers for each COS ID, e.g. CDP uses 2
> > + * COS registers (DATA and CODE) for one COS ID, but CAT uses 1 COS register.
> > + * We use below macro as the max number of COS registers used by all features.
> > + * So far, it is 2 which means CDP's COS registers number.
> > + */
> > +#define PSR_MAX_COS_NUM 2
> > +
> > +enum psr_feat_type {
> > +    PSR_SOCKET_L3_CAT,
> > +    PSR_SOCKET_FEAT_NUM,
> > +};
> 
> For identifiers going into a header, PSR_ and psr_ disambiguation prefixes
> are certainly necessary. For everything being declared / defined for just this
> one file this isn't really necessary imo (the SOCKET_ part above I'd then also
> be uncertain about). The main thing, however, is the inconsistency here: Above
> you have MAX_COS_REG_CNT and PSR_MAX_COS_NUM. I would really prefer if both
> prefix and suffix wise these were consistent.
> 
> > +static void cat_init_feature(const struct cpuid_leaf *regs,
> > +                             struct feat_node *feat,
> > +                             struct psr_socket_info *info,
> > +                             enum psr_feat_type type)
> > +{
> > +    /* No valid value so do not enable feature. */
> > +    if ( !regs->a || !regs->d )
> > +        return;
> > +
> > +    feat->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;
> > +    feat->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->cbm_len);
> 
> The word "reserved" in the comment is a little unfortunate - if there was
> anything reserved in a register, I'd expect the respective parts to either
> not be writable, or to fault upon write attempts. However, I think you mean
> that you reserve it for the described purpose. So perhaps "We reserve ..."?
> Also please have a blank before the opeing paren.
> 
> With all of the suggestion taken care of
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
Thank you! I will modify macros, enum identifiers added since this patch to
make them be consistent.

> With at least the comment adjusted (and considering how late I am with the
> other suggestions)
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Jan

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