Re: [Xen-devel] [PATCH v8 06/24] x86: refactor psr: implement get hw info flow.

["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
> @@ -84,6 +84,7 @@ enum psr_feat_type {
>      PSR_SOCKET_L3_CAT = 0,
>      PSR_SOCKET_L3_CDP,
>      PSR_SOCKET_L2_CAT,
> +    PSR_SOCKET_UNKNOWN = 0xFFFF,

Any reason to use this value, instead of just the next sequential one?

> @@ -182,6 +186,24 @@ static void free_feature(struct psr_socket_info *info)
>      }
>  }
>  
> +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type)
> +{
> +    enum psr_feat_type feat_type;
> +
> +    /* Judge if feature is enabled. */
> +    switch ( type )
> +    {
> +    case PSR_CBM_TYPE_L3:
> +        feat_type = PSR_SOCKET_L3_CAT;
> +        break;
> +    default:
> +        feat_type = PSR_SOCKET_UNKNOWN;
> +        break;
> +    }
> +
> +    return feat_type;
> +}

The comment ahead of the switch() doesn't seem to describe what's
being done - there certainly is no check here whether anything is
enabled or disabled.

> @@ -225,8 +247,22 @@ static unsigned int l3_cat_get_cos_max(const struct 
> feat_node *feat)
>      return feat->info.l3_cat_info.cos_max;
>  }
>  
> +static bool l3_cat_get_feat_info(const struct feat_node *feat,
> +                                 uint32_t data[], unsigned int array_len)
> +{
> +    if ( !data || 3 > array_len )

I think the 3 here was picked upon already: This check does not
guarantee there's no array overrun ...

> +        return false;
> +
> +    data[CBM_LEN] = feat->info.l3_cat_info.cbm_len;
> +    data[COS_MAX] = feat->info.l3_cat_info.cos_max;
> +    data[PSR_FLAG] = 0;

... anywhere here. For that you'd need a *_MAX- or *_NUM-type
constant (defined next to the array index constants).

> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -176,15 +176,19 @@ long arch_do_sysctl(
>          switch ( sysctl->u.psr_cat_op.cmd )
>          {
>          case XEN_SYSCTL_PSR_CAT_get_l3_info:
> -            ret = psr_get_cat_l3_info(sysctl->u.psr_cat_op.target,
> -                                      
> &sysctl->u.psr_cat_op.u.l3_info.cbm_len,
> -                                      
> &sysctl->u.psr_cat_op.u.l3_info.cos_max,
> -                                      &sysctl->u.psr_cat_op.u.l3_info.flags);
> +        {
> +            uint32_t data[3];
> +            ret = psr_get_info(sysctl->u.psr_cat_op.target,
> +                               PSR_CBM_TYPE_L3, data, 3);
> +
> +            sysctl->u.psr_cat_op.u.l3_info.cbm_len = data[CBM_LEN];
> +            sysctl->u.psr_cat_op.u.l3_info.cos_max = data[COS_MAX];
> +            sysctl->u.psr_cat_op.u.l3_info.flags   = data[PSR_FLAG];
>  
>              if ( !ret && __copy_field_to_guest(u_sysctl, sysctl, 
> u.psr_cat_op) )
>                  ret = -EFAULT;
>              break;
> -
> +        }

Please retain the blank line (after the } ).

> --- a/xen/include/asm-x86/psr.h
> +++ b/xen/include/asm-x86/psr.h
> @@ -19,19 +19,24 @@
>  #include <xen/types.h>
>  
>  /* CAT cpuid level */
> -#define PSR_CPUID_LEVEL_CAT   0x10
> +#define PSR_CPUID_LEVEL_CAT        0x10
>  
>  /* Resource Type Enumeration */
> -#define PSR_RESOURCE_TYPE_L3            0x2
> +#define PSR_RESOURCE_TYPE_L3       0x2
>  
>  /* L3 Monitoring Features */
> -#define PSR_CMT_L3_OCCUPANCY           0x1
> +#define PSR_CMT_L3_OCCUPANCY       0x1
>  
>  /* CDP Capability */
> -#define PSR_CAT_CDP_CAPABILITY       (1u << 2)
> +#define PSR_CAT_CDP_CAPABILITY     (1u << 2)
>  
>  /* L3 CDP Enable bit*/
> -#define PSR_L3_QOS_CDP_ENABLE_BIT       0x0
> +#define PSR_L3_QOS_CDP_ENABLE_BIT  0x0

Are all these adjustments really needed here?

> +/* Used by psr_get_info() */
> +#define CBM_LEN                    0
> +#define COS_MAX                    1
> +#define PSR_FLAG                   2

Neither comment nor names are helpful to understand the purpose of
these constants. How about PSR_INFO_IDX_* or some such?

Jan


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