Re: [Xen-devel] [PATCH v10 10/25] x86: refactor psr: L3 CAT: set value: assemble features value array.

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

On 17-04-12 03:13:12, Jan Beulich wrote:
> >>> On 12.04.17 at 07:55, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > On 17-04-11 09:11:20, Jan Beulich wrote:
> >> >>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> >> > @@ -611,7 +679,40 @@ static int insert_val_to_array(uint32_t val[],
> >> >                                 enum cbm_type type,
> >> >                                 uint32_t new_val)
> >> >  {
> >> > -    return -EINVAL;
> >> > +    const struct feat_node *feat;
> >> > +    unsigned int i;
> >> > +
> >> > +    ASSERT(feat_type < PSR_SOCKET_MAX_FEAT);
> >> > +
> >> > +    /* Insert new value into array according to feature's position in 
> >> > array. */
> >> > +    for ( i = 0; i < feat_type; i++ )
> >> > +    {
> >> > +        feat = info->features[i];
> >> > +        if ( !feat )
> >> > +            continue;
> >> > +
> >> > +        if ( array_len <= feat->props->cos_num )
> >> > +            return -ENOSPC;
> >> > +
> >> > +        array_len -= feat->props->cos_num;
> >> > +
> >> > +        val += feat->props->cos_num;
> >> > +    }
> >> > +
> >> > +    feat = info->features[feat_type];
> >> > +    if ( !feat )
> >> > +        return -ENOENT;
> >> > +
> >> > +    if ( array_len < feat->props->cos_num )
> >> > +        return -ENOSPC;
> >> > +
> >> > +    if ( !psr_check_cbm(feat->props->cbm_len, new_val) )
> >> > +        return -EINVAL;
> >> > +
> >> > +    /* Value setting position is same as feature array. */
> >> > +    val[0] = new_val;
> >> 
> >> How come this is array index 0 unconditionally, when cos_num
> >> may be greater than 1?
> >> 
> > This patch is to implement L3 CAT so I do not introduce 'type[]' yet. The
> > mechanism will be introduced in CDP patch.
> 
> Which imo is wrong, as I've pointed out in a later patch. The
> moment you have the "cos_num" field (instead of implying 1
> everywhere) you ought to be dealing with the field having a
> value other than 1. Otherwise, as that later patch shows, you'd
> then re-write _part_ of the logic without in fact _newly_
> introducing "cos_num" being a variable.
> 
Ok, I will introduce the 'type[]' with 'cos_num' together from the beginning.

> Jan

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