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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sun, Oct 08, 2017 at 04:22: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 the same COS ID (2). The L3 CAT CBM of Dom1 is 0x1ff,
>    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   |
>            ---------
> 
> 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 | 54 
> ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index daa2aeb..dbf7a4c 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -1111,25 +1111,43 @@ 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)

Should this be "static int do_write_psr_msrs"...

>  {
>      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;
>  
> -    for ( i = 0; i < cos_num; i++ )
> +    /*
> +     * Iterate all featuers to write different value (not same as MSR) for
> +     * each feature.
> +     */
> +    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 )
> +            return;

So that you can return -ENOSPC here (inline with what was previously
done in write_psr_msrs)?

> +
> +        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] != info->val[index + 
> j] )
> +            {
> +                feat->cos_reg_val[cos * cos_num + j] = info->val[index + j];
> +                props->write_msr(cos, info->val[index + j], props->type[j]);
> +            }
>          }
> +
> +        index += cos_num;
>      }
>  }
>  
> @@ -1137,30 +1155,18 @@ static int write_psr_msrs(unsigned int socket, 
> unsigned int cos,
>                            const uint32_t val[], unsigned int array_len,
>                            enum psr_feat_type feat_type)
>  {
> -    int ret;
>      struct psr_socket_info *info = get_socket_info(socket);
>      struct cos_write_info data =
>      {
>          .cos = cos,
> -        .feature = info->features[feat_type],
> -        .props = feat_props[feat_type],
> +        .features = info->features,
> +        .val = val,
> +        .array_len = array_len,
>      };
>  
>      if ( cos > info->features[feat_type]->cos_max )
>          return -EINVAL;
>  
> -    /* Skip to the feature's value head. */
> -    ret = skip_prior_features(&array_len, feat_type);
> -    if ( ret < 0 )
> -        return ret;
> -
> -    val += ret;
> -
> -    if ( array_len < feat_props[feat_type]->cos_num )

When moved inside of do_write_psr_msrs this becomes:

info->array_len < index + cos_num

Where cos_num == feat_props[feat_type]->cos_num. Is this correct?

I'm asking because the check used to be array_len < cos_num.

Thanks, Roger.

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