Re: [Xen-devel] [PATCH v8 09/24] x86: refactor psr: set value: assemble features value array.

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Feb 27, 2017 at 03:11:35PM +0800, Yi Sun wrote:
> On 17-02-26 17:43:04, Wei Liu wrote:
> > On Wed, Feb 15, 2017 at 04:49:24PM +0800, Yi Sun wrote:
> > [...]
> > >  
> > > +static unsigned int l3_cat_get_cos_num(const struct feat_node *feat)
> > > +{
> > > +    return 1;
> > > +}
> > > +
> > > +static int l3_cat_get_old_val(uint64_t val[],
> > 
> > And the length of val is? How can you bound-check the access?
> > 
> > But I *think* this is just a pointer to uint64_t, you can just use
> > uint64_t *val here and *val = x; in code?
> > 
> > Same comment applies to the set_new_val handler as well.
> > 
> Such implementation is to simplify these functions. The bound-check will be
> done in caller functions, such as combine_val_array/set_new_val_to_array/etc.
> 

Please consider adding a comment to say bound-check is don't by the
caller.

> > > +                              const struct feat_node *feat,
> > > +                              unsigned int old_cos)
> > > +{
> > > +    if ( old_cos > feat->info.l3_cat_info.cos_max )
> > > +        /* Use default value. */
> > > +        old_cos = 0;
> > > +
> > > +    /* CAT */
> > > +    val[0] =  feat->cos_reg_val[old_cos];
> > > +
> > > +    return 0;
> > > +}
> > > +
> [...]
> > > -static int assemble_val_array(uint64_t *val,
> > > +static int combine_val_array(uint64_t *val,
> > >                                uint32_t array_len,
> > >                                const struct psr_socket_info *info,
> > >                                unsigned int old_cos)
> > 
> > Please just name this function combine_val_array in your previous patch
> > instead of trying to rename it here. Or just don't change the name at
> > all -- I don't see why changing name is necessary.
> > 
> Per Konrad's suggestion to change the name. Because he thought the 'assemble'
> is not accurate here.
> 

The such change should be inside the patch to introduce the framework,
not in this patch.

Wei.

> > Wei.

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