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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 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@xxxxxxxx>

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

Jan

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