Re: [Xen-devel] [PATCH v12 16/23] x86: L2 CAT: implement CPU init flow.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/14/17 3:26 AM >>>
> @@ -279,10 +281,14 @@ static void cat_init_feature(const struct cpuid_leaf 
> *regs,
>      switch ( type )
>      {
>      case PSR_SOCKET_L3_CAT:
> +    case PSR_SOCKET_L2_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);
>  
> -        wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len));
> +        if ( type == PSR_SOCKET_L3_CAT )
> +            wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len));
> +        else
> +            wrmsrl(MSR_IA32_PSR_L2_MASK(0), cat_default_val(feat->cbm_len));

Once again I think a conditional expression would yield in easier to read code,
as that would make even more obvious that the second argument is the same for
both cases.

> @@ -317,7 +323,8 @@ static void cat_init_feature(const struct cpuid_leaf 
> *regs,
>          return;
>  
>      printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
> -           ((type == PSR_SOCKET_L3_CDP) ? "CDP" : "L3 CAT"),
> +           ((type == PSR_SOCKET_L3_CDP) ? "CDP" :
> +            ((type == PSR_SOCKET_L3_CAT) ? "L3 CAT": "L2 CAT")),

At this point it would probably be better to have a static const lookup array
for the types, or for this descriptive string to be passed into the function.

> @@ -375,6 +382,12 @@ static const struct feat_props l3_cdp_props = {
>      .write_msr = l3_cdp_write_msr,
>  };
>  
> +/* L2 CAT props */
> +static const struct feat_props l2_cat_props = {
> +    .cos_num = 1,
> +    .type[0] = PSR_CBM_TYPE_L2,
> +};

Same remark as for CDP regarding the NULL function pointers left around here
until the later patches populate them.

> @@ -1407,6 +1424,19 @@ static void psr_cpu_init(void)
>          info->feat_init = true;
>      }
>  
> +    cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s);
> +    if ( regs.b & PSR_RESOURCE_TYPE_L2 )
> +    {
> +        cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s);
> +
> +        feat = feat_l2_cat;
> +        feat_l2_cat = NULL;
> +        feat_props[PSR_SOCKET_L2_CAT] = &l2_cat_props;
> +        cat_init_feature(®s, feat, info, PSR_SOCKET_L2_CAT);
> +
> +        info->feat_init = true;

This recurring setting of feat_init starts looking suspicious here. Why can't
this be done once at the end of the function, outside of any if()-s?

> --- a/xen/include/asm-x86/psr.h
> +++ b/xen/include/asm-x86/psr.h
> @@ -23,6 +23,7 @@
>  
>  /* Resource Type Enumeration */
>  #define PSR_RESOURCE_TYPE_L3            0x2
> +#define PSR_RESOURCE_TYPE_L2            0x4

These are used in psr.c only afaics, so shouldn't be put in a header.

Jan

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