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

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

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.

> > +                              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.

> Wei.

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