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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- 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;

Two union members of the same type? What's the union good for
then? (I've peeked ahead, and L2 CAT adds yet another one. A
strong sign that you've gone too far with what needs to be per-
feature vs what can be common.)

> @@ -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] )

Stray blanks inside parentheses. Macro parameters need to be
parenthesized.

> +/*
> + * 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] )

Same here.

> @@ -1213,12 +1288,21 @@ static void cpu_init_work(void)
>      {
>          cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, &regs);
>  
> -        feat = feat_l3_cat;
> -        /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */
> -        feat_l3_cat = NULL;
> -        feat->ops = l3_cat_ops;
> -
> -        l3_cat_init_feature(regs, feat, info);
> +        if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) &&
> +             !test_bit(PSR_SOCKET_L3_CDP, &info->feat_mask) )
> +        {
> +            feat = feat_l3_cdp;
> +            /* psr_cpu_prepare will allocate it on subsequent CPU onlining. 
> */
> +            feat_l3_cdp = NULL;
> +            feat->ops = l3_cdp_ops;
> +            l3_cdp_init_feature(regs, feat, info);
> +        } else {

I think someone else has already pointed out the style issue here, so
just in case ...

> @@ -1267,6 +1351,14 @@ static int psr_cpu_prepare(void)
>           (feat_l3_cat = xzalloc(struct feat_node)) == NULL )
>          return -ENOMEM;
>  
> +    if ( feat_l3_cdp == NULL &&
> +         (feat_l3_cdp = xzalloc(struct feat_node)) == NULL )
> +    {
> +        xfree(feat_l3_cat);
> +        feat_l3_cat = NULL;

Why the freeing? We've decided to allow for one node to be kept,
so no reason to free it on the error path here.

Jan


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