Re: [Xen-devel] [PATCH v8 13/24] x86: refactor psr: implement CPU init and free flow for CDP.

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Feb 15, 2017 at 04:49:28PM +0800, Yi Sun wrote:
> This patch implements the CPU init and free flow for CDP including L3 CDP
> initialization callback function.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> ---
>  xen/arch/x86/psr.c | 104 
> +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 98 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 82bb8fe..4c08779 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -97,6 +97,7 @@ struct psr_cat_hw_info {
>  struct feat_hw_info {
>      union {
>          struct psr_cat_hw_info l3_cat_info;
> +        struct psr_cat_hw_info l3_cdp_info;
>      };
>  };
>  
> @@ -195,6 +196,22 @@ struct feat_node {
>      struct list_head list;
>  };
>  
> +/*
> + * get_data - get DATA COS register value from input COS ID.
> + * @feat:        the feature list entry.
> + * @cos:         the COS ID.
> + */
> +#define get_cdp_data(feat, cos)                  \
> +            ( feat->cos_reg_val[cos * 2] )

This should be:

((feat)->cos_reg_val[(cos) * 2])

And the same treatment should be applied to the macro below.

> +
> +/*
> + * get_cdp_code - get CODE COS register value from input COS ID.
> + * @feat:        the feature list entry.
> + * @cos:         the COS ID.
> + */
> +#define get_cdp_code(feat, cos)                  \
> +            ( feat->cos_reg_val[cos * 2 + 1] )
> +
>  struct psr_assoc {
>      uint64_t val;
>      uint64_t cos_mask;
> @@ -217,6 +234,7 @@ static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
>   * cpu_add_remove_lock spinlock.
>   */
>  static struct feat_node *feat_l3_cat;
> +static struct feat_node *feat_l3_cdp;
>  
>  /* Common functions. */
>  static void free_feature(struct psr_socket_info *info)
> @@ -457,6 +475,63 @@ static const struct feat_ops l3_cat_ops = {
>      .write_msr = l3_cat_write_msr,
>  };
>  
> +/* L3 CDP functions implementation. */
> +static void l3_cdp_init_feature(struct cpuid_leaf regs,
> +                                struct feat_node *feat,
> +                                struct psr_socket_info *info)
> +{
> +    struct psr_cat_hw_info l3_cdp = { };
> +    unsigned int socket;
> +    uint64_t val;
> +
> +    /* No valid value so do not enable feature. */
> +    if ( !regs.a || !regs.d )
> +        return;
> +
> +    l3_cdp.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1;
> +    /* Cut half of cos_max when CDP is enabled. */
> +    l3_cdp.cos_max = min(opt_cos_max, regs.d & CAT_COS_MAX_MASK) >> 1;
> +
> +    /* cos=0 is reserved as default cbm(all ones). */
> +    get_cdp_code(feat, 0) =
> +                 (1ull << l3_cdp.cbm_len) - 1;

I think that all those ull sufixes should be turned into uint64_t casts,
because that's the type that you are actually using. Or else just use ul, which
is the same and shorter.

Roger.

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