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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-03-08 08:15:33, Jan Beulich wrote:
> >>> 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?
> 
This is an error value used below, in 'psr_cbm_type_to_feat_type'. To make it
explicitly different, I assign a big value to it.

> > @@ -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.
> 
Sorry for that, will remove the comment.

> > @@ -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 ...
> 
Yes, Roger has suggested to change it to 'array_len != PSR_INFO_SIZE'.

> > +        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).
> 
This is defined in next version.

[...]
> > --- 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?
> 
Per Wei's suggestion to make codes neat.

> > +/* 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?
> 
Ok will do it in next version.

> Jan

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