Re: [Xen-devel] [PATCH v1] x86: psr: support co-exist features' values setting

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

On 17-10-06 15:38:35, Roger Pau Monn� wrote:
> On Fri, Oct 06, 2017 at 09:13:00AM +0000, Yi Sun wrote:
> > It changes the memebers in 'cos_write_info' to transfer the feature array,
> > feature properties array and value array. Then, we can write all features
> > values on the cos id into MSRs.
> > 
> > Because multiple features may co-exist, we need handle all features to write
> > values of them into a COS register with new COS ID. E.g:
> > 1. L3 CAT and L2 CAT co-exist.
> > 2. Dom1 and Dom2 share a same COS ID (2). The L3 CAT CBM of Dom1 is 0x1ff,
>                          ^ the
> >    the L2 CAT CBM of Dom1 is 0x1f.
> > 3. User wants to change L2 CBM of Dom1 to be 0xf. Because COS ID 2 is
> >    used by Dom2 too, we have to pick a new COS ID 3. The values of Dom1 on
> >    COS ID 3 are all default values as below:
> >            ---------
> >            | COS 3 |
> >            ---------
> >    L3 CAT  | 0x7ff |
> >            ---------
> >    L2 CAT  | 0xff  |
> >            ---------
> > 4. After setting, the L3 CAT CBM value of Dom1 should be kept and the new L2
> >    CAT CBM is set. So, the values on COS ID 3 should be below.
> >            ---------
> >            | COS 3 |
> >            ---------
> >    L3 CAT  | 0x1ff |
> >            ---------
> >    L2 CAT  | 0xf   |
> >            ---------
> > 
> > So, we should write all features values into their MSRs. That requires the
> > feature array, feature properties array and value array are input.
>                                                           ^ as?
> 
> I'm not sure the last sentence is helpful.
> 
Ok, will remove it.

> > 
> > Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> > ---
> > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> > CC: Roger Pau Monn? <roger.pau@xxxxxxxxxx>
> > CC: Julien Grall <julien.grall@xxxxxxx>
> > ---
> >  xen/arch/x86/psr.c | 51 +++++++++++++++++++++++++++------------------------
> >  1 file changed, 27 insertions(+), 24 deletions(-)
> > 
> > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> > index daa2aeb..596b0ca 100644
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -1111,25 +1111,40 @@ static unsigned int get_socket_cpu(unsigned int 
> > socket)
> >  struct cos_write_info
> >  {
> >      unsigned int cos;
> > -    struct feat_node *feature;
> > +    struct feat_node **features;
> >      const uint32_t *val;
> > -    const struct feat_props *props;
> > +    unsigned int array_len;
> >  };
> >  
> >  static void do_write_psr_msrs(void *data)
> >  {
> >      const struct cos_write_info *info = data;
> > -    struct feat_node *feat = info->feature;
> > -    const struct feat_props *props = info->props;
> > -    unsigned int i, cos = info->cos, cos_num = props->cos_num;
> > +    unsigned int i, index = 0, cos = info->cos;
> > +    const uint32_t *val_array = info->val;
> >  
> > -    for ( i = 0; i < cos_num; i++ )
> > +    for ( i = 0; i < ARRAY_SIZE(feat_props); i++ )
> >      {
> > -        if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] )
> > +        struct feat_node *feat = info->features[i];
> > +        const struct feat_props *props = feat_props[i];
> > +        unsigned int cos_num, j;
> > +
> > +        if ( !feat || !props )
> > +            continue;
> > +
> > +        cos_num = props->cos_num;
> > +        if ( info->array_len < index + cos_num )
> 
> Shouldn't this be '<='? index + cos_num is an index position with base
> 0 AFAICT.
> 
Nope. E.g. there are L2 CAT and CDP co-exist. cos_num of L2 is 1, cos_num of CDP
is 2. CDP is the first element in feature array. array_len is 3.
1. First loop to handle CDP: index is changed from 0 to 2.
2. Second loop to handle L2:
    cos_num = 1;
    index + cos_num = 3;
    array_len = 3;

So, we must use '<' here to check if overflow happens.

> > +            return;
> > +
> > +        for ( j = 0; j < cos_num; j++ )
> >          {
> > -            feat->cos_reg_val[cos * cos_num + i] = info->val[i];
> > -            props->write_msr(cos, info->val[i], props->type[i]);
> > +            if ( feat->cos_reg_val[cos * cos_num + j] != val_array[index + 
> > j] )
> 
> I'm afraid this code could benefit from a comment (or comments)
> explaining what all this arrays are supposed to contain. IMHO it's not
> trivial to follow what you are trying to do here.
> 
Will add comment.

> Also names like val_array are not specially helpful, it's quite clear
> that 'val_array' is an array just by looking at it's usage.
> 
Ok, may consider to remove 'val_array'.

> Thanks, Roger.

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