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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.10.17 at 09:20, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- 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;
>      const uint32_t *val;
> -    const struct feat_props *props;
> +    unsigned int array_len;
>  };

The addition wants to go into the hole after "cos".

>  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;
> +    struct cos_write_info *info = data;

const

> +    unsigned int i, index = 0, cos = info->cos;
> +    struct psr_socket_info *socket_info =

const

> +                            
> get_socket_info(cpu_to_socket(smp_processor_id()));
>  
> -    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 = socket_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;
> +        ASSERT(info->array_len >= index + cos_num);

While this transformation from the original -ENOSPC return looks to
be correct, but not obviously so, it would have been a good idea
to mention this in the commit message. After all the above can be
correct only if the original -ENOSPC return path could have been
an ASSERT() as well.

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

Looks like I only meant to comment on the uses of index above:
If you incremented it alongside j, you could use just index in the
respective array accesses, and you'd avoid the last statement
above altogether.

In the interest of getting the patch in I'll see to make the
adjustments myself. Please double check the result in case I end
up committing what I've come up with.

Jan


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